blink: Update scrollbar existence after pinch-zoom [chromium/src : main]
Robert Flack (Gerrit)
Robert Flack added 2 comments![]( "Open in Gerrit")
view->LayoutViewport()->DidUpdateVisualViewport();This doesn't fit with the function name, but also in PaintLayerScrollableArea::ShouldSupplyScrollbarsForVisualViewport you return false if it's not the root frame layout viewport. As such, couldn't we update RootFrameViewport::DidUpdateVisualViewport (called on 1133) to notify its layout_viewport_?
UpdateScrollbarProportions();Don't we need to update the proportions when the zoom changes and we early return on line 2842 because we already have scrollbars?
Related details
- Ryo Hashimoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryo Hashimoto (Gerrit)
Ryo Hashimoto added 3 comments![]( "Open in Gerrit")
view->LayoutViewport()->DidUpdateVisualViewport();This doesn't fit with the function name, but also in PaintLayerScrollableArea::ShouldSupplyScrollbarsForVisualViewport you return false if it's not the root frame layout viewport. As such, couldn't we update RootFrameViewport::DidUpdateVisualViewport (called on 1133) to notify its layout_viewport_?
Good idea. Done.
One thing is that the return type of `RootFrameViewport::LayoutViewport()` is ScrollableArea even though its actual type is always PaintLayerScrollableArea.
I made `DidUpdateVisualViewport()` a virtual member of ScrollableArea and override it in RootFrameViewport and PaintLayerScrollableArea.
Another possible option is to modify RootFrameViewport to own PaintLayerScrollableArea instead of ScrollableArea as its member variable.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/root_frame_viewport.h;drc=352511eefac2ec35f01db21d5cae2048c9e112ec;l=214
Please let me know if you prefer the latter idea or any other approach.
Don't we need to update the proportions when the zoom changes and we early return on line 2842 because we already have scrollbars?
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
view->LayoutViewport()->DidUpdateVisualViewport();Ryo HashimotoThis doesn't fit with the function name, but also in PaintLayerScrollableArea::ShouldSupplyScrollbarsForVisualViewport you return false if it's not the root frame layout viewport. As such, couldn't we update RootFrameViewport::DidUpdateVisualViewport (called on 1133) to notify its layout_viewport_?
Good idea. Done.
One thing is that the return type of `RootFrameViewport::LayoutViewport()` is ScrollableArea even though its actual type is always PaintLayerScrollableArea.
I made `DidUpdateVisualViewport()` a virtual member of ScrollableArea and override it in RootFrameViewport and PaintLayerScrollableArea.Another possible option is to modify RootFrameViewport to own PaintLayerScrollableArea instead of ScrollableArea as its member variable.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/root_frame_viewport.h;drc=352511eefac2ec35f01db21d5cae2048c9e112ec;l=214Please let me know if you prefer the latter idea or any other approach.
I guess the only thing I'm confused about is why we still need an additional call to DidUpdateVisualViewport() here, when we already have one on 1133? Isn't this effectively calling it twice?
Related details
- Ryo Hashimoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryo Hashimoto (Gerrit)
Ryo Hashimoto added 1 comment![]( "Open in Gerrit")
view->LayoutViewport()->DidUpdateVisualViewport();Ryo HashimotoThis doesn't fit with the function name, but also in PaintLayerScrollableArea::ShouldSupplyScrollbarsForVisualViewport you return false if it's not the root frame layout viewport. As such, couldn't we update RootFrameViewport::DidUpdateVisualViewport (called on 1133) to notify its layout_viewport_?
Robert FlackGood idea. Done.
One thing is that the return type of `RootFrameViewport::LayoutViewport()` is ScrollableArea even though its actual type is always PaintLayerScrollableArea.
I made `DidUpdateVisualViewport()` a virtual member of ScrollableArea and override it in RootFrameViewport and PaintLayerScrollableArea.Another possible option is to modify RootFrameViewport to own PaintLayerScrollableArea instead of ScrollableArea as its member variable.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/root_frame_viewport.h;drc=352511eefac2ec35f01db21d5cae2048c9e112ec;l=214Please let me know if you prefer the latter idea or any other approach.
I guess the only thing I'm confused about is why we still need an additional call to DidUpdateVisualViewport() here, when we already have one on 1133? Isn't this effectively calling it twice?
My bad. I forgot to remove this call. Now in the latest patch set, visual_viewport.cc is not changed.
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
return visual_viewport.IsActiveViewport() && visual_viewport.Scale() > 1;Is scale > 1 the correct check? What happens if the mobile viewport sets the initial scale greater than 1? E.g. https://output.jsbin.com/qawuyab
What about if the virtual keyboard is showing resulting in the visual viewport now being scrollable without a scale change? E.g. https://output.jsbin.com/mimefaj
If you look at https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/visual_viewport.cc;l=836;drc=f4b1a5f91e14941349a0319b35f71d3efb7350af;bpv=1;bpt=1 that seems to suggest how we could determine whether the visual viewport is currently scrollable.
Related details
- Ryo Hashimoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryo Hashimoto (Gerrit)
Ryo Hashimoto added 2 comments![]( "Open in Gerrit")
PTAL
Made the following three changes:
- ShouldSupplyScrollbarsForVisualViewport() checks if the visual viewport is actually scrollable instead of just checking the scale.
- ComputeScrollbarExistence() does not override `h_mode` and `v_mode` if it's `kAlwaysOff` to be less aggressive. Otherwise scrollbars are shown when not appropriate and it broke some web tests in PS #11.
- Instead of early exiting ScrollbarVisibilityChanged(), introduce a new method ShouldAvoidHidingOverlayScrollbars() to controll behaviors related to scrollbars_hidden_if_overlay_.
return visual_viewport.IsActiveViewport() && visual_viewport.Scale() > 1;Is scale > 1 the correct check? What happens if the mobile viewport sets the initial scale greater than 1? E.g. https://output.jsbin.com/qawuyab
What about if the virtual keyboard is showing resulting in the visual viewport now being scrollable without a scale change? E.g. https://output.jsbin.com/mimefaj
If you look at https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/visual_viewport.cc;l=836;drc=f4b1a5f91e14941349a0319b35f71d3efb7350af;bpv=1;bpt=1 that seems to suggest how we could determine whether the visual viewport is currently scrollable.
Thanks that's a good point.
Updated the function to check if the visual viewport is actually scrollable.
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack voted and added 1 comment![]( "Open in Gerrit")
Votes added by Robert Flack
| Code-Review | +1 |
1 comment
- Ryo Hashimoto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryo Hashimoto (Gerrit)
Ryo Hashimoto voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ryo Hashimoto
| Commit-Queue | +2 |
1 comment
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
blink: Update scrollbar existence after pinch-zoom
Enable scrollbars when the visual viewport is scrollable, and non-custom overlay scrollbar is used.
BUG=459997344
- M third_party/blink/renderer/core/frame/root_frame_viewport.cc
- M third_party/blink/renderer/core/frame/root_frame_viewport.h
- M third_party/blink/renderer/core/paint/compositing/compositing_test.cc
- M third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc
- M third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h
- M third_party/blink/renderer/core/paint/paint_layer_scrollable_area_test.cc
- M third_party/blink/renderer/core/scroll/scrollable_area.cc
- M third_party/blink/renderer/core/scroll/scrollable_area.h
Code-Review: +1 by Robert Flack
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |