This issue maybe related to LayerTreeImpl::UpdateDrawProperties. 

ScrollbarAnimationController::ApplyOpacityToScrollbars calls ScrollbarLayerImplBase::SetOverlayScrollbarLayerOpacityAnimated calls layer_tree_impl()->set_needs_update_draw_properties() Then the next UpdateDrawProperties flash the whole page.

I can see the overlay scrollbar works correctly if I comment out `layer_tree_impl()->set_needs_update_draw_properties()`.

pdr@ could you please give me some help? which part maybe wrong in blink-gen-property-trees.

I'm really not sure what's going on. Can you investigate more about what exactly is happening when the page flashes? Is blink sending cc a blank display list, or is this bug purely in cc? If in cc, are we committing pending->active with a nearly-empty layer list?

Hey Philip, I chatted briefly with Chao about this today. My suspicion is that we're probably calling UpdateDrawProps with an incorrect Effect tree briefly so Chao will try to confirm that.

More generally, are there any tools the paint team or graphics team use to debug visual issues like this? I know in tracing you can record display lists and play them back to see what Blink is spitting out - but it seems frame viewer has been broken for a while so it's a bit tricky to find things like flashes in tracing. Any general tips?

As of  https://crbug.com/842238 , the frame viewer in tracing should be fixed. Does that help?

Awesome! Will give that a try

1. Tried Frame Viewer, DisplayItemList is the same and no white snapshot in Frame Viewer.

2. Tried effect tree. seeing effect_changed is true in first UpdateDrawProperties, tried to change it to false but no help.

3. Tried early return UpdateDrawProperties if UpdateDrawProperties is called from SetOverlayScrollbarLayerOpacityAnimated but not help.

4. I can avoid the flash by comment 
    property_trees->effect_tree.set_needs_update(true); or
    layer_tree_impl()->set_needs_update_draw_properties();
   It seems does not make sense.
   And I only found UpdateDrawProperties using needs_update_draw_properties_.

Any other place we can check?

