Bug 87623 - comparePositions in htmlediting should consider nested Shadow DOM.
: comparePositions in htmlediting should consider nested Shadow DOM.
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-27 23:49 PST by
Modified: 2012-05-30 12:13 PST (History)


Attachments
Patch (8.71 KB, patch)
2012-05-28 04:18 PST, Shinya Kawanaka
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.75 KB, patch)
2012-05-29 02:12 PST, Shinya Kawanaka
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (521.70 KB, application/zip)
2012-05-29 06:35 PST, WebKit Review Bot
no flags Details
Patch (8.41 KB, patch)
2012-05-29 21:08 PST, Shinya Kawanaka
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (474.38 KB, application/zip)
2012-05-29 23:23 PST, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-02 (531.62 KB, application/zip)
2012-05-30 00:23 PST, WebKit Review Bot
no flags Details
Patch for landing (8.38 KB, patch)
2012-05-30 02:40 PST, Shinya Kawanaka
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-05-27 23:49:33 PST
It does not consider nested Shadow DOM, so the result value might be wrong when comparing elements between different nested Shadow DOMs.
------- Comment #1 From 2012-05-28 04:18:08 PST -------
Created an attachment (id=144329) [details]
Patch
------- Comment #2 From 2012-05-28 21:52:38 PST -------
(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.

> 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;
}
------- Comment #3 From 2012-05-28 21:57:40 PST -------
(From update of attachment 144329 [details])
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.
------- Comment #4 From 2012-05-29 01:08:16 PST -------
(In reply to comment #2)
> (From update of attachment 144329 [details] [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.
------- Comment #5 From 2012-05-29 02:12:06 PST -------
(From update of attachment 144329 [details])
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.
------- Comment #6 From 2012-05-29 02:12:56 PST -------
Created an attachment (id=144485) [details]
Patch
------- Comment #7 From 2012-05-29 06:35:49 PST -------
(From update of attachment 144485 [details])
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
------- Comment #8 From 2012-05-29 06:35:56 PST -------
Created an attachment (id=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
------- Comment #9 From 2012-05-29 12:02:17 PST -------
(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.

> 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.
------- Comment #10 From 2012-05-29 21:08:07 PST -------
Created an attachment (id=144689) [details]
Patch
------- Comment #11 From 2012-05-29 21:09:11 PST -------
(In reply to comment #9)
> (From update of attachment 144485 [details] [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.
------- Comment #12 From 2012-05-29 21:22:20 PST -------
(From update of attachment 144689 [details])
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?
------- Comment #13 From 2012-05-29 21:23:03 PST -------
Levi, Enrica, & Darin: fyi, this patch also removes legacy position code in comparePositions :)
------- Comment #14 From 2012-05-29 23:23:15 PST -------
(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
------- Comment #15 From 2012-05-29 23:23:25 PST -------
Created an attachment (id=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
------- Comment #16 From 2012-05-29 23:33:08 PST -------
(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
------- Comment #17 From 2012-05-29 23:49:04 PST -------
(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...
------- Comment #18 From 2012-05-29 23:52:29 PST -------
(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.
------- Comment #19 From 2012-05-30 00:23:34 PST -------
(From update of attachment 144689 [details])
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
------- Comment #20 From 2012-05-30 00:23:39 PST -------
Created an attachment (id=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
------- Comment #21 From 2012-05-30 00:35:15 PST -------
(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.
------- Comment #22 From 2012-05-30 00:42:01 PST -------
(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] [details])
> > > > > Attachment 144689 [details] [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.
------- Comment #23 From 2012-05-30 00:43:11 PST -------
(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.
------- Comment #24 From 2012-05-30 02:37:29 PST -------
> 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.
------- Comment #25 From 2012-05-30 02:40:45 PST -------
Created an attachment (id=144770) [details]
Patch for landing
------- Comment #26 From 2012-05-30 09:44:50 PST -------
(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.
------- Comment #27 From 2012-05-30 11:36:18 PST -------
(From update of attachment 144770 [details])
Clearing flags on attachment: 144770

Committed r118945: <http://trac.webkit.org/changeset/118945>
------- Comment #28 From 2012-05-30 11:36:24 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #29 From 2012-05-30 12:13:56 PST -------
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.