charactersAroundPosition can be wrong because it crosses editing boundaries rdar://problem/27933564
Created attachment 287027 [details] Patch
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
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
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?
(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.
Thanks Ryosuke! https://trac.webkit.org/changeset/205044