Bug 88500

Created attachment 146263 [details] Patch

Comment on attachment 146263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146263&action=review > Source/WebCore/editing/VisibleSelection.cpp:496 > + m_extent = adjustPositionForEnd(m_start.anchorNode()->treeScope(), m_end, m_start.containerNode()); Why are we using anchorNode's treeScope and then passing containerNode!? anchorNode != containerNode. Can we just pass m_start.containerNode() here and obtain the tree scope inside the function?

Created attachment 146775 [details] Patch

> Why are we using anchorNode's treeScope and then passing containerNode!? > anchorNode != containerNode. Can we just pass m_start.containerNode() here and obtain the tree scope inside the function? yeah... it was strange... I've update the patch. anchorNode()->treeScope() and containerNode()->treeScope() should be the same basically.

Comment on attachment 146775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146775&action=review > Source/WebCore/editing/VisibleSelection.cpp:501 > - m_extent = adjustPositionBefore(m_start.anchorNode()->treeScope(), m_end); > + m_extent = adjustPositionForEnd(m_start.containerNode()->treeScope(), m_end, m_start.containerNode()); Again, why do we need to call treeScope here? You can just call it inside the function, right? > Source/WebCore/editing/VisibleSelection.cpp:504 > - m_extent = adjustPositionAfter(m_end.anchorNode()->treeScope(), m_start); > + m_extent = adjustPositionForStart(m_end.containerNode()->treeScope(), m_start, m_end.containerNode()); Ditto. > LayoutTests/editing/shadow/breaking-editing-boundaries.html:10 > +<div contenteditable> > + BEFORE<div id="host" contenteditable="false">DRAG FROM<span id="target"> </span>HERE</div> > +</div> Please add a test description.

