REGRESSION(r287787): CKEditor < v4.11 relies on WebKit emitting "DOMFocusIn" / "DOMFocusOut" events

• Component: DOM
• Status: RESOLVED INVALID

[Alexey Shvayka]

REGRESSION(r287787): CKEditor < v4.10 relies on WebKit emitting "DOMFocusIn" / "DOMFocusOut" events

[Alexey Shvayka]

Created attachment 448774 [details]
Patch

[Alexey Shvayka]

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.

[Alexey Shvayka]

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.

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #3)
> Although CKEditor v4.9 seems to work even with "DOMFocusIn" / "DOMFocusOut"

*without*

[Darin Adler]

We have to figure out why we are seeing failures on the "win" EWS bot for every patch.

[Alexey Shvayka]

(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.

[Alexey Shvayka]

(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.

[Radar WebKit Bug Importer]

<rdar://problem/87685583>

[Alexey Shvayka]

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.