Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Special characters all / latin categories takes noticable longer time to load than others #6161

Closed
mlewand opened this issue Jan 29, 2020 · 8 comments
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:special-characters resolution:expired This issue was closed due to lack of feedback. squad:core Issue to be handled by the Core team. status:stale type:improvement This issue reports a possible enhancement of an existing feature. type:performance This issue reports a performance issue or a possible performance improvement.

Comments

@mlewand
Copy link
Contributor

mlewand commented Jan 29, 2020

📝 Provide a description of the improvement

Both categories hangs the browser for a brief (but notable) moment.

Since the special characters plugin is not yet out, it can be checked on nightly docs: https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/features/special-characters.html

Although I'm not convinced it's going to be perceived by most users so I'm just reporting it here.

📃 Other details

  • Browser: Chrome
  • OS: Windows 10
  • CKEditor version: current master (iteration 29)

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@mlewand mlewand added type:improvement This issue reports a possible enhancement of an existing feature. domain:ui/ux This issue reports a problem related to UI or UX. package:special-characters type:performance This issue reports a performance issue or a possible performance improvement. labels Jan 29, 2020
@jodator
Copy link
Contributor

jodator commented Jan 29, 2020

Although I'm not convinced it's going to be perceived by most users so I'm just reporting it here.

I'm in your group :( The delay is related to the size of the view and for me, it is noticeable.

The "all" feels like there's something blocked (guessing its load time is > 500ms or even close to 1s).

The "Latin" gives the impression that something might lag but is kinda OK (it is not "instantaneous" like "arrows" or other smaller ones).

@Reinmar
Copy link
Member

Reinmar commented Feb 3, 2020

This is very... disturbing.

This is the whole timeline:

image

It consists of dozens of:

image

What's even worse: after the recent change to show "All" as the default option, this plugin is now rendering all that on editor init:

image

In other words: most of the editor bootstrap time is now used by special characters button.

@Reinmar Reinmar added this to the iteration 30 milestone Feb 3, 2020
@Reinmar
Copy link
Member

Reinmar commented Feb 3, 2020

So, the upgradeGrid() is just terrible (not because of its implementation but because of how the UI lib works). It'd be hard to quickly make here some improvements.

image

However, if we look closer into where's the time spent in it, there's a room for improvement here:

image

First, it would help a lot for the entire editor if we improved listenTo(). Second, uid() should not take that much time :O

But nevertheless – the very first thing to do is to make sure that rendering this panel does not block rendering the editor: #6175.

@Reinmar
Copy link
Member

Reinmar commented Feb 3, 2020

I reported my generic findings in #6178.

As for optimizations that can be done here:

  • uid seems overly slow (worth max 1h research),
  • removing ButtonView dependency (easy),
  • reusing tile views when switching between character groups – right now we create new ones every time you change the group (relatively easy).

@oleq
Copy link
Member

oleq commented Jul 21, 2020

I checked today and Collection#addMany shaves ~20ms off the full grid creation time. So this is another micro-improvement we can use.

@oleq
Copy link
Member

oleq commented Jul 21, 2020

And honestly, the best result I came up with was using the following code which utilizes the time the user interaction gives us to perform heavy tasks.

  • It starts generating all the tiles when the dropdown button is being hovered (not clicked).
    • It takes some time for the user to hover the button and then click it. 
    • I figured we can use this time to our advantage. I'm using this time to generate tiles.
  • Additionally, to not disrupt the main browser thread, I used setTimeout
    • FYI: It also works without setTimeout but then a lag connected with opening the dropdown becomes visible on the :hover style of the dropdown button. 

It's now lightning fast.

diff --git a/packages/ckeditor5-special-characters/src/specialcharacters.js b/packages/ckeditor5-special-characters/src/specialcharacters.js
index d6ffc606f1..e187a9a6b2 100644
--- a/packages/ckeditor5-special-characters/src/specialcharacters.js
+++ b/packages/ckeditor5-special-characters/src/specialcharacters.js
@@ -94,20 +94,28 @@ export default class SpecialCharacters extends Plugin {
 			} );
 
 			dropdownView.on( 'change:isOpen', () => {
-				if ( !dropdownPanelContent ) {
-					dropdownPanelContent = this._createDropdownPanelContent( locale, dropdownView );
-
-					dropdownView.panelView.children.add( dropdownPanelContent.navigationView );
-					dropdownView.panelView.children.add( dropdownPanelContent.gridView );
-					dropdownView.panelView.children.add( dropdownPanelContent.infoView );
-				}
-
 				dropdownPanelContent.infoView.set( {
 					character: null,
 					name: null
 				} );
 			} );
 
+			dropdownView.buttonView.extendTemplate( {
+				on: {
+					mouseenter: dropdownView.buttonView.bindTemplate.to( () => {
+						if ( !dropdownPanelContent ) {
+							setTimeout( () => {
+								dropdownPanelContent = this._createDropdownPanelContent( locale, dropdownView );
+
+								dropdownView.panelView.children.add( dropdownPanelContent.navigationView );
+								dropdownView.panelView.children.add( dropdownPanelContent.gridView );
+								dropdownView.panelView.children.add( dropdownPanelContent.infoView );
+							} );
+						}
+					} )
+				}
+			} );
+
 			return dropdownView;
 		} );
 	}
@@ -202,12 +210,11 @@ export default class SpecialCharacters extends Plugin {
 		gridView.tiles.clear();
 
 		const characterTitles = this.getCharactersForGroup( currentGroupName );
+		const tileViews = Array.from( characterTitles ).map( title => {
+			return gridView.createTile( this.getCharacter( title ), title );
+		} );
 
-		for ( const title of characterTitles ) {
-			const character = this.getCharacter( title );
-
-			gridView.tiles.add( gridView.createTile( character, title ) );
-		}
+		gridView.tiles.addMany( tileViews );
 	}
 
 	/**

@oleq oleq added the squad:red label Jul 21, 2020
@Reinmar Reinmar added squad:core Issue to be handled by the Core team. and removed squad:red labels Jul 28, 2020
@pomek pomek removed this from the nice-to-have milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Nov 10, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:special-characters resolution:expired This issue was closed due to lack of feedback. squad:core Issue to be handled by the Core team. status:stale type:improvement This issue reports a possible enhancement of an existing feature. type:performance This issue reports a performance issue or a possible performance improvement.
Projects
None yet
Development

No branches or pull requests

6 participants