Bug 167525 – Regression (Safari 10.1): Pressing Return in a contenteditable no longer inserts a line break under certain conditions

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Sam Stephenson]

Created attachment 299951 [details]
Minimal test case

Pressing the Return key in a contenteditable element no longer inserts a line break under the following conditions:

1) A mutation observer is observing subtree changes to the editor element and synchronously replacing the contents of the editor element in response.

2) An input event listener is installed anywhere on the page*.

3) The window's selection | is collapsed ahead of two line breaks, in the following configuration: <div>|<br></div><div><br></div>

This behavior is new in Safari 10.1/Safari Technology Preview, and is not present in other major browsers, including older versions of Safari.

Attached please find a minimal test case (contenteditable-return.html). Use the following steps to reproduce the issue:

1) Open the test case file. The element outlined in red is a contenteditable element. Observe that the contents of the editor are a <div> with a single <br> inside. The editor has focus and the window's selection is collapsed just before the <br>.

2) Press Return. Observe that the editor grows by one line in height, as expected. The contents of the editor are now <div><br></div><div><br></div>. The script on the page restores the cursor to its previous position, before the first <br>.

3) Press Return again. Observe that the editor, unexpectedly, does not grow in height. The contents of the editor are unchanged.

The test case is a simplified representation of the mechanics of the Trix rich text editor (https://github.com/basecamp/trix), which monitors mutations and input events to update an internal document model, and then renders that model back to the contenteditable element as it changes.

* Note that on at least one computer, I am able to reproduce the issue without an input event listener installed. That it is sensitive to the timing of different computers seems to suggest a race condition. I speculate that presence of an input event listener causes WebKit to do additional work on the JS thread that it wouldn't otherwise do, triggering the symptoms above.

[Radar WebKit Bug Importer]

<rdar://problem/30270210>

[Wenson Hsieh]

Oh shoot, this is because I accidentally changed:

element.dispatchEvent(InputEvent::create(eventNames().inputEvent, inputType, true, false, element.document().defaultView(), data, WTFMove(dataTransfer), targetRanges, 0));

...to:

element.dispatchScopedEvent(InputEvent::create(eventNames().inputEvent, inputType, true, false, element.document().defaultView(), data, WTFMove(dataTransfer), targetRanges, 0));

...while refactoring. I'll fix this ASAP and write a layout test. Thanks, Sam!

[Wenson Hsieh]

(In reply to comment #2)
> Oh shoot, this is because I accidentally changed:
> 
> element.dispatchEvent(InputEvent::create(eventNames().inputEvent, inputType,
> true, false, element.document().defaultView(), data, WTFMove(dataTransfer),
> targetRanges, 0));
> 
> ...to:
> 
> element.dispatchScopedEvent(InputEvent::create(eventNames().inputEvent,
> inputType, true, false, element.document().defaultView(), data,
> WTFMove(dataTransfer), targetRanges, 0));
> 
> ...while refactoring. I'll fix this ASAP and write a layout test. Thanks,
> Sam!

(actually the other way around: we used to dispatchScopedEvent, and now we dispatch the event immediately)

[Ryosuke Niwa]

Was this change merged into our branch? If so, we should fix it in the branch as well.

[Wenson Hsieh]

Yes, the breaking change was merged into the branch. I'll put out a fix today.

[Wenson Hsieh]

Created attachment 300157 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 300157 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        We should not be dispatching the input event immediately, but rather queue the event for dispatch. This
> +        regressed from recent work related to input events, and affects rich text editors.

Wow, this matters!? Scoped event isn't really a spec concept so I'd imagine this means "input" event needs to be async.

[Wenson Hsieh]

Wait: according to https://www.w3.org/TR/uievents/#events-inputevents, input events are sync, not async. Our old behavior was async, and I changed it to be sync during my refactoring.

[Build Bot]

Comment on attachment 300157 [details]
Patch

Attachment 300157 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2976273

New failing tests:
fast/events/input-events-drag-and-drop.html

[Build Bot]

Created attachment 300161 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300157 [details]
Patch

Attachment 300157 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2976286

New failing tests:
fast/events/input-events-drag-and-drop.html

[Build Bot]

Created attachment 300162 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Comment on attachment 300157 [details]
Patch

Please add a test.

[Wenson Hsieh]

Created attachment 300301 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 300301 [details]
Patch

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

> LayoutTests/fast/events/input-events-insert-newlines-after-mutation.html:4
> +<html>
> +    <head>
> +        <script>

Maybe we can go easy on indentation by not indenting elements.

> LayoutTests/fast/events/input-events-insert-newlines-after-mutation.html:16
> +                        getSelection().removeAllRanges();
> +                        let range = document.createRange();
> +                        range.setStart(editor.firstElementChild, 1);
> +                        range.setEnd(editor.firstElementChild, 1);
> +                        getSelection().addRange(range);

Why not just getSelection().collapse(editor.firstElementChild, 1) ?

[Wenson Hsieh]

(In reply to comment #15)
> Comment on attachment 300301 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300301&action=review
> 
> > LayoutTests/fast/events/input-events-insert-newlines-after-mutation.html:4
> > +<html>
> > +    <head>
> > +        <script>
> 
> Maybe we can go easy on indentation by not indenting elements.

Sounds good -- done!

> 
> > LayoutTests/fast/events/input-events-insert-newlines-after-mutation.html:16
> > +                        getSelection().removeAllRanges();
> > +                        let range = document.createRange();
> > +                        range.setStart(editor.firstElementChild, 1);
> > +                        range.setEnd(editor.firstElementChild, 1);
> > +                        getSelection().addRange(range);
> 
> Why not just getSelection().collapse(editor.firstElementChild, 1) ?

Good idea -- changed to use collapse.

Thanks!

[Wenson Hsieh]

Created attachment 300304 [details]
Patch for landing

[Wenson Hsieh]

Created attachment 300305 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 300305 [details]
Patch for landing

Clearing flags on attachment: 300305

Committed r211471: <http://trac.webkit.org/changeset/211471>