charactersAroundPosition can be wrong because it crosses editing boundaries, affects Google hangouts

• Component: HTML Editing
• Status: RESOLVED FIXED

[Beth Dakin]

charactersAroundPosition can be wrong because it crosses editing boundaries

rdar://problem/27933564

[Beth Dakin]

Created attachment 287027 [details]
Patch

[Build Bot]

Comment on attachment 287027 [details]
Patch

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

New failing tests:
fast/dom/focus-contenteditable.html

[Build Bot]

Created attachment 287044 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

Comment on attachment 287027 [details]
Patch

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

It looks like fast/dom/focus-contenteditable.html is failing on WebKit2.  Please check that before landing it.

> Source/WebCore/ChangeLog:5
> +        -and corresponding-

I don't think we need this line.

> Source/WebCore/ChangeLog:14
> +        VisiblePosition::previous().
> +        * editing/VisibleUnits.cpp:

We need a blank line here.

> LayoutTests/editing/mac/spelling/accept-candidate-without-crossing-editing-boundary.html:3
> +<script src=../../editing.js language="javascript" type="text/javascript"></script>

We don't need language & type attributes.

> LayoutTests/editing/mac/spelling/accept-candidate-without-crossing-editing-boundary.html:10
> +    var s = window.getSelection();

It looks like this variable is never used.

> LayoutTests/editing/mac/spelling/accept-candidate-without-crossing-editing-boundary.html:32
> +<div style="visibility:hidden;">
> +    <br>
> +</div>
> +
> +<div style="width: 1px; height: 1px;"></div>

Do we really need these stray elements?

[Beth Dakin]

(In reply to comment #4)
> Comment on attachment 287027 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287027&action=review
> 
> It looks like fast/dom/focus-contenteditable.html is failing on WebKit2. 
> Please check that before landing it.
> 

I investigated, and this test is going back to its pre-https://trac.webkit.org/changeset/195078 state. That change caused this test to have a different layout because it caused more layouts to happen. Now that we don’t allow the call to charactersAroundPosition() to cross editing boundaries, those layouts don’t happen, and we have the old behavior back.

> > Source/WebCore/ChangeLog:5
> > +        -and corresponding-
> 
> I don't think we need this line.
> 
> > Source/WebCore/ChangeLog:14
> > +        VisiblePosition::previous().
> > +        * editing/VisibleUnits.cpp:
> 
> We need a blank line here.
> 

Added.

> > LayoutTests/editing/mac/spelling/accept-candidate-without-crossing-editing-boundary.html:3
> > +<script src=../../editing.js language="javascript" type="text/javascript"></script>
> 
> We don't need language & type attributes.
> 

Removed.

> > LayoutTests/editing/mac/spelling/accept-candidate-without-crossing-editing-boundary.html:10
> > +    var s = window.getSelection();
> 
> It looks like this variable is never used.
> 

Removed.

> > LayoutTests/editing/mac/spelling/accept-candidate-without-crossing-editing-boundary.html:32
> > +<div style="visibility:hidden;">
> > +    <br>
> > +</div>
> > +
> > +<div style="width: 1px; height: 1px;"></div>
> 
> Do we really need these stray elements?

We do!! These elements are key to reproduce the bug. When charactersAroundPosition can cross editing boundaries, this DOM structure tricks it into thinking that the <br> inside the hidden div is the character before the caret. And when we think a newline is the character before the caret, we don't replace the typed text with the candidate.

[Beth Dakin]

Thanks Ryosuke! https://trac.webkit.org/changeset/205044