Bug 167525 - Regression (Safari 10.1): Pressing Return in a contenteditable no longer inserts a line break under certain conditions
Summary: Regression (Safari 10.1): Pressing Return in a contenteditable no longer inse...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: Safari Technology Preview
Hardware: Macintosh macOS 10.12
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-27 12:53 PST by Sam Stephenson
Modified: 2017-02-01 07:59 PST (History)
7 users (show)

See Also:


Attachments
Minimal test case (1.77 KB, text/html)
2017-01-27 12:53 PST, Sam Stephenson
no flags Details
Patch (1.81 KB, patch)
2017-01-30 16:14 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (985.70 KB, application/zip)
2017-01-30 16:56 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.61 MB, application/zip)
2017-01-30 17:06 PST, Build Bot
no flags Details
Patch (8.18 KB, patch)
2017-01-31 22:16 PST, Wenson Hsieh
rniwa: review+
Details | Formatted Diff | Diff
Patch for landing (11.93 KB, patch)
2017-01-31 23:10 PST, Wenson Hsieh
wenson_hsieh: commit-queue+
Details | Formatted Diff | Diff
Patch for landing (8.57 KB, patch)
2017-01-31 23:10 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Stephenson 2017-01-27 12:53:36 PST
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.
Comment 1 Radar WebKit Bug Importer 2017-01-30 12:58:57 PST
<rdar://problem/30270210>
Comment 2 Wenson Hsieh 2017-01-30 14:29:30 PST
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!
Comment 3 Wenson Hsieh 2017-01-30 14:30:13 PST
(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)
Comment 4 Ryosuke Niwa 2017-01-30 15:40:04 PST
Was this change merged into our branch? If so, we should fix it in the branch as well.
Comment 5 Wenson Hsieh 2017-01-30 15:41:04 PST
Yes, the breaking change was merged into the branch. I'll put out a fix today.
Comment 6 Wenson Hsieh 2017-01-30 16:14:21 PST
Created attachment 300157 [details]
Patch
Comment 7 Ryosuke Niwa 2017-01-30 16:20:06 PST
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.
Comment 8 Wenson Hsieh 2017-01-30 16:27:34 PST
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.
Comment 9 Build Bot 2017-01-30 16:56:10 PST
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
Comment 10 Build Bot 2017-01-30 16:56:14 PST
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
Comment 11 Build Bot 2017-01-30 17:06:33 PST
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
Comment 12 Build Bot 2017-01-30 17:06:37 PST
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
Comment 13 Ryosuke Niwa 2017-01-31 22:10:46 PST
Comment on attachment 300157 [details]
Patch

Please add a test.
Comment 14 Wenson Hsieh 2017-01-31 22:16:17 PST
Created attachment 300301 [details]
Patch
Comment 15 Ryosuke Niwa 2017-01-31 22:28:29 PST
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) ?
Comment 16 Wenson Hsieh 2017-01-31 23:01:49 PST
(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!
Comment 17 Wenson Hsieh 2017-01-31 23:10:09 PST
Created attachment 300304 [details]
Patch for landing
Comment 18 Wenson Hsieh 2017-01-31 23:10:54 PST
Created attachment 300305 [details]
Patch for landing
Comment 19 WebKit Commit Bot 2017-01-31 23:47:56 PST
Comment on attachment 300305 [details]
Patch for landing

Clearing flags on attachment: 300305

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