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

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 85192 [details]
work in progress

[Eric Seidel (no email)]

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 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]

Created attachment 85422 [details]
patch for review

[Eric Seidel (no email)]

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 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]

Created attachment 85427 [details]
Added the forgotten button test

[Ryosuke Niwa]

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

[Eric Seidel (no email)]

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

Does FF match this <button> behavior?

[Ryosuke Niwa]

(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]

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]

(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]

(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]

(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]

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]

Any chance I can get a review on this patch?

[Justin Garcia]

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

r=me

[Ryosuke Niwa]

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]

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

[Ryosuke Niwa]

Created attachment 86083 [details]
debug build fix

[Ryosuke Niwa]

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 on attachment 86083 [details]
debug build fix

The change looks good to me.

[Ryosuke Niwa]

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]

Thanks for the review, Adele!

[WebKit Review Bot]

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

[Ryosuke Niwa]

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