Bug 129200

Summary: isEditablePosition and related functions shouldn't move position out of table
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bfulgham, buildbot, cdumez, commit-queue, darin, enrica, kling, koivisto, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 129201    
Bug Blocks: 129034, 142646    
Attachments:
Description Flags
Work in progress
none
WIP
none
Fixes the bug
none
Archive of layout-test-results from ews107 for mac-mavericks-wk2
none
Archive of layout-test-results from ews100 for mac-mavericks
none
Patch for landing none

Description Ryosuke Niwa 2014-02-22 03:34:57 PST
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.
Comment 1 Ryosuke Niwa 2014-02-22 04:14:00 PST
Created attachment 224959 [details]
Work in progress

This IS the patch but editing/execCommand/5481523.html fails until the bug 129201 is fixed.
Comment 2 Ryosuke Niwa 2015-02-24 16:05:27 PST
Created attachment 247276 [details]
WIP
Comment 3 Ryosuke Niwa 2015-02-24 17:59:33 PST
Created attachment 247288 [details]
Fixes the bug
Comment 4 Darin Adler 2015-02-24 18:53:09 PST
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.
Comment 5 Build Bot 2015-02-24 19:40:43 PST
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
Comment 6 Build Bot 2015-02-24 19:40:46 PST
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
Comment 7 Build Bot 2015-02-24 19:43:32 PST
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
Comment 8 Build Bot 2015-02-24 19:43:36 PST
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
Comment 9 Ryosuke Niwa 2015-02-24 19:53:25 PST
(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?
Comment 10 Ryosuke Niwa 2015-02-25 18:49:04 PST
How about isPositionBeforeOrAfterNodeCandidate or positionBeforeOrAfterNodeIsCandidate?
Comment 11 Darin Adler 2015-02-25 19:16:47 PST
I think either of those is OK. They are a bit long, but still probably good.
Comment 12 Ryosuke Niwa 2015-02-26 21:02:29 PST
Created attachment 247492 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2015-02-26 21:51:51 PST
Comment on attachment 247492 [details]
Patch for landing

Clearing flags on attachment: 247492

Committed r180726: <http://trac.webkit.org/changeset/180726>
Comment 14 WebKit Commit Bot 2015-02-26 21:51:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Brent Fulgham 2015-02-27 09:03:56 PST
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>.
Comment 16 Ryosuke Niwa 2015-02-27 13:40:36 PST
(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.
Comment 17 Chris Dumez 2015-03-11 19:52:17 PDT
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.
Comment 18 Darin Adler 2015-03-12 08:27:04 PDT
Seems like next step would be to add some new test cases covering some of the editing behaviors it broke.
Comment 19 Ryosuke Niwa 2015-03-12 15:57:10 PDT
Let's not use this bug to fix the regression. Filed the bug 129200 to track the regression.