Bug 82698

Summary: ShadowRoot.selection should return the selection whose range is in a shadow tree.
Product: WebKit Reporter: Shinya Kawanaka <shinyak@chromium.org>
Component: Event HandlingAssignee: Shinya Kawanaka <shinyak@chromium.org>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov@chromium.org, dominicc@chromium.org, gns@gnome.org, hayato@chromium.org, morrita@google.com, philn@igalia.com, rniwa@webkit.org, shinyak@chromium.org, tasak@google.com, webkit.review.bot@gmail.com, xan.lopez@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 82429, 82699    
Bug Blocks: 82697, 86598    
Attachments:
Description Flags
WIP
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description From 2012-03-29 23:47:35 PST
After fixing Bug 82429, ShadowRoot should return selection whose range is really in a shadow tree.
------- Comment #1 From 2012-05-09 21:59:39 PST -------
Created an attachment (id=141085) [details]
WIP
------- Comment #2 From 2012-05-09 22:02:18 PST -------
Attachment 141085 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1
Source/WebCore/dom/TreeScope.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #3 From 2012-05-09 22:20:43 PST -------
(From update of attachment 141085 [details])
Attachment 141085 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12648758
------- Comment #4 From 2012-05-09 22:25:25 PST -------
(From update of attachment 141085 [details])
Attachment 141085 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12644941
------- Comment #5 From 2012-05-09 22:29:32 PST -------
(From update of attachment 141085 [details])
Attachment 141085 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12653803
------- Comment #6 From 2012-05-09 22:36:47 PST -------
(From update of attachment 141085 [details])
Attachment 141085 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12649928
------- Comment #7 From 2012-05-09 22:56:42 PST -------
(From update of attachment 141085 [details])
Attachment 141085 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12648764
------- Comment #8 From 2012-05-09 23:21:50 PST -------
(From update of attachment 141085 [details])
Attachment 141085 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12656882
------- Comment #9 From 2012-05-10 00:16:24 PST -------
(From update of attachment 141085 [details])
Attachment 141085 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12649944
------- Comment #10 From 2012-05-13 22:32:53 PST -------
Created an attachment (id=141639) [details]
Patch
------- Comment #11 From 2012-05-13 22:35:50 PST -------
Created an attachment (id=141640) [details]
Patch
------- Comment #12 From 2012-05-13 23:06:08 PST -------
(From update of attachment 141640 [details])
Attachment 141640 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12659014