(In reply to comment #5) > (From update of attachment 146775 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146775&action=review > > > Source/WebCore/editing/VisibleSelection.cpp:501 > > - m_extent = adjustPositionBefore(m_start.anchorNode()->treeScope(), m_end); > > + m_extent = adjustPositionForEnd(m_start.containerNode()->treeScope(), m_end, m_start.containerNode()); > > Again, why do we need to call treeScope here? You can just call it inside the function, right? Ah, OK. I didn't understand what you said...

Created attachment 147466 [details] Patch

Comment on attachment 147466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147466&action=review > Source/WebCore/editing/VisibleSelection.cpp:468 > + if (Node* ancestor = treeScope->ancestorInThisScope(currentPosition.anchorNode())) { What if this position was an offset in the anchor node? Why is it okay to move it before or after the node? > Source/WebCore/editing/VisibleSelection.cpp:471 > + if (ancestor->contains(startContainerNode)) > + return positionAfterNode(ancestor); > + return positionBeforeNode(ancestor); Why do we need to modify position when the ancestor is in the same tree scope? That sounds red-herring.

(In reply to comment #8) > (From update of attachment 147466 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147466&action=review > > > Source/WebCore/editing/VisibleSelection.cpp:468 > > + if (Node* ancestor = treeScope->ancestorInThisScope(currentPosition.anchorNode())) { > > What if this position was an offset in the anchor node? Why is it okay to move it before or after the node? > Actually currentPosition.anchorNode()->treeScope() will not be equal to startContainerNode->treeScope() here. I should have asserted. (I'll update the patch soon) So treeScope->ancestorInThisScope(currentPosition.containerNode()) will never return currentPosition.containerNode() itself here. > > Source/WebCore/editing/VisibleSelection.cpp:471 > > + if (ancestor->contains(startContainerNode)) > > + return positionAfterNode(ancestor); > > + return positionBeforeNode(ancestor); > > Why do we need to modify position when the ancestor is in the same tree scope? That sounds red-herring. Let's think: '<div id="host">A<div>D' and host having a ShadowRoot whose innerHTML is "B<content></content>C". When selecting from A (including) to B (including), comparePosition() will judge as B is always before to A. So m_start will be B, and m_end will be A. A and B have the different tree scope, we have to adjust them. However, the current implementation on of adjustSelectionToAvoidCrossingShadowBoundaries() always move B to just after A (positionAfterNode(A)). This breaks the assumption m_start is before m_end. So in that case, we have to adjust A to positionBeforeNode(A). Node that we cannot always move A to positionBeforeNode(A). When selecting from D (including) to B (including), we don't want to select A. It's like if <div id="host"></div> is <input>, we don't want to select <input>. So in this case we want to adjust B to positionAfterNode(A).

Created attachment 147497 [details] Patch

(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 147466 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=147466&action=review > > > > > Source/WebCore/editing/VisibleSelection.cpp:468 > > > + if (Node* ancestor = treeScope->ancestorInThisScope(currentPosition.anchorNode())) { > > > > What if this position was an offset in the anchor node? Why is it okay to move it before or after the node? > > > > Actually currentPosition.anchorNode()->treeScope() will not be equal to startContainerNode->treeScope() here. Could be, but not always. > > > Source/WebCore/editing/VisibleSelection.cpp:471 > > > + if (ancestor->contains(startContainerNode)) > > > + return positionAfterNode(ancestor); > > > + return positionBeforeNode(ancestor); > > > > Why do we need to modify position when the ancestor is in the same tree scope? That sounds red-herring. > > Let's think: '<div id="host">A<div>D' and host having a ShadowRoot whose innerHTML is "B<content></content>C". > > When selecting from A (including) to B (including), comparePosition() will judge as B is always before to A. So m_start will be B, and m_end will be A. You can't say m_start is B. You can't talk about a position being at some node. A position is a point in the DOM tree consisting of anchor type, anchor, and offset. There are 5 anchor types, and anchor and offset mean different things in each type: PositionIsOffsetInAnchor - (anchor, offset) specifies the point in DOM. (div, 2) for example means that the position is before the 3rd children of the div (i.e. there are 2 children before this position). PositionIsBeforeAnchor - The position is before the anchor; offset is ignored. i.e. if we have <div><em>a</em>b</div> and the anchor is at "b", then the position is after the em element and before "b". This is the same position as (PositionIsOffsetInAnchor, div, 1) PositionIsAfterAnchor - The position is after the anchor. With the same example, the position is after "b" and is identical to (PositionIsOffsetInAnchor, div, 2) PositionIsBeforeChildren - The position is the first point in the anchor. If the anchor is at the em element above, then the position is before "a" is identical to (PositionIsOffsetInAnchor, em, 0) PositionIsAfterChildren - The position is the last point in the anchor. If the anchor is at the em element above, then the position is after "a" is identical to (PositionIsOffsetInAnchor, em, 1) > A and B have the different tree scope, we have to adjust them. However, the current implementation on of adjustSelectionToAvoidCrossingShadowBoundaries() always move B to just after A (positionAfterNode(A)). This breaks the assumption m_start is before m_end. So in that case, we have to adjust A to positionBeforeNode(A). > > Node that we cannot always move A to positionBeforeNode(A). When selecting from D (including) to B (including), we don't want to select A. It's like if <div id="host"></div> is <input>, we don't want to select <input>. So in this case we want to adjust B to positionAfterNode(A). Because of this, what you're talking here is ambiguous. For example, the position immediately before the letter "B" can be expressed as (PositionIsOffsetInAnchor, shadow root, 0), (PositionIsBeforeChildren, shadow root), (PositionIsBeforeAnchor, "B", 0), or (PositionIsOffsetInAnchor, "B", 0).

Comment on attachment 147497 [details] Patch Oops, I've uploaded a wrong patch...

> the current implementation on of adjustSelectionToAvoidCrossingShadowBoundaries() always move B to just after A (positionAfterNode(A)). Here, for example, we need to first identify which of 6 possible positions we're talking about offset 0, offset 1, before, after positions anchored at the text node "B" and beforechildren, offset 0 positions anchored at the shadow root.

> > Actually currentPosition.anchorNode()->treeScope() will not be equal to startContainerNode->treeScope() here. > > Could be, but not always. Since adjustPositionForEnd and adjustPositionForStart are used in adjustSelectionToAvoidCrossingShadowBoundaries() and it checks the the equality of treeScopes of m_start->anchorNode() and m_end->anchorNode()... if Position is valid, what kind of pattern makes them different? > Because of this, what you're talking here is ambiguous. For example, the position immediately before the letter "B" can be expressed as (PositionIsOffsetInAnchor, shadow root, 0), (PositionIsBeforeChildren, shadow root), (PositionIsBeforeAnchor, "B", 0), or (PositionIsOffsetInAnchor, "B", 0). OK. Let me try to make this strict... (in another reply)

Comment on attachment 147497 [details] Patch Attachment 147497 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12958134

Let's think: '<div id="host">A<div>D' and host having a ShadowRoot whose innerHTML is "B<content></content>C". When selecting from p1 (PositionIsOffsetAnchor, div, 1) to p2 (PositionIsOffsetInAnchor, shadowRoot, 0), comparePosition() will judge as p2 is always before to p1. So m_start will be p2, and m_end will be p1. The anchor nodes of p1 and p2 have the different tree scope, we have to adjust them. However, the current implementation on of adjustSelectionToAvoidCrossingShadowBoundaries() always move p2 to just after A (positionAfterNode(div), i.e. (PositionIsAfterAnchor, div)). This breaks the assumption m_start is before m_end. So in that case, we have to adjust A to positionBeforeNode(div) i.e. (PositionIsBeforeAnchor, div). Node that we cannot always move p1 to positionBeforeNode(div). When selecting from p3 (PositionIsOffsetAnchor, D's parent, 2), to p2, we don't want to select A. It's like if <div id="host"></div> is <input>, we don't want to select <input>. So in this case we want to adjust p2 to positionAfterNode(div).

Created attachment 147502 [details] Patch

Comment on attachment 147502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147502&action=review > Source/WebCore/ChangeLog:11 > + For example, let's think '<div id="host">A</div>D' and host has a Shadow DOM whose innerHTML is Nit: s/let's think/consider/ > Source/WebCore/ChangeLog:13 > + 'B<content></content>C'. When selecting from just after A (p1 = (PositionIsOffsetInAnchor, div, 1)) to > + B (p2 = (PositionIsOffsetInAnchor, shadowRoot, 0)), since comparePosition always judge as p2 is How does (PositionIsOffsetInAnchor, div, 1) make sense? When we have a shadow DOM on #host, #host should be treated as an atomic node. Selection should never be inside #host. Or is this a legacy editing offset?

> > Source/WebCore/ChangeLog:13 > > + 'B<content></content>C'. When selecting from just after A (p1 = (PositionIsOffsetInAnchor, div, 1)) to > > + B (p2 = (PositionIsOffsetInAnchor, shadowRoot, 0)), since comparePosition always judge as p2 is > > How does (PositionIsOffsetInAnchor, div, 1) make sense? When we have a shadow DOM on #host, #host should be treated as an atomic node. > Selection should never be inside #host. Or is this a legacy editing offset? Yeah, Selection should never be inside #host. But before adjusting, selection could be inside the host. So I wrote as (PositionIsOffsetInAnchor, div, 1). It's a position before calling adjustSelectionToAvoidCrossingShadowBoundaries().

(In reply to comment #19) > Yeah, Selection should never be inside #host. But before adjusting, selection could be inside the host. > So I wrote as (PositionIsOffsetInAnchor, div, 1). It's a position before calling adjustSelectionToAvoidCrossingShadowBoundaries(). Even before adjusting it, we should never have that position. Isn't it a legacy editing position? (i.e. m_isLegacyEditingPosition is true). If not, we need to figure out where this position is created and fix there as well.

(In reply to comment #20) > (In reply to comment #19) > > Yeah, Selection should never be inside #host. But before adjusting, selection could be inside the host. > > So I wrote as (PositionIsOffsetInAnchor, div, 1). It's a position before calling adjustSelectionToAvoidCrossingShadowBoundaries(). > > Even before adjusting it, we should never have that position. Isn't it a legacy editing position? (i.e. m_isLegacyEditingPosition is true). If not, we need to figure out where this position is created and fix there as well. Sorry for late response. Yes. It's a legacy editing position. (m_isLegacyEditingPosition is true)

(In reply to comment #21) > Sorry for late response. Yes. It's a legacy editing position. (m_isLegacyEditingPosition is true) Okay, that makes sense. In that case, (host, 0) and (host, 1) should be treated like before and after the host.

(In reply to comment #22) > (In reply to comment #21) > > Sorry for late response. Yes. It's a legacy editing position. (m_isLegacyEditingPosition is true) > > Okay, that makes sense. In that case, (host, 0) and (host, 1) should be treated like before and after the host. Really...? OK. I apologize I used a reduced example. Let me try to explain using the real example. # Before that I upload a new Patch...

Created attachment 147726 [details] Patch

Created attachment 147727 [details] Patch

(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > Sorry for late response. Yes. It's a legacy editing position. (m_isLegacyEditingPosition is true) > > > > Okay, that makes sense. In that case, (host, 0) and (host, 1) should be treated like before and after the host. > > Really...? Yes. See https://rniwa.com/2011-06-26/position-and-anchor-types/ In legacy editing positions, when the anchor node is "atomic", offset 0 and offset 1 mean before and after the anchor.

In the testcase, after performing eventSender.mouseMoveTo(nestedShadowHost.offsetLeft + 10, nestedShadowHost.offsetTop + nestedShadowHost.offsetHeight / 2), VisibleSelection::adjustSelectionToAvoidCrossingShadowBoundaries() will be executed a few times. In one of them, "if (m_baseIsFirst) { ... } " is executed. In this moment, m_baseIsFirst = false m_start = { m_anchorNode = ...snip.../SPAN[@id="shadow-host"]/#shadow-root[0]/SPAN/text() m_offset = 1, m_anchorType = 0, m_isLegacyEditingPosition = true } m_end = { m_anchorNode = ...snip.../SPAN[@id="host" and position() = 0]/text() m_offset = 13, m_anchorType = 0, m_isLegacyEditingPosition = true } This does not seem that we should treat such positons as before the host or after the host. I'm sorry for my explanation ambiguity...

I'm very sorry that my previous explanation was not correct or strict.

Comment on attachment 147727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147727&action=review > LayoutTests/editing/shadow/breaking-editing-boundaries.html:12 > + BEFORE<span id="host" contenteditable="false">DRAG FROM HERE<span id="target"> </span>FOO BAR</span> Nit: ALL CAPS DON'T READ WELL. Please use normal capitalization.

Created attachment 148141 [details] Patch for landing

Comment on attachment 148141 [details] Patch for landing Rejecting attachment 148141 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12970785

Created attachment 148184 [details] Patch for landing

Comment on attachment 148184 [details] Patch for landing Clearing flags on attachment: 148184 Committed r120666: <http://trac.webkit.org/changeset/120666>

All reviewed patches have been landed. Closing bug.