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

Research blocking rendering selection while selection is being made #10430

Closed
Reinmar opened this issue Aug 26, 2021 · 3 comments
Closed

Research blocking rendering selection while selection is being made #10430

Reinmar opened this issue Aug 26, 2021 · 3 comments
Assignees
Labels
package:engine squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@Reinmar
Copy link
Member

Reinmar commented Aug 26, 2021

Provide a description of the task

Research needed to move forward #10230.

Scope: Researching this point:

Make sure we don't touch the selection while the selection is in progress

Source: #10230 (comment)

@Reinmar Reinmar added type:task This issue reports a chore (non-production change) and other types of "todos". squad:core Issue to be handled by the Core team. labels Aug 26, 2021
@Reinmar
Copy link
Member Author

Reinmar commented Aug 26, 2021

  • Introduce some sort of "selection in progress" tracking mechanism
  • Plug it into the Renderer, so to make it stop rendering the selection when "selection in progress" is "true"
  • Test it for surprising side-effects; analysis, writing down conclusions
  • Estimate: 3
  • Note: This overlaps with beforeInput and IME. We'll also have to prevent rendering there when selection changes during composition. Most likely, we'll have to
  • Note: Slightly related to T/460 Selection rendering breaks composition ckeditor5-engine#861

@oleq
Copy link
Member

oleq commented Sep 20, 2021

There are two separate problems that contribute to the glitches with mouse selection anchoring at links/markers/mentions:

DOM selection on the wrong side of the inline filler

We never want to have the DOM selection start before the inline filler (not sure why, though). But this sometimes happens randomly as the user starts selection at a different point (geometrically, using the mouse).

And we fix this if it happens. And as we fix it, we break the flow of the user selection https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-engine/src/view/domconverter.js#L1013-L1016

// What user did by accident 
// (also, imagine they are moving mouse right to select more)
<p>[]&NBR;&NBR;&NBR;&NBR;&NBR;&NBR;&NBR;<a>FOO</a></p>

// We fix into by imperatively collapsing it in DOM.
<p>&NBR;&NBR;&NBR;&NBR;&NBR;&NBR;&NBR;[]<a>FOO</a></p>

// Which breaks the flow of creating the selection using mouse;
// the selection remains collapsed forever despite moving mouse right.
// BTW: The filler will be also removed (see the second problem) in this case so 
// this HTML is just to give you the idea of what happened.
<p>&NBR;&NBR;&NBR;&NBR;&NBR;&NBR;&NBR;<a>FO[]O</a></p>

This could be fixed by blocking the imperative rendering of the DOM selection while the selection is being made (which is the scope of the research).

  • After this fix, the original issue is harder to reproduce (yet still exists).
  • The PoC of the fix.

The disappearing inline filler

While the selection is being made, the inline filler disappears because it's no longer needed. When it disappears, the DOM selection anchored in that filler is collapsed by the web browser to the focus position. Then, as the user continues moving the cursor using the mouse, it stays collapsed forever.

// That's cool. We need this filler.
<p>&NBR;&NBR;&NBR;&NBR;&NBR;&NBR;&NBR;[]<a>FOO</a></p>

// But now the inline filler is obsolete...
<p>&NBR;&NBR;&NBR;&NBR;&NBR;&NBR;&NBR;[<a>F]OO</a></p>

// So the filler is out and the browser DOM selection collapses to the focus position.
// The flow of extending the selection using the mouse was broken.
<p><a>F[]OO</a></p>

This could be fixed by keeping the inline filler as long as the selection stays in the same block.

  • And what's important, this must be the exact same text node/filler. If we replace one filler with another, the flow of extending the selection will also break.
  • FYI: You can play with keeping the filler for non-collapsed selections in _needsInlineFillerAtSelection() and _isSelectionInInlineFiller()
    • (Not 100% sure of it) Keep in mind that the logic in these methods is probably flawed because it always considers the first position of the selection only. Needs a thorough check.
    • Together with @niegowski, we tried this briefly but it still replaced the inline filler each time with a new text node. So a deeper/smarter/thought-through fix would be necessary. Since this was outside of the scope of research it makes for a good follow-up.
  • A side-question to answer: Why we decided 5y ago to have fillers for collapsed selection only?

@oleq
Copy link
Member

oleq commented Sep 21, 2021

The research will be followed by a fix in #10562. 

The permanent inline fillers part of the research was moved to #10563.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

2 participants