Bug 56027 - Assert that editing does not ignore position's anchorNode if position is an offset in anchor
Summary: Assert that editing does not ignore position's anchorNode if position is an o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 56025
Blocks: 52098 56161
  Show dependency treegraph
 
Reported: 2011-03-09 10:30 PST by Ryosuke Niwa
Modified: 2011-03-17 13:49 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 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.
Comment 1 Ryosuke Niwa 2011-03-09 10:33:09 PST
Created attachment 85192 [details]
work in progress
Comment 2 Eric Seidel (no email) 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?
Comment 3 Ryosuke Niwa 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.
Comment 4 Ryosuke Niwa 2011-03-10 19:16:48 PST
Created attachment 85422 [details]
patch for review
Comment 5 Eric Seidel (no email) 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. :)
Comment 6 Ryosuke Niwa 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.
Comment 7 Ryosuke Niwa 2011-03-10 19:38:10 PST
Created attachment 85427 [details]
Added the forgotten button test
Comment 8 Ryosuke Niwa 2011-03-10 19:42:04 PST
Created attachment 85428 [details]
Added some comments about testability of changes
Comment 9 Eric Seidel (no email) 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?
Comment 10 Ryosuke Niwa 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.
Comment 11 Levi Weintraub 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.
Comment 12 Ryosuke Niwa 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).
Comment 13 Levi Weintraub 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.
Comment 14 Ryosuke Niwa 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.
Comment 15 Darin Adler 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.
Comment 16 Ryosuke Niwa 2011-03-14 11:17:16 PDT
Any chance I can get a review on this patch?
Comment 17 Justin Garcia 2011-03-16 23:09:27 PDT
Comment on attachment 85428 [details]
Added some comments about testability of changes

r=me
Comment 18 Ryosuke Niwa 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.
Comment 19 Ryosuke Niwa 2011-03-17 12:03:07 PDT
Committed r81374: <http://trac.webkit.org/changeset/81374>
Comment 20 Ryosuke Niwa 2011-03-17 12:55:18 PDT
Created attachment 86083 [details]
debug build fix
Comment 21 Ryosuke Niwa 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.
Comment 22 Enrica Casucci 2011-03-17 13:32:22 PDT
Comment on attachment 86083 [details]
debug build fix

The change looks good to me.
Comment 23 Ryosuke Niwa 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.
Comment 24 Ryosuke Niwa 2011-03-17 13:40:51 PDT
Thanks for the review, Adele!
Comment 25 WebKit Review Bot 2011-03-17 13:41:11 PDT
http://trac.webkit.org/changeset/81374 might have broken GTK Linux 64-bit Debug
Comment 26 Ryosuke Niwa 2011-03-17 13:49:28 PDT
Committed r81384: <http://trac.webkit.org/changeset/81384>