Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2965603002:
Update hover states when dragging the mouse into an element (Closed)
DescriptionUpdate hover states when dragging the mouse into an element
Now when we move the mouse into an element while pressing the left button,
we do not update the hover states of the element. We should update hover
states of the element, which is now underneath the mouse cursor no matter
the mouse button is pressed or not.
The design doc:
https://docs.google.com/document/d/1TgqvDg8L_TkEiNjBUaz39FDgxpp5aj9ZdIWeQSanKHY/
BUG=122746
Review-Url: https://codereview.chromium.org/2965603002
Cr-Commit-Position: refs/heads/master@{#486537}
Committed: https://chromium.googlesource.com/chromium/src/+/1c014b7ed8bd0c568f7f69b5f0c241f6fd635d2f
Patch Set 1 #Patch Set 2 : hover active chain #
Total comments: 3
Patch Set 3 : hover drag #
Total comments: 11
Patch Set 4 : hover drag #Messages
Total messages: 56 (44 generated)
The CQ bit was checked by lanwei@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...
Description was changed from ========== hover drag hover drag hover drag BUG= ========== to ========== Update hover states when dragging the mouse into an element Now when we move the mouse into an element while pressing the left button, we do not update the hover states of the element. We should update hover states of the element, which is now underneath the mouse cursor no matter the mouse button is pressed or not. BUG=122746 ==========
Description was changed from ========== Update hover states when dragging the mouse into an element Now when we move the mouse into an element while pressing the left button, we do not update the hover states of the element. We should update hover states of the element, which is now underneath the mouse cursor no matter the mouse button is pressed or not. BUG=122746 ========== to ========== Update hover states when dragging the mouse into an element Now when we move the mouse into an element while pressing the left button, we do not update the hover states of the element. We should update hover states of the element, which is now underneath the mouse cursor no matter the mouse button is pressed or not. BUG=122746 ==========
lanwei@chromium.org changed reviewers: + nzolghadr@chromium.org
lanwei@chromium.org changed reviewers: + rbyers@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lanwei@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lanwei@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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lanwei@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...
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by lanwei@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...
Patchset #3 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
A quick question. Does this code path affect when we are actually dragging a draggable item? Also now that we set the hover states for some new elements do we need to send the boundary events to them as well? https://codereview.chromium.org/2965603002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2965603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:6532: // no problem in skipping this step. I believe this comment doesn't hold anymore. Does it? Can you update the comment accordingly? https://codereview.chromium.org/2965603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:6544: elements_to_add_to_hover_chain.push_back(curr); Do we need a check like old_hover_element != new_hover_element here? Basically if they are the same we don't want to change any hover and this array should be empty similar to the elements_to_remove_from_chain array?
The CQ bit was checked by lanwei@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_...)
On 2017/07/04 17:51:59, Navid Zolghadr wrote: > A quick question. Does this code path affect when we are actually dragging a > draggable item? > Also now that we set the hover states for some new elements do we need to send > the boundary events to them as well? I tested that this code change won't affect dragging an element, I added a test for this. We already sent boundary events to them, but we did not update the hover states, now we send the boundary events and update the hover. > https://codereview.chromium.org/2965603002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/2965603002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Document.cpp:6532: // no problem in skipping > this step. > I believe this comment doesn't hold anymore. Does it? Can you update the comment > accordingly? > > https://codereview.chromium.org/2965603002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Document.cpp:6544: > elements_to_add_to_hover_chain.push_back(curr); > Do we need a check like old_hover_element != new_hover_element here? Basically > if they are the same we don't want to change any hover and this array should be > empty similar to the elements_to_remove_from_chain array?
https://codereview.chromium.org/2965603002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2965603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:6532: // no problem in skipping this step. On 2017/07/04 17:51:59, Navid Zolghadr wrote: > I believe this comment doesn't hold anymore. Does it? Can you update the comment > accordingly? Done.
lgtm https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/no-hover-when-mouse-drag.html (right): https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/no-hover-when-mouse-drag.html:37: assert_equals(getComputedStyle(downbox).backgroundColor, "rgb(0, 0, 0)"); nit: I assume we expect this to also be true. Right? Can you add this too? assert_equals(getComputedStyle(event.target).backgroundColor, "rgb(255, 255, 0) https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:6547: // mustBeInActiveChain (which makes this not hold). nit: I believe this comment also doesn't hold since it talks about mustBeInActiveChain and you have removed it.
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org
lanwei@chromium.org changed reviewers: - dtapuska@chromium.org
This looks like a reasonable change, thanks! LGTM It's been quite a while now since I looked at the hover/active code so I'm not entirely confident I understand all the implications of the code changes. /cc mustaq who has done a bunch of work in that space more recently and so is more of an expert than myself (but his call if wants to review the CL in detail, I know he's already reviewed the design). Can you add a link in your CL description to your design doc with the details of behavior on other browsers? I _think_ it's reasonable to consider this a bugfix to align with other browsers, but there is some potential compat implications so we should clearly document your reasoning. This whole space is pretty subtle, I'd feel a lot better about it if we had web-platform-tests we could show we were increasing our consistency with other browsers on. I know that's block on Navid's work to automate the pointer events tests first, but I hope we can get there soon :-) https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/update-hover-when-mouse-press.html (right): https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/update-hover-when-mouse-press.html:69: var testHover = async_test('Tests that if we update the hover states when we move the mouse to the element while the left button is pressed.'); nit: typo? "Test that we update..."?
mustaq@chromium.org changed reviewers: + mustaq@chromium.org
Thanks for looking into this cryptic piece of code. I have a few questions mostly around the old code that I need to understand---I think the code (old+new) should be much simpler. Now that you have more tests to cover the expected behavior, we should try a cleaner solution. Let's talk in front of a whiteboard. The big picture is that active vs hover chains can be totally disjoint except for the DOM root, so it should be cleaner to update these two chains separately in this method. The old code is a mess: - partially updates only the active chain in Lines 6475~6498 without touching the hover chain, - then updates the hover chain in Lines 6524~6583 together special cases for active-chain changes. Why can't we have two separate blocks (in two sub-methods really): one for only active-chain update, another for only hover-chain update? Sorry for missing this perspective before, I've never looked into this method end-to-end carefully before. https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:6474: Element* old_active_element = ActiveHoverElement(); This is not your change but this looks odd: all uses of (Set)ActiveHoverElement or active_hover_element_ suggest it is really about active element and not about hover. Please double-check the uses and (if I am correct) rename them to drop "hover". https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:6499: The |if-else| blocks above covers two changes in active chain: when the active element changes from non-null to null, and when it changes from non to non-null. I think part of this block should consider button-drag as well. https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:6536: HeapVector<Member<Element>, 32> elements_to_remove_from_chain; This asymmetric list for removal vs addition give me some worries. Why we remove w/o worrying about active/hover distinction but care about the distinction when adding?
On 2017/07/10 15:40:48, mustaq wrote: > Thanks for looking into this cryptic piece of code. I have a few questions > mostly around the old code that I need to understand---I think the code > (old+new) should be much simpler. Now that you have more tests to cover the > expected behavior, we should try a cleaner solution. Let's talk in front of a > whiteboard. > > The big picture is that active vs hover chains can be totally disjoint except > for the DOM root, so it should be cleaner to update these two chains separately > in this method. The old code is a mess: > - partially updates only the active chain in Lines 6475~6498 without touching > the hover chain, > - then updates the hover chain in Lines 6524~6583 together special cases for > active-chain changes. > > Why can't we have two separate blocks (in two sub-methods really): one for only > active-chain update, another for only hover-chain update? > > Sorry for missing this perspective before, I've never looked into this method > end-to-end carefully before. > > https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Document.cpp:6474: Element* > old_active_element = ActiveHoverElement(); > This is not your change but this looks odd: all uses of (Set)ActiveHoverElement > or active_hover_element_ suggest it is really about active element and not about > hover. Please double-check the uses and (if I am correct) rename them to drop > "hover". > > https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Document.cpp:6499: > The |if-else| blocks above covers two changes in active chain: when the active > element changes from non-null to null, and when it changes from non to non-null. > I think part of this block should consider button-drag as well. > > https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Document.cpp:6536: > HeapVector<Member<Element>, 32> elements_to_remove_from_chain; > This asymmetric list for removal vs addition give me some worries. Why we > remove w/o worrying about active/hover distinction but care about the > distinction when adding? Btw, I forgot to mention before: splitting the code for hover/active chains here moves us closer to our bigger end goal: merging boundary events logic with hover.
Description was changed from ========== Update hover states when dragging the mouse into an element Now when we move the mouse into an element while pressing the left button, we do not update the hover states of the element. We should update hover states of the element, which is now underneath the mouse cursor no matter the mouse button is pressed or not. BUG=122746 ========== to ========== Update hover states when dragging the mouse into an element Now when we move the mouse into an element while pressing the left button, we do not update the hover states of the element. We should update hover states of the element, which is now underneath the mouse cursor no matter the mouse button is pressed or not. The design doc: https://docs.google.com/document/d/1TgqvDg8L_TkEiNjBUaz39FDgxpp5aj9ZdIWeQSanKHY/ BUG=122746 ==========
The CQ bit was checked by lanwei@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 checked by lanwei@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...
Patchset #4 (id:120001) has been deleted
https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/no-hover-when-mouse-drag.html (right): https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/no-hover-when-mouse-drag.html:37: assert_equals(getComputedStyle(downbox).backgroundColor, "rgb(0, 0, 0)"); On 2017/07/07 18:21:00, Navid Zolghadr wrote: > nit: I assume we expect this to also be true. Right? Can you add this too? > > assert_equals(getComputedStyle(event.target).backgroundColor, "rgb(255, 255, 0) Done. https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/update-hover-when-mouse-press.html (right): https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/update-hover-when-mouse-press.html:69: var testHover = async_test('Tests that if we update the hover states when we move the mouse to the element while the left button is pressed.'); On 2017/07/07 20:02:17, Rick Byers wrote: > nit: typo? "Test that we update..."? Done. https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:6474: Element* old_active_element = ActiveHoverElement(); On 2017/07/10 15:40:45, mustaq wrote: > This is not your change but this looks odd: all uses of (Set)ActiveHoverElement > or active_hover_element_ suggest it is really about active element and not about > hover. Please double-check the uses and (if I am correct) rename them to drop > "hover". Done. https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:6536: HeapVector<Member<Element>, 32> elements_to_remove_from_chain; On 2017/07/10 15:40:45, mustaq wrote: > This asymmetric list for removal vs addition give me some worries. Why we > remove w/o worrying about active/hover distinction but care about the > distinction when adding? Done. https://codereview.chromium.org/2965603002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:6547: // mustBeInActiveChain (which makes this not hold). On 2017/07/07 18:21:00, Navid Zolghadr wrote: > nit: I believe this comment also doesn't hold since it talks about > mustBeInActiveChain and you have removed it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Still lgtm. Thanks. The code looks cleaner now having two different functions for the active and hover. It does seem that active state update uses a logic that I believe we are not sure about in every case and it doesn't use that common ancestor mechanism we are using in hover state update. So I'm certainly okay with keeping the behavior the same for active and later come up with reasons and document them on why for example we can change the active in the move cases (only for the initial active chain).
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2965603002/#ps140001 (title: "hover drag")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499979943922440,
"parent_rev": "5c0cdc08290b127c8a3a060dd53b9f2627700bc2", "commit_rev":
"1c014b7ed8bd0c568f7f69b5f0c241f6fd635d2f"}
Message was sent while issue was closed.
Description was changed from ========== Update hover states when dragging the mouse into an element Now when we move the mouse into an element while pressing the left button, we do not update the hover states of the element. We should update hover states of the element, which is now underneath the mouse cursor no matter the mouse button is pressed or not. The design doc: https://docs.google.com/document/d/1TgqvDg8L_TkEiNjBUaz39FDgxpp5aj9ZdIWeQSanKHY/ BUG=122746 ========== to ========== Update hover states when dragging the mouse into an element Now when we move the mouse into an element while pressing the left button, we do not update the hover states of the element. We should update hover states of the element, which is now underneath the mouse cursor no matter the mouse button is pressed or not. The design doc: https://docs.google.com/document/d/1TgqvDg8L_TkEiNjBUaz39FDgxpp5aj9ZdIWeQSanKHY/ BUG=122746 Review-Url: https://codereview.chromium.org/2965603002 Cr-Commit-Position: refs/heads/master@{#486537} Committed: https://chromium.googlesource.com/chromium/src/+/1c014b7ed8bd0c568f7f69b5f0c2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/chromium/src/+/1c014b7ed8bd0c568f7f69b5f0c2... |
