MainFrame CustomScrollbars are not drawn on RLS |
|||||||
Issue descriptionVersion: <TOT> OS: <Linux> What steps will reproduce the problem? (1) Open news.google.com with --root-layer-scrolling. (2) You dont see the scrollbars(custom scrollbars). What is the expected output? Custom scrollbars should be painted. What do you see instead? An empty custom scrollbar space white strip is displayed. Please use labels and text to provide additional information.
,
Jun 28 2016
Are all custom scrollbars broken in RLS? What's different about news.google.com? See if you can construct a minimized reproduction.
,
Jul 5 2016
not all the custom scrollbars are broken. Only scrollbars related to main frame is broken. check testcase LayoutTests/scrollbars/custom-scrollbar-thumb-inactive-pseudo.html ; It draws inner divs and iframes scrollbars, but not drawing mainframe scrollbars on root-layer-scrolls.
,
Jul 6 2016
Is it correlated with the use of "overflow: scroll" on the <html> element? This relies on special codepaths to propagate styles to the viewport (see Document::inheritHtmlAndBodyElementStyles and LayoutBlock::allowsOverflowClip). Maybe something in that code needs to become RLS-aware.
,
Jul 11 2016
after removing overflow:scroll attribute, still i see the issue persist. i have modified the sample by removing overflow:scroll attribute. Still the customscrollbars are created but not painted.
,
Jul 11 2016
In that case, ignore comment #5. :)
,
Jul 29 2016
Hi Skobes, On Commenting https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp?rcl=0&l=1131 these two lines issue is fixed. The reason is on root-layer-scrolling GraphicsLayer tree looks like LayoutView #document | Scrolling Layer | OverflowControlsHost | Horizontal, Vertical, Scroll Corner Layers due to this during Frameview::synchronizedPaintRecursively for LayoutView #document recursively paints all the children of the tree, due to which we are able to see the custom scrollbar on --root-layer-scrolling In NonWorking Case, Without Commenting those lines this is how the tree looks like Root Transform Layer | Inner View Port Container Layer | OverflowControlsHost | Horizontal, Vertical, Scroll Corner Layers As LayoutView #document doesnt have Horizontal, Vertical, Scroll Corner Layers as its children it misses to paint them during Frameview::synchronizedPaintRecursively We have to either add the OverflowControlsHost -> Horizontal, Vertical, Scroll Corner Layers to LayoutView#document or check in the visualViewport to trigger the paint calls properly which paints these Horizontal, Vertical, Scroll Corner Layers. Im not sure which path to check, Please share your thoughts to fix this.
,
Jul 29 2016
We attach the FrameView's scrollbars to the visual viewport clip layer (above page scale layer) so that they don't zoom in when the user pinches and always stay in view. We want to maintain that property. skobes@ will know better, but for root layer scrolls I think it should be the PaintLayerScrollableArea's scrollbars, not FrameView that we should be using, right? The code in PaintLayerCompositor::updateOverflowControlsLayers() assumes the scrollbars are coming from FrameView (but I don't know if this code is used for custom scrollbars) so that's probably a place to start poking at. FrameView::synchronizedPaint should call synchronizedPaintRecursively directly on the scrollbar layers registered with the compositor. My guess is your problem is that the scrollbar layers registered in PaintLayerCompositor are not the PLSA's custom scrollbar layers.
,
Jul 29 2016
Root layer scrolling should use the scrollbar layers owned by the LayoutView's CompositedLayerMapping, not the scrollbar layers owned by the PaintLayerCompositor. All of the code for managing scrollbar layers in PaintLayerCompositor should go away after root layer scrolling. I would hope FrameView::synchronizedPaint does not need to be directly aware of the LayoutView's scrollbar layers, but perhaps something special is needed to get them working correctly above the page scale layer.
,
Aug 1 2016
@skobes, i agree that Root Layer scrolling should use the scrollbar layers owned by the LayoutView's CompositedLayerMapping. But these scrollbars are attached to Inner View Port Container Layer which is expected behavior for the normal(platform) scrollbars. Where as in the custom scrollbars these needs to be attached to the rootframeview layer itself. As Custom Scrollbars are scalable scrollbars which scales as the page scales, as they are part of the page. I see that in root layer scrolling if its customscrollbar we shouldnt add these scrollbar layers to "Inner View Port Container Layer". wdyt ? do i make sense ? based on this concept i have upload a patch @ https://codereview.chromium.org/2198853002. PTAL.
,
Aug 1 2016
@bokan: Is it ok / expected for custom scrollbars to violate the properties you mention in #9 (not zooming with pinch / always staying in view)? If yes, then this approach looks good.
,
Aug 2 2016
Hmm...today, custom scrollbars behave like platform scrollbars under pinch zoom, they don't zoom with the content. But, unlike platform scrollbars, they do zoom under browser zoom which is maybe what #11 was referring to. TBH, I'm not sure that any of these choices were deliberate so I'm not outright opposed to making changes. That said, I find it a little strange whenever custom scrollbars behave differently from platform. I don't see why we'd need to change custom scrollbars to zoom with the content if platform scrollbars don't. In general, I think we shouldn't treat them differently based on whether they're custom or not. If we were to make any changes, I would prefer that we remove CSS (browser) zoom from custom scrollbars to make them behave more like platform scrollbars. But authors might be relying on that.
,
Aug 2 2016
IMO, As width and height property is been provided to the custom scrollbar, user can specify his own width and height, we cant restrict them unlike platform scrollbars with constant width and height. These properties are scaled as pages scales, we have maintain this property for custom scrollbar.
,
Aug 2 2016
We're not restricting the width/height though, we're making a choice about whether they should be affected by pinch-zoom or not. The implementation we ship today treats custom and platform scrollbars the same way under pinch-zoom, I haven't seen any bugs filed about that behavior. We used to allow pinch-zoom to affect mainframe scrollbars but we changed to the current model for improved user experience. I see the arguments for making that change applying to both platform and custom scrollbars. Additionally, this would increase implementation complexity since we'd now have (yet another) separate path for custom scrollbars so in the absence of a compelling case I don't think we should change this. To clarify, I think the original bug here is definitely worth fixing. I just don't think that we need to place custom scrollbars in a different location in the layer tree. We should fix the painting code to know where the scrollbars are. More specifically, it sounds like with RLS on we need to make sure we're always dealing with the PaintLayerScrollableArea's scrollbars and not the FrameView's.
,
Aug 4 2016
I have investigated a bit more on this issue, here is my analysis. 1. when we dont have custom scrollbar; CompositedLayerMapping::toggleScrollbarLayerIfNeeded notifies the scrollableAreaScrollbarLayerDidChange for creating scrollbars where we decide to scrollbar either customscrollbar or webScrollbarLayer (createSolidColorScrollbarLayer(SolidColorScrollbarLayer) or createScrollbarLayer(PaintedScrollbarLayer)); As we assumed that we dont have custom scrollbar; PaintedScrollbarLayers(Horizontal & Vertical scrollbar) are created in the chromium desktop case, and these scrollbars are attached to overflowControlsHostLayer and whose parent is InnerViewPortContainingLayer. InnerViewPortContainingLayer | OverflowControlsHostLayer | Horizontal,Vertical Scrollbar Layers. During LayerTreeImpl::UpdateScrollbars; as PaintedScrollbarLayers are created and attached to InnerViewPortContainingLayer; on invoking ScrollbarsFor(scroll_layer_id) method retrives the Horizontal And Vertical PaintedScrollbarLayers and updates them due to which the PaintedScrollbarLayers are painted on --root-layer-scrolls. 2. When we have custom scrollbar; CompositedLayerMapping::toggleScrollbarLayerIfNeeded notifies the scrollableAreaScrollbarLayerDidChange for creating scrollbars, where we create custom scrollbars and return back from there. We dont create any layer in the CC nor attach any layer to layer tree. That is why even tough we attach Horizontal, Vertical, Scrollbar Layers to InnerViewPortContainingLayer via OverflowControlsHostLayer, we are not able to paint these custom scrollbar layers as During LayerTreeImpl::UpdateScrollbars; on invoking ScrollbarsFor(scroll_layer_id) method doesnt retrive any scrollbar layers as we havnt registered any custom scrollbars at cc side. That is why we are not painting Custom Scrollbars on --root-layer-scrolls. Do we need to create a CustomScrollbarLayer in CC side just as paintedScrollbarLayer ? let me know pointers to solve this issue.
,
Aug 4 2016
Custom scrollbars are never painted by cc. A composited custom scrollbar gets a normal cc::Layer painted in Blink, not a PaintedScrollbarLayer.
,
Aug 8 2016
yeah i understood that paintedscrollbarlayer wont be created, i was asking do we need to create a customscrollbarlayer kind off in cc ? do we need to do this explicitly ? means creating a cc::layer and making it painted in blink ?
,
Aug 8 2016
We don't need a new kind of scrollbar layer. Blink already supports composited custom scrollbars on overflow scrollers. The only wrinkle for root layer scrolling is the pinch zoom issue bokan mentioned, which affects the structure of the layer tree. I'm a little confused by the existence of FrameView::synchronizedPaint, because I thought GraphicsLayers were painted by cc through ContentLayerDelegate::paintContents. @chrishtr: Should FrameView::synchronizedPaint be responsible for painting layers that are attached outside of PaintLayerCompositor::m_overflowControlsHostLayer?
,
Aug 10 2016
All GraphicsLayers should be painted through synchronizedPaint. Is it failing in this case?
,
Aug 10 2016
If I understand correctly, FrameView::synchronizedPaint begins at PaintLayerCompositor::rootGraphicsLayer(), but this is not the "true" root due to the rewiring done by VisualViewport::attachToLayerTree. In particular the scrollbar layers are not reachable from the PLC root since they need to sit above the page scale layer, therefore synchronizedPaint also explicitly paints the scrollbar layers. The problem is it is painting the PaintLayerCompositor's scrollbar layers, but in RLS mode it should instead paint the scrollbar layers owned by the LayoutView's CompositedLayerMapping. I am generally hesitant to add RLS awareness to FrameView but I guess it's ok here since synchronizedPaint is part of its lifecycle management duties and not its ScrollableArea duties. On the other hand it would be even better if synchronizedPaint could start at the real root which is VisualViewport::rootGraphicsLayer(). Then it wouldn't need to know about the scrollbar layers at all.
,
Aug 10 2016
PTAL @ https://codereview.chromium.org/2198853002/#ps20001, This starts painting from VisualViewport::rootGraphicsLayer() synchronously.
,
Aug 10 2016
The patch referred to in comment 22 undoes what was fixed in https://codereview.chromium.org/1526093006, i.e. to not paint the inspector overlay, because it is not part of the concept of the document's rendering lifecycle. Re comment 21: you say "in RLS mode it should instead paint the scrollbar layers owned by the LayoutView's CompositedLayerMapping". Are those not below view.compositor()->rootGraphicsLayer()?
,
Aug 10 2016
I think the LayoutView CLM scrollbars are pulled out of the page scale layer in the same manner as the PLC scrollbars, for the same reason, by this block in CLM::updateInternalHierarchy:
if (m_isMainFrameLayoutViewLayer)
bottomLayer = layoutObject()->frame()->page()->frameHost().visualViewport().containerLayer();
,
Aug 10 2016
I see, ok, thanks. Then it seems the code that's already there in FrameView::synchronizedPaint is a good way to continue painting those scrollbars,
even with RLS. i.e. replace code like:
if (GraphicsLayer* layerForHorizontalScrollbar = view.compositor()->layerForHorizontalScrollbar()) {
synchronizedPaintRecursively(layerForHorizontalScrollbar);
}
with something that queries the clm directly for the scroll layers, under the condition that they
are above the root graphics layer of the clm. Alternatively maybe the clm of the LayoutView could
provide an iterator, but that seems overkill.
,
Aug 10 2016
,
Aug 11 2016
I have uploaded patch @https://codereview.chromium.org/1526093006. Current FrameView::synchronizedPaint has code for if (GraphicsLayer* layerForHorizontalScrollbar = view.compositor()->layerForHorizontalScrollbar()) { synchronizedPaintRecursively(layerForHorizontalScrollbar); } which picks the PLC scrollbars. In case of RLS scrollbars are associated with CLM, if CLM has scrollbars, painted those scrollbars.
,
Aug 11 2016
sorry uploaded patch @ https://codereview.chromium.org/2198853002/.
,
Sep 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7dbeb9e5f11913900a9560691a60510376b6dc73 commit 7dbeb9e5f11913900a9560691a60510376b6dc73 Author: sataya.m <sataya.m@samsung.com> Date: Thu Sep 08 05:54:26 2016 Draw main frame custom scrollbars in root layer scrolling mode. Draw main frame custom scrollbars in root layer scrolling mode. On RLS custom scrollbars are picked from CompositedLayerMapping, Non-RLS custom scrollbars are picked from PaintLayerCompositor. BUG= 623853 Review-Url: https://codereview.chromium.org/2198853002 Cr-Commit-Position: refs/heads/master@{#417203} [add] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-expected.png [add] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-expected.txt [add] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display.html [modify] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/Source/core/frame/FrameView.cpp [modify] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/Source/core/frame/RootFrameViewport.cpp [modify] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/Source/core/frame/RootFrameViewport.h
,
Sep 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7dbeb9e5f11913900a9560691a60510376b6dc73 commit 7dbeb9e5f11913900a9560691a60510376b6dc73 Author: sataya.m <sataya.m@samsung.com> Date: Thu Sep 08 05:54:26 2016 Draw main frame custom scrollbars in root layer scrolling mode. Draw main frame custom scrollbars in root layer scrolling mode. On RLS custom scrollbars are picked from CompositedLayerMapping, Non-RLS custom scrollbars are picked from PaintLayerCompositor. BUG= 623853 Review-Url: https://codereview.chromium.org/2198853002 Cr-Commit-Position: refs/heads/master@{#417203} [add] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-expected.png [add] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display-expected.txt [add] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/LayoutTests/scrollbars/custom-scrollbar-display.html [modify] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/Source/core/frame/FrameView.cpp [modify] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/Source/core/frame/RootFrameViewport.cpp [modify] https://crrev.com/7dbeb9e5f11913900a9560691a60510376b6dc73/third_party/WebKit/Source/core/frame/RootFrameViewport.h
,
Sep 19 2016
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by satay...@samsung.com
, Jun 28 2016