Bug 87623 – comparePositions in htmlediting should consider nested Shadow DOM.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Shinya Kawanaka]

It does not consider nested Shadow DOM, so the result value might be wrong when comparing elements between different nested Shadow DOMs.

[Shinya Kawanaka]

Created attachment 144329 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 144329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144329&action=review

Don't we also need to fix Range::compareBoundaryPoints? If that's the case, we should share more code between comparePositions and compareBoundaryPoints first before making this change.

> Source/WebCore/dom/TreeScope.cpp:258
> +    Vector<TreeScope*> treeScopesA;
> +    listTreeScopes(nodeA, treeScopesA);
> +
> +    Vector<TreeScope*> treeScopesB;
> +    listTreeScopes(nodeB, treeScopesB);

We should probably give some inline buffers (around 4-5) so that we don't have to heap-allocate in common cases.

> Source/WebCore/dom/TreeScope.cpp:268
> +    Vector<TreeScope*>::reverse_iterator itA =  treeScopesA.rbegin();
> +    Vector<TreeScope*>::reverse_iterator itB =  treeScopesB.rbegin();
> +
> +    for (; itA != treeScopesA.rend() && itB != treeScopesB.rend() && *itA == *itB; ++itA, ++itB) { }
> +
> +    if (itA == treeScopesA.rbegin())
> +        return 0;
> +
> +    return *--itA;

We prefer using indices over iterators for vectors. See the style guide.

> Source/WebCore/editing/htmlediting.cpp:109
> +    int offsetB;
> +    Node* descendentB;
> +    if (nodeB->treeScope() == commonScope) {
> +        descendentB = 0;
> +        offsetB = b.deprecatedEditingOffset();
> +    } else {
> +        do {
> +            descendentB = nodeB;
> +            nodeB = nodeB->shadowAncestorNode();
> +        } while (nodeB->treeScope() != commonScope);
> +        offsetB = 0;
> +    }

I think it's better to combine two cases and do:
Node* descendentB = 0;
int offsetB = b.deprecatedEditingOffset();
while (nodeB->treeScope() != commonScope) {
    descendentB = nodeB;
    nodeB = nodeB->shadowAncestorNode();
    offsetB = 0;
}

[Ryosuke Niwa]

Comment on attachment 144329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144329&action=review

> LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:20
> +// Returns div in nested Shadow DOM.

This comment repeats what the code does. Please remove.

> LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:27
> +    var darkRoot = new WebKitShadowRoot(div);
> +    var darkDiv = document.createElement('div');

Why don't we just call this nestedShadowRoot?

> LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:38
> +var div1 = results1[2];
> +var div2 = results2[2];

It would read better if createNestedShadowDOM just returned the nested shadow root instead of returning 3-tuple
since you can get other nodes by calling methods.

[Shinya Kawanaka]

(In reply to comment #2)
> (From update of attachment 144329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144329&action=review
> 
> Don't we also need to fix Range::compareBoundaryPoints? If that's the case, we should share more code between comparePositions and compareBoundaryPoints first before making this change.
> 

Actually it does not consider Shadow DOM at all. It would be OK for now.

I'll update the patch soon.

[Shinya Kawanaka]

Comment on attachment 144329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144329&action=review

>> Source/WebCore/dom/TreeScope.cpp:258
>> +    listTreeScopes(nodeB, treeScopesB);
> 
> We should probably give some inline buffers (around 4-5) so that we don't have to heap-allocate in common cases.

Done.

>> Source/WebCore/dom/TreeScope.cpp:268
>> +    return *--itA;
> 
> We prefer using indices over iterators for vectors. See the style guide.

Done.

>> Source/WebCore/editing/htmlediting.cpp:109
>> +    }
> 
> I think it's better to combine two cases and do:
> Node* descendentB = 0;
> int offsetB = b.deprecatedEditingOffset();
> while (nodeB->treeScope() != commonScope) {
>     descendentB = nodeB;
>     nodeB = nodeB->shadowAncestorNode();
>     offsetB = 0;
> }

Done

>> LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:20
>> +// Returns div in nested Shadow DOM.
> 
> This comment repeats what the code does. Please remove.

Done

>> LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:27
>> +    var darkDiv = document.createElement('div');
> 
> Why don't we just call this nestedShadowRoot?

Done

>> LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:38
>> +var div2 = results2[2];
> 
> It would read better if createNestedShadowDOM just returned the nested shadow root instead of returning 3-tuple
> since you can get other nodes by calling methods.

Done.

[Shinya Kawanaka]

Created attachment 144485 [details]
Patch

[WebKit Review Bot]

Comment on attachment 144485 [details]
Patch

Attachment 144485 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12848495

New failing tests:
editing/pasteboard/drop-text-events.html
editing/pasteboard/drop-inputtext-acquires-style.html
fast/events/5056619.html
editing/selection/select-across-readonly-input-2.html

[WebKit Review Bot]

Created attachment 144544 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Ryosuke Niwa]

