ShadowRoot's getSelection().getRangeAt(0) returns an incorrect Range object

ShadowRoot's getSelection().getRangeAt(0) returns an incorrect Range object

DOMSelection::getRangeAt() uses different calculation from what we do in
other methods such as DOMSelection::anchorNode(), and it always returns
parentOrShadowHostNode(), which is incorrect.

This patch makes DOMSelection::getRangeAt() to use the same logic as
DOMSelection::anchorNode() etc. if the selection is within a shadow
tree.

We keep existing code if the selection is not within a shadow tree
because it has a special case to return m_logicalSelection so that
we can return the range set by scripts without it being canonicalized.

BUG=380690
TEST=LayoutTests/editing/shadow/getRangeAt-end-of-text-node.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187976

[kojii, 2015-01-06 08:03:32]

kojii@chromium.org changed reviewers:
+ yosin@google.com

[kojii, 2015-01-06 08:03:33]

PTAL!

[kojii, 2015-01-06 08:18:33]

kojii@chromium.org changed reviewers:
+ yosin@chromium.org
- yosin@google.com

[yosin_UTC9, 2015-01-06 08:39:37]

https://codereview.chromium.org/800633004/diff/20001/LayoutTests/editing/shad...
File LayoutTests/editing/shadow/getRangeAt-end-of-text-node.html (right):

https://codereview.chromium.org/800633004/diff/20001/LayoutTests/editing/shad...
LayoutTests/editing/shadow/getRangeAt-end-of-text-node.html:11:
assert_equals(selection.anchorNode, textNode);
Since we change |getRangeAt()| function, we don't need to check |anchorNode|,
|focusNode|, and so on.

https://codereview.chromium.org/800633004/diff/20001/LayoutTests/editing/shad...
LayoutTests/editing/shadow/getRangeAt-end-of-text-node.html:23:
selection.setBaseAndExtent(textNode, startOffset, textNode, endOffset);
Since, Firefox doesn't support |setBaseAndExtend()|, please use |collaspe()| and
|extend()| supported in both Chrome and Firefox.

