Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
848843002:
VisibleSelection::nonBoundaryShadowTreeRootNode should return null when its anchor is a shadow root (Closed)
DescriptionVisibleSelection::nonBoundaryShadowTreeRootNode should return null when its anchor is a shadow root
TEST=Automated. Added ua-shadow-select-all-crash.html.
BUG=447906
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188788
Patch Set 1 #
Total comments: 12
Patch Set 2 : #Patch Set 3 : #
Messages
Total messages: 16 (5 generated)
keishi@chromium.org changed reviewers: + yosin@chromium.org
It looks like when an input with focus changes type, the frame selection has an anchor node which is a shadow root. When FrameSelection::selectAll is called, Node::nonBoundaryShadowTreeRootNode is called on the anchor node which is a shadow root, causing it to fail the !isShadowRoot() assert. VisibleSelection::nonBoundaryShadowTreeRootNode only seems to be used in FrameSelection::selectAll so I think we can safely change it.
https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... File LayoutTests/fast/forms/ua-shadow-select-all-crash.html (right): https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... LayoutTests/fast/forms/ua-shadow-select-all-crash.html:1: <html> nit: Could you add "<!DOCTYPE html>" to make Blink parser in strict mode? https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... LayoutTests/fast/forms/ua-shadow-select-all-crash.html:7: var input1=document.createElement('input'); nit: Please put spaces around |=| https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... LayoutTests/fast/forms/ua-shadow-select-all-crash.html:8: button1.appendChild(input1); Can we put <input type="week"> in markup rather than constructing in script? https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... LayoutTests/fast/forms/ua-shadow-select-all-crash.html:10: input1.type='week'; nit: Please put spaces around |=| https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... LayoutTests/fast/forms/ua-shadow-select-all-crash.html:12: document.execCommand('SelectAll', false, false); nit: We need first argument only. https://codereview.chromium.org/848843002/diff/1/Source/core/editing/VisibleS... File Source/core/editing/VisibleSelection.cpp (right): https://codereview.chromium.org/848843002/diff/1/Source/core/editing/VisibleS... Source/core/editing/VisibleSelection.cpp:755: return start().deprecatedNode() && !start().deprecatedNode()->isShadowRoot() ? start().deprecatedNode()->nonBoundaryShadowTreeRootNode() : 0; I think |start()| should not point to shadow root. It seems selection adjust code after node removal made this. So, it is better to change adjust code.
https://codereview.chromium.org/848843002/diff/1/Source/core/editing/VisibleS... File Source/core/editing/VisibleSelection.cpp (right): https://codereview.chromium.org/848843002/diff/1/Source/core/editing/VisibleS... Source/core/editing/VisibleSelection.cpp:755: return start().deprecatedNode() && !start().deprecatedNode()->isShadowRoot() ? start().deprecatedNode()->nonBoundaryShadowTreeRootNode() : 0; On 2015/01/14 01:33:44, Yosi_UTC9 wrote: > I think |start()| should not point to shadow root. It seems selection adjust > code after node removal made this. So, it is better to change adjust code. I have confirmed that this is what's happening. 1. JS code sets selection to <input type=text>'s UA shadow editor element. (input1.setSelectionRange(0)) 2. input1's type is changed to 'week'. 3. TextFieldInputType::destroyShadowSubtree 4. FrameSelection::nodeWillBeRemoved 5. FrameSelection::respondToNodeModification 6. positionInParentBeforeNode returns the UA shadow root (because it is the parent of the editor element) which is then set to the VisualSelection::m_start. How should this work? Where should the selection go when all the elements in the UA shadow root is removed? Should positionInParentBeforeNode return the shadow host element instead of the shadow root?
yosin@chromium.org changed reviewers: + hayato@chromium.org
On 2015/01/14 05:04:51, keishi wrote: > https://codereview.chromium.org/848843002/diff/1/Source/core/editing/VisibleS... > File Source/core/editing/VisibleSelection.cpp (right): > > https://codereview.chromium.org/848843002/diff/1/Source/core/editing/VisibleS... > Source/core/editing/VisibleSelection.cpp:755: return start().deprecatedNode() && > !start().deprecatedNode()->isShadowRoot() ? > start().deprecatedNode()->nonBoundaryShadowTreeRootNode() : 0; > On 2015/01/14 01:33:44, Yosi_UTC9 wrote: > > I think |start()| should not point to shadow root. It seems selection adjust > > code after node removal made this. So, it is better to change adjust code. > > I have confirmed that this is what's happening. > 1. JS code sets selection to <input type=text>'s UA shadow editor element. > (input1.setSelectionRange(0)) > 2. input1's type is changed to 'week'. > 3. TextFieldInputType::destroyShadowSubtree > 4. FrameSelection::nodeWillBeRemoved > 5. FrameSelection::respondToNodeModification > 6. positionInParentBeforeNode returns the UA shadow root (because it is the > parent of the editor element) which is then set to the VisualSelection::m_start. > > How should this work? Where should the selection go when all the elements in the > UA shadow root is removed? Should positionInParentBeforeNode return the shadow > host element instead of the shadow root? Add hayato@ as shadow DOM expert. I think selection should collapse to shadow host when shadow root, regardless UA and author, has no children. hyato@, WDYT?
On 2015/01/15 01:46:55, Yosi_UTC9 wrote: > On 2015/01/14 05:04:51, keishi wrote: > > > https://codereview.chromium.org/848843002/diff/1/Source/core/editing/VisibleS... > > File Source/core/editing/VisibleSelection.cpp (right): > > > > > https://codereview.chromium.org/848843002/diff/1/Source/core/editing/VisibleS... > > Source/core/editing/VisibleSelection.cpp:755: return start().deprecatedNode() > && > > !start().deprecatedNode()->isShadowRoot() ? > > start().deprecatedNode()->nonBoundaryShadowTreeRootNode() : 0; > > On 2015/01/14 01:33:44, Yosi_UTC9 wrote: > > > I think |start()| should not point to shadow root. It seems selection adjust > > > code after node removal made this. So, it is better to change adjust code. > > > > I have confirmed that this is what's happening. > > 1. JS code sets selection to <input type=text>'s UA shadow editor element. > > (input1.setSelectionRange(0)) > > 2. input1's type is changed to 'week'. > > 3. TextFieldInputType::destroyShadowSubtree > > 4. FrameSelection::nodeWillBeRemoved > > 5. FrameSelection::respondToNodeModification > > 6. positionInParentBeforeNode returns the UA shadow root (because it is the > > parent of the editor element) which is then set to the > VisualSelection::m_start. > > > > How should this work? Where should the selection go when all the elements in > the > > UA shadow root is removed? Should positionInParentBeforeNode return the shadow > > host element instead of the shadow root? > > Add hayato@ as shadow DOM expert. > I think selection should collapse to shadow host when shadow root, regardless > UA and author, has no children. > > hyato@, WDYT? Aside from this case, is there any case where shadowRoot.getSelection().anchorNode, or something similar, can return a shadow root? If so, I don't see any reason for us to collapse it to shadow host in this case...
On 2015/01/15 02:13:11, hayato wrote: > On 2015/01/15 01:46:55, Yosi_UTC9 wrote: > > On 2015/01/14 05:04:51, keishi wrote: > > > > > > https://codereview.chromium.org/848843002/diff/1/Source/core/editing/VisibleS... > > > File Source/core/editing/VisibleSelection.cpp (right): > > > > > > > > > https://codereview.chromium.org/848843002/diff/1/Source/core/editing/VisibleS... > > > Source/core/editing/VisibleSelection.cpp:755: return > start().deprecatedNode() > > && > > > !start().deprecatedNode()->isShadowRoot() ? > > > start().deprecatedNode()->nonBoundaryShadowTreeRootNode() : 0; > > > On 2015/01/14 01:33:44, Yosi_UTC9 wrote: > > > > I think |start()| should not point to shadow root. It seems selection > adjust > > > > code after node removal made this. So, it is better to change adjust code. > > > > > > I have confirmed that this is what's happening. > > > 1. JS code sets selection to <input type=text>'s UA shadow editor element. > > > (input1.setSelectionRange(0)) > > > 2. input1's type is changed to 'week'. > > > 3. TextFieldInputType::destroyShadowSubtree > > > 4. FrameSelection::nodeWillBeRemoved > > > 5. FrameSelection::respondToNodeModification > > > 6. positionInParentBeforeNode returns the UA shadow root (because it is the > > > parent of the editor element) which is then set to the > > VisualSelection::m_start. > > > > > > How should this work? Where should the selection go when all the elements in > > the > > > UA shadow root is removed? Should positionInParentBeforeNode return the > shadow > > > host element instead of the shadow root? > > > > Add hayato@ as shadow DOM expert. > > I think selection should collapse to shadow host when shadow root, regardless > > UA and author, has no children. > > > > hyato@, WDYT? > > Aside from this case, is there any case where > shadowRoot.getSelection().anchorNode, or something similar, can return a shadow > root? > If so, I don't see any reason for us to collapse it to shadow host in this > case... LGTM. We discussed offline. At this time, we're not clear how we adjust selection after removing node in shadow tree. We'll define behavior later. So, at mean time, this patch is reasonable.
https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... File LayoutTests/fast/forms/ua-shadow-select-all-crash.html (right): https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... LayoutTests/fast/forms/ua-shadow-select-all-crash.html:1: <html> On 2015/01/14 01:33:44, Yosi_UTC9 wrote: > nit: Could you add "<!DOCTYPE html>" to make Blink parser in strict mode? Done. https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... LayoutTests/fast/forms/ua-shadow-select-all-crash.html:7: var input1=document.createElement('input'); On 2015/01/14 01:33:44, Yosi_UTC9 wrote: > nit: Please put spaces around |=| Done. https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... LayoutTests/fast/forms/ua-shadow-select-all-crash.html:8: button1.appendChild(input1); On 2015/01/14 01:33:44, Yosi_UTC9 wrote: > Can we put <input type="week"> in markup rather than constructing in script? Done. Changing the type later is the important bit so I added <input type=text> in markup https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... LayoutTests/fast/forms/ua-shadow-select-all-crash.html:10: input1.type='week'; On 2015/01/14 01:33:44, Yosi_UTC9 wrote: > nit: Please put spaces around |=| Done. https://codereview.chromium.org/848843002/diff/1/LayoutTests/fast/forms/ua-sh... LayoutTests/fast/forms/ua-shadow-select-all-crash.html:12: document.execCommand('SelectAll', false, false); On 2015/01/14 01:33:44, Yosi_UTC9 wrote: > nit: We need first argument only. Done.
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/848843002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/848843002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=188788 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