1 is good investigation but 2-4 sound like guesses. I think we need to find exactly what causes the flash to occur. In cc we have double-buffered tree with the active and pending. Can you print the active tree on every frame? I expect there is a frame with an active tree that looks white (maybe it's empty or is missing layers), then we can debug back to find out how it got that way.

I also tried to log the layer number in LayerTreeHostImpl::ActivateSyncTree and UpdateDrawProperties. No empty layer list saw, but I did not print the layers details.

We found the blink-gen-property-tree is missing transform node, effect node and scroll node for overlay scrollbar. 

pdr@ how can we do that?

Deleted: blink-gen-property-tree.txt 8.2 KB

blink-gen-property-tree.txt 8.2 KB View Download

Deleted: cc-gen-property-tree.txt 12.4 KB

cc-gen-property-tree.txt 12.4 KB View Download

Nice find! Do you know if fixing that will fix the page flash?

With your recent change (https://chromium-review.googlesource.com/c/chromium/src/+/1069493), we should be setting the scrollbar layer state properly. Can you confirm that we are creating the property tree nodes blink-side for the overlay scrollbars? You can do this by adding "showAllPropertyTrees(*this);" as the last line of PrePaint in local_frame_view.cc.

If we have the correct property tree blink-side, the issue must be that we aren't sending it to cc. In local_frame_view.cc::CollectDrawableLayersForLayerListRecursively, we walk the GraphicsLayer tree and create foreign layers. Later, in PaintArtifactCompositor, we walk these foreign layers and create cc-side property tree nodes for each. I wonder if CollectDrawableLayersForLayerListRecursively is skipping (or failing to find) the overlay scrollbar layers and not creating foreign layers for them? Can you see if CollectDrawableLayersForLayerListRecursively is creating a foreign layer for the overlay scrollbar layers?

@pdr, it seems we are not creating transform node, effect node and scroll node on blink side. 

[1:1:0619/114742.029415:ERROR:paint_property_tree_printer.cc(208)] Transform tree:
root 0x1bfa2fd9c170 {"flattensInheritedTransform":false,"backface":"visible","scroll":"0x1bfa2fd043d0"}
  VisualViewport Scale Node 0x1bfa2fd9c2d0 {"parent":"0x1bfa2fd9c170","flattensInheritedTransform":false,"compositorElementId":"(8)"}
    VisualViewport Translate Node 0x1bfa2fd9c430 {"parent":"0x1bfa2fd9c2d0","flattensInheritedTransform":false,"scroll":"0x1bfa2fd04490"}
      PaintOffsetTranslation (LayoutView #document) 0x1bfa2fd9c6f0 {"parent":"0x1bfa2fd9c430"}
        ScrollTranslation (LayoutView #document) 0x1bfa2fd9c850 {"parent":"0x1bfa2fd9c6f0","directCompositingReasons":"rootScroller","scroll":"0x1bfa2fd04550"}

[1:1:0619/114742.030388:ERROR:paint_property_tree_printer.cc(213)] Clip tree:
root 0x1bfa2fd90250 {"localTransformSpace":"0x1bfa2fd9c170","rect":"InfiniteIntRect"}
  OverflowClip (LayoutView #document) 0x1bfa2fd90490 {"parent":"0x1bfa2fd90250","changed":true,"localTransformSpace":"0x1bfa2fd9c6f0","rect":"0,0 1404x635","rectExcludingOverlayScrollbars":"0,0 1404x635"}

[1:1:0619/114742.031131:ERROR:paint_property_tree_printer.cc(218)] Effect tree:

[1:1:0619/114742.031477:ERROR:paint_property_tree_printer.cc(223)] Scroll tree:
root 0x1bfa2fd043d0 {}
  VisualViewport Scroll Node 0x1bfa2fd04490 {"parent":"0x1bfa2fd043d0","containerRect":"0,0 1404x635","contentsRect":"0,0 1404x635","userScrollable":"both","scrollsInnerViewport":"true","maxScrollOffsetAffectedByPageScale":"true","compositorElementId":"(10)"}
    Scroll (LayoutView #document) 0x1bfa2fd04550 {"parent":"0x1bfa2fd04490","containerRect":"0,0 1404x635","contentsRect":"0,0 1404x1320","userScrollable":"both","compositorElementId":"(58)"}

Should I add them in paint_property_tree_builder.cc Needs(Effect, Scroll, Transform)?

Yes - check where the effect node is created in cc's PropertyTreeBuilder (I assume it's parent is the scroller's effect node). In paint_property_tree_builder, we'll need to create a new node in the effect tree if the scroller uses fading overlay scrollbars.

That seems like we are creating the necessary nodes for the overlay scrollbars. Do the overlay scrollbars need to use different nodes than PaintOffsetTranslation and OverflowClip? Looking at your BGPT vs CC tree files in comment #12, it looks like BGPT creates one less clip node but I am not sure that is a bug. It looks like the CC tree just has a duplicated transform and clip. Both BGPT and CC create nodes that should work for the scrollbars.

I do not think the missing transform node is actually a problem any more. Do you agree?


I think we should go back to Comment 9 and find out exactly what is causing the flash by printing the cc active tree (layer list + property trees) on every frame.

An overlay scrollbar needs an effect node IIUC since it fades out by setting opacity. 

Talked with @bokan and I agree that we need an effect node for the scrollbars.

Chao, I think the correct solution here is something along these lines:

Add a method like UsesCompositorEffects to ScrollbarTheme and override it to return true in the AuraOverlay theme and Android themes (android should have the same issue, right?). This bit will tell Blink whether it needs to create Effect and Transform nodes for the scrollbar (scroll node and clip node just for the scrollbar isn't needed)

We then need to check this bit when creating the paint property tree for a scroller and add an effect+transform node. Do this somewhere in FragmentPaintPropertyTreeBuilder::UpdateScrollAndScrollTranslation which calls into ObjectPaintProperties (e.g. UpdateScroll). Not sure exactly where this belongs but if you can get it to work we can figure out the exact location in review.

Finally, we need to make sure these get created on the cc PropertyTree. This happens in PaintArtifactCompositor::Update. I think this should just work if the scroller is a composited layer (which is should be wherever we use these kinds of overlays) since we'll look at the layer and create the property tree nodes as well as their parents. But if you run into issues it'd be a good idea to print out the tree built here and confirm it looks correct.

pdr@ can correct me if I got something wrong.

Just one small thing to add: I think you'll need to add a new effect node to object_paint_properties.h.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ddf2f3c431250185d62e37b7288ac73441b89493

commit ddf2f3c431250185d62e37b7288ac73441b89493
Author: chaopeng <chaopeng@chromium.org>
Date: Sat Jul 07 20:23:07 2018

[blink-gen-property-trees] Add effect node for overlay scrollbar

This issue is caused by:
1. scrollbar layer linked to the page's effect node
2. change scrollbar opacity actually change the opacity in effect node

In this patch, we create effect node when walk thought layout object
with overlay scrollbars (PaintedOverlayScrollbarLayer for Aura and
SolidColorScrollbarLayer for Android).

Bug:  846790 
Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Id4d53154324d142228f46ad308bd2f4ccdcf3153
Reviewed-on: https://chromium-review.googlesource.com/1114127
Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: Jianpeng Chao <chaopeng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573173}
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/frame/root_frame_viewport.cc
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/frame/root_frame_viewport.h
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/frame/visual_viewport.cc
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/frame/visual_viewport.h
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.cc
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.h
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/paint/compositing/compositing_layer_property_updater.cc
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/paint/compositing/compositing_layer_property_updater_test.cc
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/paint/find_properties_needing_update.h
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/paint/object_paint_properties.h
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/platform/graphics/compositor_element_id.cc
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/platform/graphics/compositor_element_id.h
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/platform/scroll/scrollable_area.cc
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/platform/scroll/scrollable_area.h
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/platform/scroll/scrollbar.cc
[modify] https://crrev.com/ddf2f3c431250185d62e37b7288ac73441b89493/third_party/blink/renderer/platform/scroll/scrollbar.h