https://codereview.chromium.org/800633004/diff/20001/LayoutTests/editing/shad...
LayoutTests/editing/shadow/getRangeAt-end-of-text-node.html:48: test(function ()
{
Since we change |getRangeAt()| function, we don't need to check |anchorNode|,
|focusNode|, and so on.

https://codereview.chromium.org/800633004/diff/20001/Source/core/editing/DOMS...
File Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/800633004/diff/20001/Source/core/editing/DOMS...
Source/core/editing/DOMSelection.cpp:170: FrameSelection& selection =
m_frame->selection();
I think |isCollapsed()| should be |return anchorNode() != focusNode() ||
  anchorOffset() != focusOffset();|

If above condition doesn't match to current behavior, let's file another bug and
fix to this.

https://codereview.chromium.org/800633004/diff/20001/Source/core/editing/DOMS...
Source/core/editing/DOMSelection.cpp:384: Position anchor =
anchorPosition(visibleSelection());
I think we can implement as below:
if (!isShadowRoot(m_treeScope.rootNode())
  return m_frame->selection().firstRange();
auto const anchorNode = anchorNode();
auto const anchorOffset = anchorOffset();
auto const focusNode = focusNode();
auto const focusOffset = focusOffset();
return Range::create(anchorNode->document(), ....);

[yosin_UTC9, 2015-01-07 01:06:29]

LGTM w/ nit

https://codereview.chromium.org/800633004/diff/40001/Source/core/editing/DOMS...
File Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/800633004/diff/40001/Source/core/editing/DOMS...
Source/core/editing/DOMSelection.cpp:376: ASSERT(anchor);
I think we don't need to |ASSERT(anchor)| here. We've got null pointer exception
in somewhere in |Range::create()| and we can get stack trace from crash log or
Clusterfuzz.

[kojii, 2015-01-07 05:35:41]

Patchset #5 (id:80001) has been deleted

[kojii, 2015-01-07 05:35:57]

Patchset #5 (id:100001) has been deleted

[kojii, 2015-01-07 05:36:18]

Patchset #5 (id:120001) has been deleted

[kojii, 2015-01-07 06:53:51]

kojii@chromium.org changed reviewers:
+ yutak@chromium.org

[kojii, 2015-01-07 06:53:53]

yutak@, PTAL!

https://codereview.chromium.org/800633004/diff/20001/LayoutTests/editing/shad...
File LayoutTests/editing/shadow/getRangeAt-end-of-text-node.html (right):

https://codereview.chromium.org/800633004/diff/20001/LayoutTests/editing/shad...
LayoutTests/editing/shadow/getRangeAt-end-of-text-node.html:11:
assert_equals(selection.anchorNode, textNode);
On 2015/01/06 08:39:37, Yosi_afk_til_01_05 wrote:
> Since we change |getRangeAt()| function, we don't need to check |anchorNode|,
> |focusNode|, and so on.

Done.

https://codereview.chromium.org/800633004/diff/20001/LayoutTests/editing/shad...
LayoutTests/editing/shadow/getRangeAt-end-of-text-node.html:23:
selection.setBaseAndExtent(textNode, startOffset, textNode, endOffset);
On 2015/01/06 08:39:36, Yosi_afk_til_01_05 wrote:
> Since, Firefox doesn't support |setBaseAndExtend()|, please use |collaspe()|
and
> |extend()| supported in both Chrome and Firefox.

Done.

https://codereview.chromium.org/800633004/diff/20001/LayoutTests/editing/shad...
LayoutTests/editing/shadow/getRangeAt-end-of-text-node.html:48: test(function ()
{
On 2015/01/06 08:39:37, Yosi_afk_til_01_05 wrote:
> Since we change |getRangeAt()| function, we don't need to check |anchorNode|,
> |focusNode|, and so on.

Done.

https://codereview.chromium.org/800633004/diff/20001/Source/core/editing/DOMS...
File Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/800633004/diff/20001/Source/core/editing/DOMS...
Source/core/editing/DOMSelection.cpp:170: FrameSelection& selection =
m_frame->selection();
On 2015/01/06 08:39:37, Yosi_afk_til_01_05 wrote:
> I think |isCollapsed()| should be |return anchorNode() != focusNode() ||
>   anchorOffset() != focusOffset();|
> 
> If above condition doesn't match to current behavior, let's file another bug
and
> fix to this.

Done.

https://codereview.chromium.org/800633004/diff/20001/Source/core/editing/DOMS...
Source/core/editing/DOMSelection.cpp:384: Position anchor =
anchorPosition(visibleSelection());
On 2015/01/06 08:39:37, Yosi_afk_til_01_05 wrote:
> I think we can implement as below:
> if (!isShadowRoot(m_treeScope.rootNode())
>   return m_frame->selection().firstRange();
> auto const anchorNode = anchorNode();
> auto const anchorOffset = anchorOffset();
> auto const focusNode = focusNode();
> auto const focusOffset = focusOffset();
> return Range::create(anchorNode->document(), ....);

Done.

https://codereview.chromium.org/800633004/diff/40001/Source/core/editing/DOMS...
File Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/800633004/diff/40001/Source/core/editing/DOMS...
Source/core/editing/DOMSelection.cpp:376: ASSERT(anchor);
On 2015/01/07 01:06:29, Yosi_UTC9 wrote:
> I think we don't need to |ASSERT(anchor)| here. We've got null pointer
exception
> in somewhere in |Range::create()| and we can get stack trace from crash log or
> Clusterfuzz.

Done.

[Yuta Kitamura, 2015-01-07 07:50:57]

https://codereview.chromium.org/800633004/diff/140001/Source/core/editing/DOM...
File Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/800633004/diff/140001/Source/core/editing/DOM...
Source/core/editing/DOMSelection.cpp:377: return
Range::create(*anchor.document(), focusNode(), focusOffset(),
shadowAdjustedNode(anchor), anchorOffset());
Does anchorOffset() work for shadowAdjustedNode(anchor)?

My understanding is that shadowAdjustedNode() may return
a Node in a different place, thus anchorOffset() may not
work if that happens.

[kojii, 2015-01-07 08:20:43]

https://codereview.chromium.org/800633004/diff/140001/Source/core/editing/DOM...
File Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/800633004/diff/140001/Source/core/editing/DOM...
Source/core/editing/DOMSelection.cpp:377: return
Range::create(*anchor.document(), focusNode(), focusOffset(),
shadowAdjustedNode(anchor), anchorOffset());
On 2015/01/07 07:50:57, Yuta Kitamura wrote:
> Does anchorOffset() work for shadowAdjustedNode(anchor)?
> 
> My understanding is that shadowAdjustedNode() may return
> a Node in a different place, thus anchorOffset() may not
> work if that happens.

On 2015/01/07 07:50:57, Yuta Kitamura wrote:
> Does anchorOffset() work for shadowAdjustedNode(anchor)?
> 
> My understanding is that shadowAdjustedNode() may return
> a Node in a different place, thus anchorOffset() may not
> work if that happens.

Yes, this |anchorOffset()| is |DOMSelection::anchorOffset()|
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

which gets offset from |shadowAdjustedNode()|.

In other words, this code actually calculates adjustedNode
twice -- once for node and once for offset within |anchorOffset()|,
which I tried to avoid in Patch Set 2, but Yosi preferred
to calculate twice to reduce the number of lines in this patch.

[Yuta Kitamura, 2015-01-07 08:47:54]

lgtm

https://codereview.chromium.org/800633004/diff/140001/Source/core/editing/DOM...
File Source/core/editing/DOMSelection.cpp (right):

https://codereview.chromium.org/800633004/diff/140001/Source/core/editing/DOM...
Source/core/editing/DOMSelection.cpp:377: return
Range::create(*anchor.document(), focusNode(), focusOffset(),
shadowAdjustedNode(anchor), anchorOffset());
On 2015/01/07 08:20:43, koji wrote:
> Yes, this |anchorOffset()| is |DOMSelection::anchorOffset()|
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> which gets offset from |shadowAdjustedNode()|.
> 
> In other words, this code actually calculates adjustedNode
> twice -- once for node and once for offset within |anchorOffset()|,
> which I tried to avoid in Patch Set 2, but Yosi preferred
> to calculate twice to reduce the number of lines in this patch.

Okay, then the PS5 looks fine to me.

I don't have a strong preference on which of PS2 and PS5
is better (I somewhat agree that the PS2 has too much code
churn, while the correctness of this line in PS5 is not very
obvious).

[kojii, 2015-01-07 15:28:21]

On 2015/01/07 08:47:54, Yuta Kitamura wrote:
> Okay, then the PS5 looks fine to me.

Thank you!

> I don't have a strong preference on which of PS2 and PS5
> is better (I somewhat agree that the PS2 has too much code
> churn, while the correctness of this line in PS5 is not very
> obvious).

I agree. Yosi and I talked about that and discussed a few other options.

But I prefer to go this patch without including larger re-writes nor refactoring
since the progress of W3C Selection API will let us re-visit here sooner or
later,
and there are at least 3-4 people waiting for this bug to be fixed.

Thank you for your support as always.

[kojii, 2015-01-07 15:29:04]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2015-01-07 15:30:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/800633004/140001

[commit-bot: I haz the power, 2015-01-07 15:33:38]

Committed patchset #5 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187976