Bug 56027

Summary:

Assert that editing does not ignore position's anchorNode if position is an offset in anchor

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Enhancement

CC:

abarth, adele, darin, enrica, eric, justin.garcia, leviw, mrobinson, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

56025

Bug Blocks:

52098, 56161

Attachments:

Description	Flags
work in progress	none
patch for review	none
Added the forgotten button test	none
Added some comments about testability of changes	none
debug build fix	adele: review+

Ryosuke Niwa

Reported 2011-03-09 10:30:56 PST

Since it doesn't make sense to have a offset position inside br, hr, and so forth, we should assert that editingIgnoresContent returns false on the anchor node when we're instantiating position with the type PositionIsOffsetInAnchor or moving position to a node-offset pair.

Attachments
work in progress (3.29 KB, patch) 2011-03-09 10:33 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
patch for review (6.04 KB, patch) 2011-03-10 19:16 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Added the forgotten button test (8.42 KB, patch) 2011-03-10 19:38 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Added some comments about testability of changes (8.77 KB, patch) 2011-03-10 19:42 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
debug build fix (3.73 KB, patch) 2011-03-17 12:55 PDT, Ryosuke Niwa	adele: review+	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2011-03-09 10:33:09 PST

Created attachment 85192 [details] work in progress

Eric Seidel (no email)

Comment 2 2011-03-09 10:45:50 PST

Comment on attachment 85192 [details] work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=85192&action=review > Source/WebCore/dom/Position.cpp:97 > + ASSERT(!m_anchorNode || !editingIgnoresContent(m_anchorNode.get())); When is this ever called with a null anchorNode? > Source/WebCore/dom/Position.cpp:103 > + ASSERT(!editingIgnoresContent(m_anchorNode.get())); Why validate m_anchorNode here as well? Do you mean "node" isntead?

Ryosuke Niwa

Comment 3 2011-03-09 10:50:25 PST

Comment on attachment 85192 [details] work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=85192&action=review >> Source/WebCore/dom/Position.cpp:97 >> + ASSERT(!m_anchorNode || !editingIgnoresContent(m_anchorNode.get())); > > When is this ever called with a null anchorNode? Numerous places. I'm not sure if I can get rid of m_anchorNode in the final patch because it requires fixing lots of code. >> Source/WebCore/dom/Position.cpp:103 >> + ASSERT(!editingIgnoresContent(m_anchorNode.get())); > > Why validate m_anchorNode here as well? Do you mean "node" isntead? Oops, that's what I meant.

Ryosuke Niwa

Comment 4 2011-03-10 19:16:48 PST

Created attachment 85422 [details] patch for review

Eric Seidel (no email)

Comment 5 2011-03-10 19:29:27 PST

Comment on attachment 85422 [details] patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=85422&action=review Behavior changes need tests. > Source/WebCore/dom/PositionIterator.cpp:43 > + if (editingIgnoresContent(m_nodeAfterPositionInAnchor->parentNode())) > + return positionBeforeNode(m_anchorNode); How does this change behavior? How do we test this? > Source/WebCore/editing/ApplyStyleCommand.cpp:1699 > + positionForStyleComparison = firstPositionInOrBeforeNode(startNode.get()); Same question. > Source/WebCore/editing/htmlediting.cpp:-79 > - !node->hasTagName(buttonTag) && Same question. :) Buttons are editing boundaries, aren't they? And aren't their editable kids shadow DOMs? > Source/WebCore/editing/visible_units.cpp:381 > + VisiblePosition visPos = startNode->isTextNode() ? VisiblePosition(Position(startNode, static_cast<InlineTextBox *>(startBox)->start(), Position::PositionIsOffsetInAnchor), DOWNSTREAM) Same qeustion. :)

Ryosuke Niwa

Comment 6 2011-03-10 19:36:21 PST

Comment on attachment 85422 [details] patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=85422&action=review >> Source/WebCore/dom/PositionIterator.cpp:43 >> + return positionBeforeNode(m_anchorNode); > > How does this change behavior? How do we test this? It doesn't. It only helps us moving towards getting rid of weird positions like [textarea, 0] >> Source/WebCore/editing/ApplyStyleCommand.cpp:1699 >> + positionForStyleComparison = firstPositionInOrBeforeNode(startNode.get()); > > Same question. Ditto. >> Source/WebCore/editing/htmlediting.cpp:-79 >> - !node->hasTagName(buttonTag) && > > Same question. :) Buttons are editing boundaries, aren't they? And aren't their editable kids shadow DOMs? Right. I forgot to do "svn add". I'm uploading a new patch with a test. >> Source/WebCore/editing/visible_units.cpp:381 >> + VisiblePosition visPos = startNode->isTextNode() ? VisiblePosition(Position(startNode, static_cast<InlineTextBox *>(startBox)->start(), Position::PositionIsOffsetInAnchor), DOWNSTREAM) > > Same qeustion. :) Same. We don't really change the apparent behavior. It's just that we used to have [br, 0] and now we do before br.

