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

Ryosuke Niwa
Reported 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.
Attachments
Work in progress (10.10 KB, patch)
2014-02-22 04:14 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
WIP (12.46 KB, patch)
2015-02-24 16:05 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Fixes the bug (12.99 KB, patch)
2015-02-24 17:59 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (1.39 MB, application/zip)
2015-02-24 19:40 PST, Build Bot 		no flags
Details
Archive of layout-test-results from ews100 for mac-mavericks (1.36 MB, application/zip)
2015-02-24 19:43 PST, Build Bot 		no flags
Details
Patch for landing (13.52 KB, patch)
2015-02-26 21:02 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 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.
Ryosuke Niwa
Comment 2 2015-02-24 16:05:27 PST
Created attachment 247276 [details] WIP
Ryosuke Niwa
Comment 3 2015-02-24 17:59:33 PST
Created attachment 247288 [details] Fixes the bug
Darin Adler
Comment 4 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.
Build Bot
Comment 5 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
Build Bot
Comment 6 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
Build Bot
Comment 7 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
Build Bot
Comment 8 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
Ryosuke Niwa
Comment 9 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?
Ryosuke Niwa
Comment 10 2015-02-25 18:49:04 PST
How about isPositionBeforeOrAfterNodeCandidate or positionBeforeOrAfterNodeIsCandidate?
Darin Adler
Comment 11 2015-02-25 19:16:47 PST
I think either of those is OK. They are a bit long, but still probably good.
Ryosuke Niwa
Comment 12 2015-02-26 21:02:29 PST
Created attachment 247492 [details] Patch for landing
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2015-02-26 21:51:56 PST
All reviewed patches have been landed. Closing bug.
Brent Fulgham
Comment 15 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>.
Ryosuke Niwa
Comment 16 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.
Chris Dumez
Comment 17 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.
Darin Adler
Comment 18 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.
Ryosuke Niwa
Comment 19 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.
Note You need to log in before you can comment on or make changes to this bug.