Bug 87623 - comparePositions in htmlediting should consider nested Shadow DOM.
: comparePositions in htmlediting should consider nested Shadow DOM.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Shinya Kawanaka
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-27 23:49 PDT by Shinya Kawanaka
Modified: 2012-05-30 12:13 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-05-27 23:49:33 PDT
It does not consider nested Shadow DOM, so the result value might be wrong when comparing elements between different nested Shadow DOMs.
Comment 1 Shinya Kawanaka 2012-05-28 04:18:08 PDT
Created attachment 144329 [details]
Patch
Comment 2 Ryosuke Niwa 2012-05-28 21:52:38 PDT
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;
}
Comment 3 Ryosuke Niwa 2012-05-28 21:57:40 PDT
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.
Comment 4 Shinya Kawanaka 2012-05-29 01:08:16 PDT
(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.
Comment 5 Shinya Kawanaka 2012-05-29 02:12:06 PDT
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.
Comment 6 Shinya Kawanaka 2012-05-29 02:12:56 PDT
Created attachment 144485 [details]
Patch
Comment 7 WebKit Review Bot 2012-05-29 06:35:49 PDT
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
Comment 8 WebKit Review Bot 2012-05-29 06:35:56 PDT
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
Comment 9 Ryosuke Niwa 2012-05-29 12:02:17 PDT
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.
Comment 10 Shinya Kawanaka 2012-05-29 21:08:07 PDT
Created attachment 144689 [details]
Patch
Comment 11 Shinya Kawanaka 2012-05-29 21:09:11 PDT
(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.
Comment 12 Ryosuke Niwa 2012-05-29 21:22:20 PDT
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?
Comment 13 Ryosuke Niwa 2012-05-29 21:23:03 PDT
Levi, Enrica, & Darin: fyi, this patch also removes legacy position code in comparePositions :)
Comment 14 WebKit Review Bot 2012-05-29 23:23:15 PDT
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
Comment 15 WebKit Review Bot 2012-05-29 23:23:25 PDT
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
Comment 16 Shinya Kawanaka 2012-05-29 23:33:08 PDT
(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
Comment 17 Shinya Kawanaka 2012-05-29 23:49:04 PDT
(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...
Comment 18 Ryosuke Niwa 2012-05-29 23:52:29 PDT
(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.
Comment 19 WebKit Review Bot 2012-05-30 00:23:34 PDT
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
Comment 20 WebKit Review Bot 2012-05-30 00:23:39 PDT
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
Comment 21 Shinya Kawanaka 2012-05-30 00:35:15 PDT
(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.
Comment 22 Shinya Kawanaka 2012-05-30 00:42:01 PDT
(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.
Comment 23 Ryosuke Niwa 2012-05-30 00:43:11 PDT
(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 Shinya Kawanaka 2012-05-30 02:37:29 PDT
> 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 Shinya Kawanaka 2012-05-30 02:40:45 PDT
Created attachment 144770 [details]
Patch for landing
Comment 26 Ryosuke Niwa 2012-05-30 09:44:50 PDT
(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 WebKit Review Bot 2012-05-30 11:36:18 PDT
Comment on attachment 144770 [details]
Patch for landing

Clearing flags on attachment: 144770

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