Ryosuke Niwa

Comment 7 2011-03-10 19:38:10 PST

Created attachment 85427 [details] Added the forgotten button test

Ryosuke Niwa

Comment 8 2011-03-10 19:42:04 PST

Created attachment 85428 [details] Added some comments about testability of changes

Eric Seidel (no email)

Comment 9 2011-03-10 19:57:40 PST

Comment on attachment 85428 [details] Added some comments about testability of changes Does FF match this <button> behavior?

Ryosuke Niwa

Comment 10 2011-03-10 20:02:36 PST

(In reply to comment #9) > (From update of attachment 85428 [details]) > Does FF match this <button> behavior? FF and IE won't let you edit it. But the problem is that we already let user put caret inside the button and edit text. It's just that it doesn't work properly because of the bug I'm fixing here. Not to let user edit button's content, we must reimplement button element using shadow DOM.

Levi Weintraub

Comment 11 2011-03-11 11:17:33 PST

The current state in which you can get a caret inside a button but not, for example, hold shift to extend selection is quite broken. This patch makes this editing behave more consistently, so I believe it's worth doing.

Ryosuke Niwa

Comment 12 2011-03-11 11:18:56 PST

(In reply to comment #11) > The current state in which you can get a caret inside a button but not, for example, hold shift to extend selection is quite broken. This patch makes this editing behave more consistently, so I believe it's worth doing. Right, that was my justification as well. But given that no other browser lets you put caret inside the button, the correct fix seems to be to reimplement button to use the shadow DOM (the bug 54173).

Levi Weintraub

Comment 13 2011-03-11 11:21:35 PST

(In reply to comment #12) > Right, that was my justification as well. But given that no other browser lets you put caret inside the button, the correct fix seems to be to reimplement button to use the shadow DOM (the bug 54173). Oh awesome -- that's even better :) I was surprised to see it wasn't already using shadow DOM.

Ryosuke Niwa

Comment 14 2011-03-11 11:37:23 PST

(In reply to comment #13) > (In reply to comment #12) > > Right, that was my justification as well. But given that no other browser lets you put caret inside the button, the correct fix seems to be to reimplement button to use the shadow DOM (the bug 54173). > > Oh awesome -- that's even better :) I was surprised to see it wasn't already using shadow DOM. Ok, so we have to decide wether we fix the bug 56211 first or this one first. Given that fixing the bug 56211 requires adding new binding model into WebCore and we need to fix PositionIterator not to enter DOM inside button element before being able to fix this bug, I'm inclined to land this patch for now. This patch at least makes our editing behavior consistent inside/outside button element.

Darin Adler

Comment 15 2011-03-12 19:15:54 PST

While shadow DOM would be handy for editing, I don’t think it’s right for <button>. We won’t be using shadow DOM for cases where normal DOM manipulation is expected and allowed to see DOM elements. The fix for editing and <button> is presumably some special treatment in editing code, not shadow DOM.

Ryosuke Niwa

Comment 16 2011-03-14 11:17:16 PDT

Any chance I can get a review on this patch?

Justin Garcia

Comment 17 2011-03-16 23:09:27 PDT

Comment on attachment 85428 [details] Added some comments about testability of changes r=me

Ryosuke Niwa

Comment 18 2011-03-16 23:10:03 PDT

Justin pointed out that there are code changes in my patch that prevents new assertions to hit. And those are clearly testable by some tests although I claim that "This change only affects positions WebCore uses internally and therefore cannot be tested directly". I should have clarified that those changes are tested by the existing layout tests.

Ryosuke Niwa

Comment 19 2011-03-17 12:03:07 PDT

Committed r81374: <http://trac.webkit.org/changeset/81374>

Ryosuke Niwa

Comment 20 2011-03-17 12:55:18 PDT

Created attachment 86083 [details] debug build fix

Ryosuke Niwa

Comment 21 2011-03-17 12:57:12 PDT

Apparently, code changed since I uploaded my patch and caused assertions to hit in some places. This patch removes code that instantiates bad positions and suppresses those assertions.

Enrica Casucci

Comment 22 2011-03-17 13:32:22 PDT

Comment on attachment 86083 [details] debug build fix The change looks good to me.

Ryosuke Niwa

Comment 23 2011-03-17 13:35:14 PDT

Thanks for the informal review, Enrica. Could someone r+/r- it? (In reply to comment #22) > (From update of attachment 86083 [details]) > The change looks good to me.

Ryosuke Niwa

Comment 24 2011-03-17 13:40:51 PDT

Thanks for the review, Adele!

WebKit Review Bot

Comment 25 2011-03-17 13:41:11 PDT

http://trac.webkit.org/changeset/81374 might have broken GTK Linux 64-bit Debug

Ryosuke Niwa

Comment 26 2011-03-17 13:49:28 PDT

Committed r81384: <http://trac.webkit.org/changeset/81384>

Note You need to log in before you can comment on or make changes to this bug.