New failing tests:
fast/dom/shadow/selection-in-shadow.html
------- Comment #13 From 2012-05-13 23:06:13 PST -------
Created an attachment (id=141645) [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
------- Comment #14 From 2012-05-13 23:16:00 PST -------
Ah, old selection tests are failing...
It should be deprecated or something...
------- Comment #15 From 2012-05-13 23:29:30 PST -------
Created an attachment (id=141646) [details]
Patch
------- Comment #16 From 2012-05-14 00:30:54 PST -------
(From update of attachment 141646 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=141646&action=review

Could you test for following?
- ShadowRoot of orphan(out-of-document) node

> LayoutTests/ChangeLog:26
> +

Duplicated entry.
------- Comment #17 From 2012-05-14 00:42:22 PST -------
(In reply to comment #16)
> (From update of attachment 141646 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141646&action=review
> 
> Could you test for following?
> - ShadowRoot of orphan(out-of-document) node

sure. wait a moment...

> 
> > LayoutTests/ChangeLog:26
> > +
> 
> Duplicated entry.

Oops...
------- Comment #18 From 2012-05-14 01:08:03 PST -------
Created an attachment (id=141662) [details]
Patch
------- Comment #19 From 2012-05-14 23:19:55 PST -------
(From update of attachment 141662 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=141662&action=review

> Source/WebCore/dom/TreeScope.cpp:145
> +    if (this != rootNode()->document())
> +        return rootNode()->document()->getSelection();

Why are you making this change? You should at least explain why you're making this change in the change log.

> Source/WebCore/dom/TreeScopeAdjuster.cpp:47
> -    do {
> +    while (node) {
>          if (node->treeScope() == treeScope())
>              return node;
>          if (!node->isInShadowTree())
>              return 0;
> -    } while ((node = node->shadowAncestorNode()));
> +        node = node->shadowAncestorNode();
> +    }

Why are you re-writing this do-while loop to a while loop?
Again, this needs to be explained somewhere.

> Source/WebCore/page/DOMSelection.cpp:503
> +Node* DOMSelection::adjustedNode(const Position& position) const

I would call it shadowAdjustedNode so that I know with respect to what it is adjusted.

> Source/WebCore/page/DOMSelection.cpp:509
> +    Node* adjustedNode = TreeScopeAdjuster(m_treeScope).ancestorInThisScope(containerNode);

Why do we need a class for this? All these proliferations of tiny classes is going to increase our build time.
It should have been much better if this was just a function in TreeScope, VisibleSelection, or even in htmlediting.

> LayoutTests/editing/shadow/selection-of-shadowroot-expected.txt:4
> +PASS divs[0] is selection.anchorNode.parentNode
> +PASS anchorTreeScopeRoot is internals.treeScopeRootNode(selection.focusNode)
> +PASS anchorTreeScopeRoot is internals.treeScopeRootNode(selection.baseNode)
> +PASS anchorTreeScopeRoot is internals.treeScopeRootNode(selection.extentNode)

These outputs don't tell me what this test is testing.
Ideal test outputs makes what and why we're asserting self-evident.
------- Comment #20 From 2012-05-14 23:38:38 PST -------
(From update of attachment 141662 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=141662&action=review

>> Source/WebCore/dom/TreeScope.cpp:145
>> +        return rootNode()->document()->getSelection();
> 
> Why are you making this change? You should at least explain why you're making this change in the change log.

This was because I would like to use the same instance of DOMSelection for document and shadow root. 
I'll comment it just before this in next patch.

>> Source/WebCore/dom/TreeScopeAdjuster.cpp:47
>> +    }
> 
> Why are you re-writing this do-while loop to a while loop?
> Again, this needs to be explained somewhere.

This is because node can be null.

>> Source/WebCore/page/DOMSelection.cpp:509
>> +    Node* adjustedNode = TreeScopeAdjuster(m_treeScope).ancestorInThisScope(containerNode);
> 
> Why do we need a class for this? All these proliferations of tiny classes is going to increase our build time.
> It should have been much better if this was just a function in TreeScope, VisibleSelection, or even in htmlediting.

Hmm... Actually I was proposed to create this class by morrita. But I'm OK to remove it. Let's do in another patch in that case...

>> LayoutTests/editing/shadow/selection-of-shadowroot-expected.txt:4
>> +PASS anchorTreeScopeRoot is internals.treeScopeRootNode(selection.extentNode)
> 
> These outputs don't tell me what this test is testing.
> Ideal test outputs makes what and why we're asserting self-evident.

OK. Thanks for mentioning this. I'll update the output to be comprehensive.
------- Comment #21 From 2012-05-14 23:40:10 PST -------
(From update of attachment 141662 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=141662&action=review

>>> Source/WebCore/dom/TreeScope.cpp:145
>>> +        return rootNode()->document()->getSelection();
>> 
>> Why are you making this change? You should at least explain why you're making this change in the change log.
> 
> This was because I would like to use the same instance of DOMSelection for document and shadow root. 
> I'll comment it just before this in next patch.

Let me discard the previous comment... I'll comment it in ChangeLog.
------- Comment #22 From 2012-05-15 02:21:31 PST -------
Created an attachment (id=141899) [details]
Patch
------- Comment #23 From 2012-05-15 02:29:47 PST -------
(From update of attachment 141899 [details])
Attachment 141899 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12706250
------- Comment #24 From 2012-05-15 03:01:14 PST -------
(From update of attachment 141899 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=141899&action=review

> Source/WebCore/page/DOMSelection.cpp:532
> +        return position.offsetInContainerNode();

This is going to assert when position is not of the type PositionIsOffsetInAnchor. You should call computeOffsetInContainerNode() instead.

> LayoutTests/editing/shadow/selection-of-orphan-shadowroot-expected.txt:1
> +Nodes of the selection for orphan shadow root should return null.

Nit: for an orphan shadow root.

> LayoutTests/editing/shadow/selection-of-shadowroot-expected.txt:14
> +Dragging from divs[0] to divs[2]

I still don't understand what divs[0] and divs[2] mean, and what output I should be expecting.
Also, all outputs on the right of PASS appear to be identical for each test case.
------- Comment #25 From 2012-05-16 00:16:13 PST -------
Created an attachment (id=142169) [details]
Patch
------- Comment #26 From 2012-05-16 00:17:17 PST -------
Created an attachment (id=142171) [details]
Patch
------- Comment #27 From 2012-05-16 00:19:37 PST -------
Thanks for reviewing!

(In reply to comment #24)
> (From update of attachment 141899 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141899&action=review
> 
> > Source/WebCore/page/DOMSelection.cpp:532
> > +        return position.offsetInContainerNode();
> 
> This is going to assert when position is not of the type PositionIsOffsetInAnchor. You should call computeOffsetInContainerNode() instead.
> 
> > LayoutTests/editing/shadow/selection-of-orphan-shadowroot-expected.txt:1
> > +Nodes of the selection for orphan shadow root should return null.
> 
> Nit: for an orphan shadow root.
> 
> > LayoutTests/editing/shadow/selection-of-shadowroot-expected.txt:14
> > +Dragging from divs[0] to divs[2]
> 
> I still don't understand what divs[0] and divs[2] mean, and what output I should be expecting.
> Also, all outputs on the right of PASS appear to be identical for each test case.


Hmm...
I've updated the patch to reduce the test and added a few description. I think this is much more understandable than before... But if you don't think so, could you suggest an advice to make it more comprehensive?
------- Comment #28 From 2012-05-16 00:28:46 PST -------
(From update of attachment 142171 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=142171&action=review

> Source/WebCore/ChangeLog:25
> +        (WebCore::TreeScope::getSelection):
> +          When shadow DOM feature is not enabled, we want to use the same instance of DOMSelection
> +          among Document and ShadowRoot.

We normally start description immediately after: followed by a single space, and we don't indent descriptions by two-spaces.

> Source/WebCore/ChangeLog:29
> +          Since node could be null, I've added a node check code.

Ditto.

> Source/WebCore/ChangeLog:43
> +          Gets the corresponding node in the m_treeScope from the Position.

Ditto.

> Source/WebCore/ChangeLog:46
> +          Gets the corresponding node offset in the m_treeScope from the Position.

Ditto.
------- Comment #29 From 2012-05-16 01:31:27 PST -------
(In reply to comment #28)
> (From update of attachment 142171 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142171&action=review
> 
> > Source/WebCore/ChangeLog:25
> > +        (WebCore::TreeScope::getSelection):
> > +          When shadow DOM feature is not enabled, we want to use the same instance of DOMSelection
> > +          among Document and ShadowRoot.
> 
> We normally start description immediately after: followed by a single space, and we don't indent descriptions by two-spaces.
> 
> > Source/WebCore/ChangeLog:29
> > +          Since node could be null, I've added a node check code.
> 
> Ditto.
> 
> > Source/WebCore/ChangeLog:43
> > +          Gets the corresponding node in the m_treeScope from the Position.
> 
> Ditto.
> 
> > Source/WebCore/ChangeLog:46
> > +          Gets the corresponding node offset in the m_treeScope from the Position.
> 
> Ditto.

Thanks for mentioning this... I didn't know that rule!
------- Comment #30 From 2012-05-16 01:35:22 PST -------
Created an attachment (id=142193) [details]
Patch for landing
------- Comment #31 From 2012-05-16 01:47:11 PST -------
After the bots become green, I'll land this patch.
------- Comment #32 From 2012-05-16 02:16:45 PST -------
(In reply to comment #31)
> After the bots become green, I'll land this patch.

Since cr-linux was tested by me, it should be OK to land now...
------- Comment #33 From 2012-05-16 03:07:43 PST -------
Committed r117249: <http://trac.webkit.org/changeset/117249>