Comment on attachment 144485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144485&action=review

Please make sure your patch passes tests.

> Source/WebCore/editing/htmlediting.cpp:100
> +    TreeScope* commonScope = commonTreeScope(a.deprecatedNode(), b.deprecatedNode());
> +
> +    ASSERT(commonScope);
> +    if (!commonScope)
> +        return 0;
> +
>      Node* nodeA = a.deprecatedNode();
>      ASSERT(nodeA);
> +    int offsetA = a.deprecatedEditingOffset();
> +    bool hasDescendentA = adjustPositionToTreeScope(commonScope, nodeA, offsetA);
> +
>      Node* nodeB = b.deprecatedNode();
>      ASSERT(nodeB);
> -    int offsetA = a.deprecatedEditingOffset();
>      int offsetB = b.deprecatedEditingOffset();

I don't know how hard it is to start using containerNode() and computeOffsetInContainerNode() here
but we should move toward that eventually.

[Shinya Kawanaka]

Created attachment 144689 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #9)
> (From update of attachment 144485 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144485&action=review
> 
> Please make sure your patch passes tests.

Ah... sorry. the newly implemented algorithm was wrong.

> I don't know how hard it is to start using containerNode() and computeOffsetInContainerNode() here
> but we should move toward that eventually.

Done.

[Ryosuke Niwa]

Comment on attachment 144689 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144689&action=review

This is a great improvement if it passes all the tests.

> Source/WebCore/dom/TreeScope.cpp:268
> +    if (treeScopesA[indexA] == treeScopesB[indexB])
> +        return treeScopesA[indexA];
> +
> +    return 0;

Maybe use ternary operator here?

[Ryosuke Niwa]

Levi, Enrica, & Darin: fyi, this patch also removes legacy position code in comparePositions :)

[WebKit Review Bot]

Comment on attachment 144689 [details]
Patch

Attachment 144689 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12838870

New failing tests:
editing/selection/document-mutation.html
editing/deleting/25322-2.html

[WebKit Review Bot]

Created attachment 144715 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Shinya Kawanaka]

