Bug 82698

Summary: ShadowRoot.selection should return the selection whose range is in a shadow tree.
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: Event HandlingAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, gns, hayato, morrita, philn, rniwa, shinyak, tasak, webkit.review.bot, xan.lopez
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 Shinya Kawanaka 2012-03-29 23:47:35 PDT
After fixing Bug 82429, ShadowRoot should return selection whose range is really in a shadow tree.
Comment 1 Shinya Kawanaka 2012-05-09 21:59:39 PDT
Created attachment 141085 [details]
WIP
Comment 2 WebKit Review Bot 2012-05-09 22:02:18 PDT
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 Build Bot 2012-05-09 22:20:43 PDT
Comment on attachment 141085 [details]
WIP

Attachment 141085 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12648758
Comment 4 Build Bot 2012-05-09 22:25:25 PDT
Comment on attachment 141085 [details]
WIP

Attachment 141085 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12644941
Comment 5 Early Warning System Bot 2012-05-09 22:29:32 PDT
Comment on attachment 141085 [details]
WIP

Attachment 141085 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12653803
Comment 6 Early Warning System Bot 2012-05-09 22:36:47 PDT
Comment on attachment 141085 [details]
WIP

Attachment 141085 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12649928
Comment 7 Gyuyoung Kim 2012-05-09 22:56:42 PDT
Comment on attachment 141085 [details]
WIP

Attachment 141085 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12648764
Comment 8 Build Bot 2012-05-09 23:21:50 PDT
Comment on attachment 141085 [details]
WIP

Attachment 141085 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12656882
Comment 9 Gustavo Noronha (kov) 2012-05-10 00:16:24 PDT
Comment on attachment 141085 [details]
WIP

Attachment 141085 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12649944
Comment 10 Shinya Kawanaka 2012-05-13 22:32:53 PDT
Created attachment 141639 [details]
Patch
Comment 11 Shinya Kawanaka 2012-05-13 22:35:50 PDT
Created attachment 141640 [details]
Patch
Comment 12 WebKit Review Bot 2012-05-13 23:06:08 PDT
Comment on attachment 141640 [details]
Patch

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 WebKit Review Bot 2012-05-13 23:06:13 PDT
Created attachment 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 Shinya Kawanaka 2012-05-13 23:16:00 PDT
Ah, old selection tests are failing...
It should be deprecated or something...
Comment 15 Shinya Kawanaka 2012-05-13 23:29:30 PDT
Created attachment 141646 [details]
Patch
Comment 16 Hajime Morrita 2012-05-14 00:30:54 PDT
Comment on attachment 141646 [details]
Patch

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 Shinya Kawanaka 2012-05-14 00:42:22 PDT
(In reply to comment #16)
> (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

sure. wait a moment...

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

Oops...
Comment 18 Shinya Kawanaka 2012-05-14 01:08:03 PDT
Created attachment 141662 [details]
Patch
Comment 19 Ryosuke Niwa 2012-05-14 23:19:55 PDT
Comment on attachment 141662 [details]
Patch

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 Shinya Kawanaka 2012-05-14 23:38:38 PDT
Comment on attachment 141662 [details]
Patch

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 Shinya Kawanaka 2012-05-14 23:40:10 PDT
Comment on attachment 141662 [details]
Patch

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 Shinya Kawanaka 2012-05-15 02:21:31 PDT
Created attachment 141899 [details]
Patch
Comment 23 WebKit Review Bot 2012-05-15 02:29:47 PDT
Comment on attachment 141899 [details]
Patch

Attachment 141899 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12706250
Comment 24 Ryosuke Niwa 2012-05-15 03:01:14 PDT
Comment on attachment 141899 [details]
Patch

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 Shinya Kawanaka 2012-05-16 00:16:13 PDT
Created attachment 142169 [details]
Patch
Comment 26 Shinya Kawanaka 2012-05-16 00:17:17 PDT
Created attachment 142171 [details]
Patch
Comment 27 Shinya Kawanaka 2012-05-16 00:19:37 PDT
Thanks for reviewing!

(In reply to comment #24)
> (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.


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 Ryosuke Niwa 2012-05-16 00:28:46 PDT
Comment on attachment 142171 [details]
Patch

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 Shinya Kawanaka 2012-05-16 01:31:27 PDT
(In reply to comment #28)
> (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.

Thanks for mentioning this... I didn't know that rule!
Comment 30 Shinya Kawanaka 2012-05-16 01:35:22 PDT
Created attachment 142193 [details]
Patch for landing
Comment 31 Shinya Kawanaka 2012-05-16 01:47:11 PDT
After the bots become green, I'll land this patch.
Comment 32 Shinya Kawanaka 2012-05-16 02:16:45 PDT
(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 Shinya Kawanaka 2012-05-16 03:07:43 PDT
Committed r117249: <http://trac.webkit.org/changeset/117249>