This shows up in profiles of the benchmark: https://share.firefox.dev/3KXIQOB

If I add an early return to Element::Focus, the benchmark score on Try (Linux64 Shippable) improves from 32.5 ms to 28.5 ms, so maybe there are ways to improve this.

EditorEventListener::Focus synchronously flushes style and layout information: https://searchfox.org/mozilla-central/rev/6050205f3174fd24c2b6be11c69ecd788cd2b6b3/editor/libeditor/EditorEventListener.cpp#1149

Can we do that more lazily, or avoid doing it altogether?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

EditorEventListener::Focus synchronously flushes style and layout information: https://searchfox.org/mozilla-central/rev/6050205f3174fd24c2b6be11c69ecd788cd2b6b3/editor/libeditor/EditorEventListener.cpp#1149

Can we do that more lazily, or avoid doing it altogether?

No, it's required for checking whether the editor has already been destroyed by a reframing. If TextControlState can manage it without flushing layout, it can notify editor of that instead. Then, we can get rid of the ugly flush.

For example, check this case: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/editing/crashtests/contenteditable-will-be-blurred-by-focus-event-listener.html

Not flushing is tricky.

See also
https://bugzilla.mozilla.org/show_bug.cgi?id=1369140 and
https://bugzilla.mozilla.org/show_bug.cgi?id=1377447

I wonder if editor could use similar type of flushing as nsFocusManage, EnsurePresShellInitAndFrames

How do I run matrix-react benchmark?

(In reply to Olli Pettay [:smaug] from comment #6)

How do I run matrix-react benchmark?

See code and README here: https://github.com/mozilla-spidermonkey/matrix-react-bench The benchmark score is logged to the web console.
Let me know if you run into any problems with that.

It's also available as a Browsertime CI job, matrix-react-benchmark. It's possible to run that version locally but I don't know how to do that.

Or, hmm, EnsurePresShellInitAndFrames wouldn't help, since it is the style which is slow here.

From point of view of editor, it can avoid calling FlushPendingNotifications if it can check with a cheaper way whther the corresponding frame may have already been destroyed or not. If not (it should be the normal case), editor does not require to flush other pending notifications.

This also happens on Matrix and to some degree on Slack

Matrix profile: https://share.firefox.dev/3TlO3Ey
Slack profile: https://share.firefox.dev/3QOCp3w

(Search for HTMLElement.focus if you are lost in those profiles)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #9)

From point of view of editor, it can avoid calling FlushPendingNotifications if it can check with a cheaper way whther the corresponding frame may have already been destroyed or not. If not (it should be the normal case), editor does not require to flush other pending notifications.

What you mean with this? If you have access to a frame, it is not destroyed, right? If you just need to know whether it is deleted by some operation you can use WeakFrame/AutoWeakFrame to detect that.

The Performance Priority Calculator has determined this bug's performance priority to be P1. If you'd like to request re-triage, you can reset the Performance flag to "?" or needinfo the triage sheriff.

Platforms: [x] Windows [x] macOS [x] Linux [x] Android
Impact on site: Causes noticeable jank
Websites affected: Major
[x] Able to reproduce locally
[x] Bug affects multiple sites

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] from comment #11)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #9)

From point of view of editor, it can avoid calling FlushPendingNotifications if it can check with a cheaper way whther the corresponding frame may have already been destroyed or not. If not (it should be the normal case), editor does not require to flush other pending notifications.

What you mean with this? If you have access to a frame, it is not destroyed, right? If you just need to know whether it is deleted by some operation you can use WeakFrame/AutoWeakFrame to detect that.

I meant that if flushing pending notifications causes destroying the corresponding nsTextControlFrame (e.g., hiding a parent element), TextEditor shouldn't handle focus event (bug 1755104) because it'll set Selection even though it has already lost the rights to change the DOM tree (TextEditor and its anonymous subtree are recreated every reframing).

Therefore, if TextEditor can check whether it will keep having focus or will lose focus after flushing pending notifications without actually doing it, it can avoid flushing them.

I'll try to write a patch to stop flushing pending things in most cases.

The severity field for this bug is set to S3. However, the Performance Impact field flags this bug as having a high impact on the performance.
:masayuki, could you consider increasing the severity of this performance-impacting bug? Alternatively, if you think the performance impact is lower than previously assessed, could you request a re-triage from the performance team by setting the Performance Impact flag to ??

For more information, please visit auto_nag documentation.

Hmm, looks like that retrieving all parent styles is not so cheap in the worst case (e.g., the <input> or <textarea> is too deep node).

I wonder, IMEContentObserver sends notifications in next tick which must be 41ms at worst (in 24fps). This could be enough safe to wait next keyboard input which should be handled by IME (the IPC of focus notification is sync so that the delay is minimized). Fortunately, it's very early state of current cycle. It's worthwhile to try it.

There are some new failures. I need to update the tests.
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=dM7rCH-GTwuW3QqTOj9t2Q.0&revision=b009ce7793b8efcc34f279a1f2f30db1c219f939

Making it async causes odd behavior of TSFTextStore. I guess that it's caused by TSFTextStore may create its instance at SetInputContext too, but cleaned up at NOTIFY_IME_OF_BLUR. This hack makes me suspect this. I keep investigating...

Some updates:

1. Making NOTIFY_IME_OF_FOCUS notification async works fine in most cases.
2. Except for virtual keyboard management, it causes flickering virtual keyboard on Android when e.blur(), e.focus(); since the latter focus notification is sent later than now
3. For solving the VKB issue, we also need to make NOTIFY_IME_OF_BLUR notification async
4. It fixes the issue, however, it means that the order of setting input context and NOTIFY_IME_OF_BLUR notification is swapped.
5. Perhaps, we should put off to setting input context to disabled at sending NOTIFY_IME_OF_BLUR, but requires more changes.