Bug 235036 - REGRESSION(r287787): CKEditor < v4.11 relies on WebKit emitting "DOMFocusIn" / "DOMFocusOut" events
Summary: REGRESSION(r287787): CKEditor < v4.11 relies on WebKit emitting "DOMFocusIn" ...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-10 10:47 PST by Alexey Shvayka
Modified: 2022-01-20 13:34 PST (History)
10 users (show)

See Also:


Attachments
Patch (11.85 KB, patch)
2022-01-10 10:49 PST, Alexey Shvayka
darin: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2022-01-10 10:47:00 PST
REGRESSION(r287787): CKEditor < v4.10 relies on WebKit emitting "DOMFocusIn" / "DOMFocusOut" events
Comment 1 Alexey Shvayka 2022-01-10 10:49:29 PST
Created attachment 448774 [details]
Patch
Comment 2 Alexey Shvayka 2022-01-10 12:14:46 PST
Comment on attachment 448774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448774&action=review

> Source/WebCore/ChangeLog:14
> +        show up in Chrome stats due to UA-conditional code.

Hmm, taking a fresh look I don't see why

    editable.attachListener( editable, CKEDITOR.env.webkit ? 'DOMFocusIn' : 'focus', function()

wouldn't show up in a Chrome stats...

Feature usage counter seems to be in place: https://github.com/chromium/chromium/blob/0c27745e3c88ecab65fb1180d7033f936eb3a965/third_party/blink/renderer/core/dom/events/event_target.cc#L192-L193.
Comment 3 Alexey Shvayka 2022-01-10 12:56:53 PST
Although CKEditor v4.9 seems to work even with "DOMFocusIn" / "DOMFocusOut" events, we can't rely on Chrome counters for this, and there might other code performing UA-testing to work around Firefox but not Safari.
Comment 4 Alexey Shvayka 2022-01-10 12:57:12 PST
(In reply to Alexey Shvayka from comment #3)
> Although CKEditor v4.9 seems to work even with "DOMFocusIn" / "DOMFocusOut"

*without*
Comment 5 Darin Adler 2022-01-10 14:43:58 PST
We have to figure out why we are seeing failures on the "win" EWS bot for every patch.
Comment 6 Alexey Shvayka 2022-01-10 15:05:25 PST
(In reply to Darin Adler from comment #5)

Thanks for review, Darin!

I'm going to investigate the impact of removing legacy event types on CKEditor < v4.11 before deciding if we should bring them back.

(In reply to Alexey Shvayka from comment #2)
> Hmm, taking a fresh look I don't see why
> 
>     editable.attachListener( editable, CKEDITOR.env.webkit ? 'DOMFocusIn' :
> 'focus', function()
> 
> wouldn't show up in a Chrome stats...

It appears the code above is invoked very rarely, only with a particular add-on to CKEditor < v4.11, and causes only a minor issue to UX, which Firefox also suffered from. Not a show-stopper.

Chrome has a single method to count event type usage, so it's unlikely the stats are incorrect.

While we can indeed preserve the optimization while still emitting these events, removing them is a small step towards making web platform more compatible.

Any advice is appreciated.
Comment 7 Alexey Shvayka 2022-01-10 15:07:03 PST
(In reply to Darin Adler from comment #5)
> We have to figure out why we are seeing failures on the "win" EWS bot for
> every patch.

streams/readable-stream-lock-after-worker-terminates-crash.html timeout may be related to https://bugs.webkit.org/show_bug.cgi?id=234890, although the "win" EWS is green for the patch itself.
Comment 8 Radar WebKit Bug Importer 2022-01-17 10:47:16 PST
<rdar://problem/87685583>
Comment 9 Alexey Shvayka 2022-01-20 13:34:06 PST
Closing per https://bugs.webkit.org/show_bug.cgi?id=235036#c6: I've extensively tested ToT with CKEditor v4.9, everything works as expected, including text (de-)selection.