(In reply to comment #14)
> (From update of attachment 144689 [details])
> Attachment 144689 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12838870
> 
> New failing tests:
> editing/selection/document-mutation.html
> editing/deleting/25322-2.html

hmmmmmmmmmmmmmmmm

[Shinya Kawanaka]

(In reply to comment #16)
> (In reply to comment #14)
> > (From update of attachment 144689 [details] [details])
> > Attachment 144689 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/12838870
> > 
> > New failing tests:
> > editing/selection/document-mutation.html
> > editing/deleting/25322-2.html
> 
> hmmmmmmmmmmmmmmmm

When using deprecated position, those tests pass...

[Ryosuke Niwa]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #14)
> > > (From update of attachment 144689 [details] [details] [details])
> > > Attachment 144689 [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > Output: http://queues.webkit.org/results/12838870
> > > 
> > > New failing tests:
> > > editing/selection/document-mutation.html
> > > editing/deleting/25322-2.html
> > 
> > hmmmmmmmmmmmmmmmm
> 
> When using deprecated position, those tests pass...


Right. That's why I said I don't know how hard it is.

[WebKit Review Bot]

Comment on attachment 144689 [details]
Patch

Attachment 144689 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12844779

New failing tests:
editing/selection/document-mutation.html
editing/deleting/25322-2.html

[WebKit Review Bot]

Created attachment 144733 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Shinya Kawanaka]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #14)
> > > > (From update of attachment 144689 [details] [details] [details] [details])
> > > > Attachment 144689 [details] [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > > Output: http://queues.webkit.org/results/12838870
> > > > 
> > > > New failing tests:
> > > > editing/selection/document-mutation.html
> > > > editing/deleting/25322-2.html
> > > 
> > > hmmmmmmmmmmmmmmmm
> > 
> > When using deprecated position, those tests pass...
> 
> 
> Right. That's why I said I don't know how hard it is.

OK. I understand the reason. When calling comparePositions in case replacing text, the containerNode() might not have the replaced text yet. Maybe we should have an alternative version of computeOffsetInContainerNode() which does not check max length. Let me try.

[Shinya Kawanaka]

(In reply to comment #21)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > > (In reply to comment #14)
> > > > > (From update of attachment 144689 [details] [details] [details] [details] [details])
> > > > > Attachment 144689 [details] [details] [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > > > Output: http://queues.webkit.org/results/12838870
> > > > > 
> > > > > New failing tests:
> > > > > editing/selection/document-mutation.html
> > > > > editing/deleting/25322-2.html
> > > > 
> > > > hmmmmmmmmmmmmmmmm
> > > 
> > > When using deprecated position, those tests pass...
> > 
> > 
> > Right. That's why I said I don't know how hard it is.
> 
> OK. I understand the reason. When calling comparePositions in case replacing text, the containerNode() might not have the replaced text yet. Maybe we should have an alternative version of computeOffsetInContainerNode() which does not check max length. Let me try.

Hmm... it only holds for document-mutation.html case... 25322-2.html is caused by another reason.

[Ryosuke Niwa]

(In reply to comment #21)
> OK. I understand the reason. When calling comparePositions in case replacing text, the containerNode() might not have the replaced text yet. Maybe we should have an alternative version of computeOffsetInContainerNode() which does not check max length. Let me try.

I see. It appears to me that we need to change the order in which text is replaced and selection is updated. See https://bugs.webkit.org/show_bug.cgi?id=66120.

[Shinya Kawanaka]

> Hmm... it only holds for document-mutation.html case... 25322-2.html is caused by another reason.

25322-2.html has the following case:

<img><br>

and m_start = (br, 0), and m_end = (img, 1).

m_start and m_end should be the same, however, it does not hold in the deprecated way...
And it is required to make DeleteSelectionCommand work for now.

So I gave up removing deprecatedNode() and deprecatedEditingOffset() for now. We have to fix DeleteSelectionCommand to remove them.

[Shinya Kawanaka]

Created attachment 144770 [details]
Patch for landing

[Ryosuke Niwa]

(In reply to comment #24)
> > Hmm... it only holds for document-mutation.html case... 25322-2.html is caused by another reason.
> 
> 25322-2.html has the following case:
> 
> <img><br>
> 
> and m_start = (br, 0), and m_end = (img, 1).
> 
> m_start and m_end should be the same, however, it does not hold in the deprecated way...
> And it is required to make DeleteSelectionCommand work for now.

That's the exact problem I encountered the last time I tired it.
 
> So I gave up removing deprecatedNode() and deprecatedEditingOffset() for now. We have to fix DeleteSelectionCommand to remove them.

Okay. That's okay since this is a very difficult change to make.

[WebKit Review Bot]

Comment on attachment 144770 [details]
Patch for landing

Clearing flags on attachment: 144770

Committed r118945: <http://trac.webkit.org/changeset/118945>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

The word descendant was misspelled as descendent in this patch. The one with the “e” is a different word, an adjective rather than a noun.