Summary: | isEditablePosition and related functions shouldn't move position out of table | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||
Component: | HTML Editing | Assignee: | 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
Ryosuke Niwa
2014-02-22 03:34:57 PST
Created attachment 224959 [details] Work in progress This IS the patch but editing/execCommand/5481523.html fails until the bug 129201 is fixed. Created attachment 247276 [details]
WIP
Created attachment 247288 [details]
Fixes the bug
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 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 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 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 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
(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? How about isPositionBeforeOrAfterNodeCandidate or positionBeforeOrAfterNodeIsCandidate? I think either of those is OK. They are a bit long, but still probably good. Created attachment 247492 [details]
Patch for landing
Comment on attachment 247492 [details] Patch for landing Clearing flags on attachment: 247492 Committed r180726: <http://trac.webkit.org/changeset/180726> All reviewed patches have been landed. Closing bug. 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>. (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. 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. Seems like next step would be to add some new test cases covering some of the editing behaviors it broke. Let's not use this bug to fix the regression. Filed the bug 129200 to track the regression. |