Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2668133003:
Change hover element to scrollbar's parent when hit test contains scrollbar (Closed)
DescriptionChange hover element to scrollbar's parent when hit test contains scrollbar
This issue is caused by remove hit test element in
|Document::updateHoverActiveState| when hit test contains scrollbar. Instead we
should replace the hit test element to be scrollbar's parent when hitting a
scrollbar.
BUG=686678
Review-Url: https://codereview.chromium.org/2668133003
Cr-Commit-Position: refs/heads/master@{#448078}
Committed: https://chromium.googlesource.com/chromium/src/+/4d68b33ea8398044d86aa76353e6f7de6880bad6
Patch Set 1 #Patch Set 2 : add test #
Total comments: 5
Patch Set 3 : bokan comment addressed #
Total comments: 4
Patch Set 4 : add clientwidth check #
Total comments: 1
Patch Set 5 : use non-overlay-scrollbar #
Total comments: 1
Patch Set 6 : bokan comment addressed #
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
Please take a early look at this patch. Thank you.
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by chaopeng@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_...)
Thanks, looks good, just some suggestions. https://codereview.chromium.org/2668133003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2668133003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:6102: innerElementInDocument = You've confirmed that this preserves the hover effect for custom scrollbars? https://codereview.chromium.org/2668133003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2668133003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:10836: // elements below(not the scrollbar's parent) unless the scrollbar is faded out. I'd change this to "except the Element that owns the scrollbar" https://codereview.chromium.org/2668133003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11051: } I think it'll work, but please add to this test case a check for when a scrollbar exists but is disabled due to no overflow. i.e. remove the inner div and (as commented below) make the scroller div overflow: scroll so that removing the inner div will keep the scrollbar but disable it. Hovering over the disabled scrollbar should still apply hover to the div. https://codereview.chromium.org/2668133003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/data/scrollbar-and-element-hover.html (right): https://codereview.chromium.org/2668133003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/data/scrollbar-and-element-hover.html:10: overflow-x: auto; should this be overflow-y? You want a vertical scrollbar, right? It works because adding hidden/scroll/auto on x will implicitly make y auto but I'd make this overflow-y: scroll
Updated. PTAL. Thank you. https://codereview.chromium.org/2668133003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2668133003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:6102: innerElementInDocument = On 2017/02/02 23:29:26, bokan wrote: > You've confirmed that this preserves the hover effect for custom scrollbars? Yes, the customer scrollbar issue is also cause by scrollbar depends on it's parent's hover state. https://codereview.chromium.org/2668133003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2668133003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11069: EXPECT_FALSE(scrollableArea->verticalScrollbar()); I tried to get the disabled scrollbar here. But I got no scrollbar.
https://codereview.chromium.org/2668133003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2668133003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11067: scrollableArea = toLayoutBox(parentDiv->layoutObject())->getScrollableArea(); Do you need this? I wouldn't expect us to create a new ScrollableArea. https://codereview.chromium.org/2668133003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11069: EXPECT_FALSE(scrollableArea->verticalScrollbar()); On 2017/02/03 01:12:05, chaopeng wrote: > I tried to get the disabled scrollbar here. But I got no scrollbar. Interesting. I would expect us to have created a disabled scrollbar. Check that the Element's clientWidth shrunk by the scrollbar width. I expect that in here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai... The LayoutBox::hasOverflowClip returns false in scrollsOverflowY so we don't create a scrollbar. That begs the question, how do we paint the scrollbar...anyway, you can confirm but if the clientWidth shrinks that's enough for me.
Updated. PTAL. Thank you. https://codereview.chromium.org/2668133003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2668133003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11067: scrollableArea = toLayoutBox(parentDiv->layoutObject())->getScrollableArea(); On 2017/02/03 01:51:22, bokan wrote: > Do you need this? I wouldn't expect us to create a new ScrollableArea. I just tried to get scrollbar with this. Removed.
The CQ bit was checked by chaopeng@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/2668133003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2668133003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11072: EXPECT_LE(hitTestResult.innerElement()->clientWidth(), 180); clientWidth should be 165 (180 - 15). This means the disabled scrollbar doesn't show up. Please investigate why and fix. (are overlay scrollbars turned on here perhaps?)
On 2017/02/03 18:24:28, bokan wrote: > https://codereview.chromium.org/2668133003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): > > https://codereview.chromium.org/2668133003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11072: > EXPECT_LE(hitTestResult.innerElement()->clientWidth(), 180); > clientWidth should be 165 (180 - 15). This means the disabled scrollbar doesn't > show up. Please investigate why and fix. (are overlay scrollbars turned on here > perhaps?) Yes, This is a overlay scrollbar mock. I finally found a way to turn it off. It can be width == 165 now.
lgtm % nit https://codereview.chromium.org/2668133003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2668133003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11074: EXPECT_EQ(hitTestResult.innerElement()->clientWidth(), 165); Check instead that the clientWidth is < 180 so that we don't hardcode the scrollbar width.
The CQ bit was checked by chaopeng@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.
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2668133003/#ps100001 (title: "bokan comment addressed")
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": 100001, "attempt_start_ts": 1486158064504570,
"parent_rev": "29a8a4270da13b0093a7d7c2c6f542bc6e9bc11e", "commit_rev":
"4d68b33ea8398044d86aa76353e6f7de6880bad6"}
Message was sent while issue was closed.
Description was changed from ========== Change hover element to scrollbar's parent when hit test contains scrollbar This issue is caused by remove hit test element in |Document::updateHoverActiveState| when hit test contains scrollbar. Instead we should replace the hit test element to be scrollbar's parent when hitting a scrollbar. BUG=686678 ========== to ========== Change hover element to scrollbar's parent when hit test contains scrollbar This issue is caused by remove hit test element in |Document::updateHoverActiveState| when hit test contains scrollbar. Instead we should replace the hit test element to be scrollbar's parent when hitting a scrollbar. BUG=686678 Review-Url: https://codereview.chromium.org/2668133003 Cr-Commit-Position: refs/heads/master@{#448078} Committed: https://chromium.googlesource.com/chromium/src/+/4d68b33ea8398044d86aa76353e6... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4d68b33ea8398044d86aa76353e6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
