Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2021793002:
[Editing][CSS] Drag from a -webkit-user-select:none element should not start selection (Closed)
|
Created:
4 years, 6 months ago by yoichio Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dcheng, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Editing][CSS] Drag from a -webkit-user-select:none element should not start selection
This CL is updated of reverted one:
https://codereview.chromium.org/2001993002
This CL considers the reverted shadow boundary issue.
BUG=481985
Committed: https://crrev.com/08acdf950e1776571def8e6f16c288769e7e9e27
Cr-Commit-Position: refs/heads/master@{#412457}
Patch Set 1 : update #
Total comments: 10
Patch Set 2 : update #
Total comments: 1
Patch Set 3 : update comment #
Messages
Total messages: 40 (26 generated)
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== revenge user-select:none BUG= ========== to ========== revenge user-select:none BUG=481985 ==========
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== revenge user-select:none BUG=481985 ========== to ========== [Editing][CSS] Drag from a -webkit-user-select:none element should not start selection This CL is updated of reverted one: https://codereview.chromium.org/2001993002 This CL considers the reverted shadow boundary issue. BUG=481985 ==========
yoichio@chromium.org changed reviewers: + yosin@chromium.org
https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/mouse/drag_user_select_none.html (right): https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_user_select_none.html:8: assert_true(window.eventSender != null, 'Window should have eventSender'); assert_not_equals(window.eventSender, undefined, 'This test requires window.eventSender'); When |window| has no property named |eventSender|, |window.eventSender| returns |undefined| instead of |null|. https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/mouse/select_user_select_in_shadow.html (right): https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/select_user_select_in_shadow.html:8: if (!window.eventSender) Use assert_not_equals(window.eventSender, undefined, 'This test requires window.eventSender'); https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/select_user_select_in_shadow.html:11: var host = div.createShadowRoot(); Let's use |attachShadow({mode: 'open'})|, this is V1, instead of deprecating method |createShadowRoot()|, this is V0. https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/select_user_select_in_shadow.html:15: host.appendChild(child); We also want to have a test case for distributed node. https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:948: for (const Node* parent = FlatTreeTraversal::parent(*this); parent; parent = FlatTreeTraversal::parent(*parent)) { Could you add TODO to use |FlatTreeTraversal::ancestorsOf()|?
The CQ bit was checked by yoichio@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/mouse/drag_user_select_none.html (right): https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/drag_user_select_none.html:8: assert_true(window.eventSender != null, 'Window should have eventSender'); On 2016/08/16 06:34:42, Yosi_UTC9 wrote: > assert_not_equals(window.eventSender, undefined, 'This test requires > window.eventSender'); > > When |window| has no property named |eventSender|, |window.eventSender| returns > |undefined| instead of |null|. Done. https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/mouse/select_user_select_in_shadow.html (right): https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/select_user_select_in_shadow.html:8: if (!window.eventSender) On 2016/08/16 06:34:42, Yosi_UTC9 wrote: > Use assert_not_equals(window.eventSender, undefined, 'This test requires > window.eventSender'); Done. https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/select_user_select_in_shadow.html:11: var host = div.createShadowRoot(); On 2016/08/16 06:34:42, Yosi_UTC9 wrote: > Let's use |attachShadow({mode: 'open'})|, this is V1, instead of deprecating > method |createShadowRoot()|, this is V0. Done. https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/mouse/select_user_select_in_shadow.html:15: host.appendChild(child); On 2016/08/16 06:34:42, Yosi_UTC9 wrote: > We also want to have a test case for distributed node. Done. https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2021793002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:948: for (const Node* parent = FlatTreeTraversal::parent(*this); parent; parent = FlatTreeTraversal::parent(*parent)) { On 2016/08/16 06:34:42, Yosi_UTC9 wrote: > Could you add TODO to use |FlatTreeTraversal::ancestorsOf()|? Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yoichio@chromium.org changed reviewers: + tkent@chromium.org
tkent@, could you owner-review?
https://codereview.chromium.org/2021793002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2021793002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:950: // TODO(yoichio): Use FlatTreeTraversal::ancestorsOf(). Do you mean we don't have FlatTreeTraversal::ancestorsOf() yet?
On 2016/08/17 02:34:20, tkent wrote: > https://codereview.chromium.org/2021793002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/Node.cpp (right): > > https://codereview.chromium.org/2021793002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Node.cpp:950: // TODO(yoichio): Use > FlatTreeTraversal::ancestorsOf(). > Do you mean we don't have FlatTreeTraversal::ancestorsOf() yet? Yes. We don't have FlatTreeTraversal foreachs.
lgtm On 2016/08/17 at 03:53:04, yoichio wrote: > On 2016/08/17 02:34:20, tkent wrote: > > https://codereview.chromium.org/2021793002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/dom/Node.cpp (right): > > > > https://codereview.chromium.org/2021793002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/dom/Node.cpp:950: // TODO(yoichio): Use > > FlatTreeTraversal::ancestorsOf(). > > Do you mean we don't have FlatTreeTraversal::ancestorsOf() yet? > > Yes. We don't have FlatTreeTraversal foreachs. Please update the comment to something like "TODO(yoichio): Add FlatTreeTraversal::ancestorsOf(), and use it."
On 2016/08/17 03:58:34, tkent wrote: > lgtm > > On 2016/08/17 at 03:53:04, yoichio wrote: > > On 2016/08/17 02:34:20, tkent wrote: > > > > https://codereview.chromium.org/2021793002/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/dom/Node.cpp (right): > > > > > > > https://codereview.chromium.org/2021793002/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/dom/Node.cpp:950: // TODO(yoichio): Use > > > FlatTreeTraversal::ancestorsOf(). > > > Do you mean we don't have FlatTreeTraversal::ancestorsOf() yet? > > > > Yes. We don't have FlatTreeTraversal foreachs. > > Please update the comment to something like "TODO(yoichio): Add > FlatTreeTraversal::ancestorsOf(), and use it." Done.
The CQ bit was checked by yoichio@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2021793002/#ps80001 (title: "update comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Editing][CSS] Drag from a -webkit-user-select:none element should not start selection This CL is updated of reverted one: https://codereview.chromium.org/2001993002 This CL considers the reverted shadow boundary issue. BUG=481985 ========== to ========== [Editing][CSS] Drag from a -webkit-user-select:none element should not start selection This CL is updated of reverted one: https://codereview.chromium.org/2001993002 This CL considers the reverted shadow boundary issue. BUG=481985 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Editing][CSS] Drag from a -webkit-user-select:none element should not start selection This CL is updated of reverted one: https://codereview.chromium.org/2001993002 This CL considers the reverted shadow boundary issue. BUG=481985 ========== to ========== [Editing][CSS] Drag from a -webkit-user-select:none element should not start selection This CL is updated of reverted one: https://codereview.chromium.org/2001993002 This CL considers the reverted shadow boundary issue. BUG=481985 Committed: https://crrev.com/08acdf950e1776571def8e6f16c288769e7e9e27 Cr-Commit-Position: refs/heads/master@{#412457} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/08acdf950e1776571def8e6f16c288769e7e9e27 Cr-Commit-Position: refs/heads/master@{#412457}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:80001) has been created in https://codereview.chromium.org/2256083003/ by wfh@chromium.org. The reason for reverting is: this is causing hangs on latest canary. crbug.com/638868. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
