Remove focus event from RichText #44304
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -29 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
youknowriad
commented
Sep 20, 2022
| }; | ||
| onSelectionChange( index, index ); | ||
| } else { | ||
| if ( isSelected ) { | ||
| applyRecord( record.current ); | ||
| onSelectionChange( record.current.start, record.current.end ); | ||
| } |
There was a problem hiding this comment.
It turns out, this is the only "useful" code here that doesn't duplicate selectionchange event. It's useful when applying formats (like links), that way the selection is restored at the right position after inserting links. Maybe we could even move this code elsewhere (when we actually update record.current) and avoid the event handler entirely but I'm not familiar enough with this code to try it.
youknowriad
commented
Sep 20, 2022
| end: index, | ||
| activeFormats: EMPTY_ACTIVE_FORMATS, | ||
| }; | ||
| onSelectionChange( index, index ); |
There was a problem hiding this comment.
The main goal for my explorations on this PR is to get rid of this, which was causing onSelectionChange to be called twice on focus, I'm assuming it's already done in the "selectionchange" event.
There was a problem hiding this comment.
If it's called twice, it will be called first here, then in the "selectionchange" event.
|
Check what this does to the "focus" metric.
The reality is that the "focus" metric is not complete, we're only counting the "focus" event, while now "focus" doesn't do much but the re-rendering is moved to "focus-in".
I suspect that the current PR still improves the "block selection" metric but our current metric is not representative. |
|
Is this just for performance? |
yes, this is my main motivation #44304 (comment) |
ellatrix
reviewed
Sep 21, 2022
| // frame. The event listener for selection changes may be added too late | ||
| // at this point, but this focus event is still too early to calculate | ||
| // the selection. | ||
| rafId = defaultView.requestAnimationFrame( handleSelectionChange ); |
There was a problem hiding this comment.
Should we first try to remove it without removing this?
There was a problem hiding this comment.
It's not clear to me why we need this since when "focus" happens, the selection changes so the callback is called no?
There was a problem hiding this comment.
I'm just curious which of these changes makes which tests fail, so we can address it more easily.
There was a problem hiding this comment.
I'm just curious which of these changes makes which tests fail, so we can address it more easily.
There was a problem hiding this comment.
I'm just curious which of these changes makes which tests fail, so we can address it more easily.
There was a problem hiding this comment.
Looks like everything passes now that I restored that part
There was a problem hiding this comment.
I'd say let's look into that separately then.
youknowriad
requested review from
fluiddot,
ntwb,
nerrad and
ajitbohra
as code owners
youknowriad
force-pushed
the
try/remove-focus-event
branch
from
11a7c7e
to
a136a6b
Compare
|
I updated the way we compute the "focus" metric, that way we will have better results in the future. It now includes "focus", "focusin", "focusout" and "selectionchange" events. |
youknowriad
force-pushed
the
try/remove-focus-event
branch
from
a136a6b
to
d89fb70
Compare
youknowriad
force-pushed
the
try/remove-focus-event
branch
from
d89fb70
to
ba8f297
Compare
|
Changes similar to this PR have been merged already. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just trying to understand the purpose of this event listener and if there's any impact.