Bug 129200 – isEditablePosition and related functions shouldn't move position out of table

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Right now, isEditablePosition and related functions check whether we're at the beginning or at the end of an element with RenderTableElement
to work around the fact we use (table, 0) and (table, <number of child nodes>) to mean before and after the table.

This prevents us from fixing the bug 129034 because this information is only available via renderer.

[Ryosuke Niwa]

Created attachment 224959 [details]
Work in progress

This IS the patch but editing/execCommand/5481523.html fails until the bug 129201 is fixed.

[Ryosuke Niwa]

Created attachment 247276 [details]
WIP

[Ryosuke Niwa]

Created attachment 247288 [details]
Fixes the bug

[Darin Adler]

Comment on attachment 247288 [details]
Fixes the bug

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

> Source/WebCore/dom/Position.cpp:339
> +    if (isRenderedTable(node) || editingIgnoresContent(node))

Seems like we might want a helper function for “isRenderedTable || editingIgnoresContent” since we are using this four different times. If we come up with the right name for it, it might even make the code easier to read.

[Build Bot]

Comment on attachment 247288 [details]
Fixes the bug

Attachment 247288 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5893639444103168

New failing tests:
fast/forms/targeted-frame-submission.html

[Build Bot]

Created attachment 247293 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

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

[Build Bot]

Comment on attachment 247288 [details]
Fixes the bug

Attachment 247288 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6108830727405568

New failing tests:
fast/loader/text-document-wrapping.html

[Build Bot]

Created attachment 247295 [details]
Archive of layout-test-results from ews100 for mac-mavericks

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

[Ryosuke Niwa]

(In reply to comment #4)
> Comment on attachment 247288 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247288&action=review
> 
> > Source/WebCore/dom/Position.cpp:339
> > +    if (isRenderedTable(node) || editingIgnoresContent(node))
> 
> Seems like we might want a helper function for “isRenderedTable ||
> editingIgnoresContent” since we are using this four different times. If we
> come up with the right name for it, it might even make the code easier to
> read.

nodeHasCandidateBeforeOrAfter?  isCandidateBeforeOrAfterNode?

[Ryosuke Niwa]

How about isPositionBeforeOrAfterNodeCandidate or positionBeforeOrAfterNodeIsCandidate?

[Darin Adler]

I think either of those is OK. They are a bit long, but still probably good.

[Ryosuke Niwa]

Created attachment 247492 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 247492 [details]
Patch for landing

Clearing flags on attachment: 247492

Committed r180726: <http://trac.webkit.org/changeset/180726>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

Note: This rebaseline was also needed on Windows. It looks like the EFL and GTK ports are now failing this test for the same reason.

I checked in a Windows fix under r180755 <https://trac.webkit.org/changeset/180755>.

[Ryosuke Niwa]

(In reply to comment #15)
> Note: This rebaseline was also needed on Windows. It looks like the EFL and
> GTK ports are now failing this test for the same reason.
> 
> I checked in a Windows fix under r180755
> <https://trac.webkit.org/changeset/180755>.

Oops, thanks for the rebaselines. Done that for other platforms in http://trac.webkit.org/changeset/180778.

[Chris Dumez]

This broke some basic editing behaviors. For e.g. if you do the following:
1. Click the "Reply" link on a bugzilla comment
2. Click below the comment quote so that the caret is at the bottom
3. Press Backspace key

-> it clears the whole textarea.

I bisected it so I am confident it is this change.

[Darin Adler]

Seems like next step would be to add some new test cases covering some of the editing behaviors it broke.

[Ryosuke Niwa]

Let's not use this bug to fix the regression. Filed the bug 129200 to track the regression.