New issue
Advanced search Search tips

Issue 734187 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 417782



Sign in to add a comment

[root layer scrolls] issues with background-attachment: fixed

Project Member Reported by skobes@chromium.org, Jun 16 2017

Issue description

$ content_shell --root-layer-scrolls --enable-prefer-compositing-to-lcd-text http://output.jsbin.com/ximuyov/quiet

Background should be cyan gradient.  Instead it is just white.
 
fixedbg.html
593 bytes View Download
Status: Started (was: Assigned)
Update - fixed backgrounds are also broken in two other ways:

- On low DPI with a fixed background on the document, RLS breaks LCD text by compositing the background separately.

- A fixed background on a <div> scroller has to be repainted when the document scrolls.  (This is true regardless of whether the <div> is composited.)  With RLS enabled, these repaints aren't happening.

I'm looking into all three of these.  The missing background on high DPI is related to the reparenting of CLM::m_backgroundLayer.
Summary: [root layer scrolls] issues with background-attachment: fixed (was: [root layer scrolls] fixed backgrounds do not render in high DPI)
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 25 2017

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

commit a6554021d3f7a2be5d6285f65beb0a84c1a8d594
Author: Steve Kobes <skobes@chromium.org>
Date: Tue Jul 25 18:59:44 2017

Render high DPI fixed root backgrounds with RLS.

This disables CLM::background_layer_ paths, which were used for fixed root
background compositing on high DPI.  With RLS, the main GraphicsLayer serves
this purpose.

Bug:  734187 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I4043650de34017b2e4cc5469964ba912e0f54089
Reviewed-on: https://chromium-review.googlesource.com/578064
Reviewed-by: Stefan Zager <szager@chromium.org>
Commit-Queue: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489392}
[modify] https://crrev.com/a6554021d3f7a2be5d6285f65beb0a84c1a8d594/third_party/WebKit/LayoutTests/FlagExpectations/root-layer-scrolls
[modify] https://crrev.com/a6554021d3f7a2be5d6285f65beb0a84c1a8d594/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
[modify] https://crrev.com/a6554021d3f7a2be5d6285f65beb0a84c1a8d594/third_party/WebKit/Source/core/frame/VisualViewportTest.cpp
[modify] https://crrev.com/a6554021d3f7a2be5d6285f65beb0a84c1a8d594/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/a6554021d3f7a2be5d6285f65beb0a84c1a8d594/third_party/WebKit/Source/core/layout/LayoutView.cpp
[modify] https://crrev.com/a6554021d3f7a2be5d6285f65beb0a84c1a8d594/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/a6554021d3f7a2be5d6285f65beb0a84c1a8d594/third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
[modify] https://crrev.com/a6554021d3f7a2be5d6285f65beb0a84c1a8d594/third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 25 2017

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

commit 38dcd19421bf26743dafa77675110f26d79e4be9
Author: Steve Kobes <skobes@chromium.org>
Date: Tue Jul 25 22:50:09 2017

Invalidate fixed-background objects when the root layer scrolls.

With this patch, RLS correctly renders a fixed background on an element within
the document.  Such a background requires a paint invalidation whenever the
document scrolls.  (This is true even if the element is composited, since all
its layers can move in relation to the viewport.)

This patch also brings us closer to rendering a low DPI fixed background on the
document itself.  But additional changes are still needed for that case.

In the case of a high DPI entirely-fixed document background, we avoid setting
the IsBackgroundAttachmentFixedObject bit, so no invalidation is generated.

Bug:  734187 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I7691ff96ce0c4e86076673ac4c09c2290e547ec2
Reviewed-on: https://chromium-review.googlesource.com/585409
Reviewed-by: Stefan Zager <szager@chromium.org>
Commit-Queue: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489471}
[modify] https://crrev.com/38dcd19421bf26743dafa77675110f26d79e4be9/third_party/WebKit/LayoutTests/FlagExpectations/root-layer-scrolls
[modify] https://crrev.com/38dcd19421bf26743dafa77675110f26d79e4be9/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 10 2017

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

commit d3d4c253e079b7d9a112f4c7fc1bff982a6a1f3f
Author: Steve Kobes <skobes@chromium.org>
Date: Thu Aug 10 20:33:39 2017

Use "fast path" to paint local-attach background in overflow scrollers.

With RLS, this is the common case (Document::InheritHtmlAndBodyElementStyles
converts "scroll" to "local").

The rect passed by ViewPainter is modified to exclude scrollbars with RLS (as it
already did for non-RLS).  This makes the fast path's tile size checks happy.

Bug:  734187 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Id4325633529b86a80e8d4cb384daa4d5830895c1
Reviewed-on: https://chromium-review.googlesource.com/609464
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493525}
[modify] https://crrev.com/d3d4c253e079b7d9a112f4c7fc1bff982a6a1f3f/third_party/WebKit/Source/core/paint/BoxPainterBase.cpp
[modify] https://crrev.com/d3d4c253e079b7d9a112f4c7fc1bff982a6a1f3f/third_party/WebKit/Source/core/paint/ViewPainter.cpp

Cc: pnangunoori@chromium.org
Labels: Needs-Feedback
Tested on Mac 10.12.6, Ubuntu 14.04 and Windows 7 on Chrome #62.0.3182.0 and behavior seems to similar on all the browsers when the flags “--root-layer-scrolls --enable-prefer-compositing-to-lcd-text” enabled or not enabled. Attached the screenshots on different versions.

@skobes -- Could you please confirm whether the behavior seen in the screenshots is intended behavior or not.

Please let us know if we have missed anything.

Thanks in advance.
734187.png
3.0 MB View Download
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 26 2017

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

commit f5fb596736f90c3e00f8f2aa70daea555e8bb664
Author: Steve Kobes <skobes@chromium.org>
Date: Sat Aug 26 00:01:39 2017

Refactor BoxPaintInvalidator::InvalidateScrollingContentsBackgroundIfNeeded.

This method was doing five things with confusing flow:

(1) deciding what type of background invalidation was needed
(2) updating paint invalidation bits on the LayoutBox
(3) issuing the paint invalidation on the GraphicsLayer
(4) setting the PaintLayer::NeedsRepaint() flag
(5) clearing the layer's DisplayItem cache

I have extracted (1) into a separate method and moved (2) into InvalidatePaint,
leaving only (3), (4), and (5).  There is no change in behavior.

Bug:  734187 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I03aa84f27189a997cb02a7a3c5186f94a33c5a34
Reviewed-on: https://chromium-review.googlesource.com/633851
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497601}
[modify] https://crrev.com/f5fb596736f90c3e00f8f2aa70daea555e8bb664/third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp
[modify] https://crrev.com/f5fb596736f90c3e00f8f2aa70daea555e8bb664/third_party/WebKit/Source/core/paint/BoxPaintInvalidator.h

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 30 2017

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

commit 14e906a67f9ae321535d917a486c14a17b7ea3cb
Author: Steve Kobes <skobes@chromium.org>
Date: Wed Aug 30 00:31:29 2017

Fix repaint of scrolling contents layer on layout overflow change.

If a composited scroller with a solid-color background has its layout overflow
rect resized, the scrolling contents layer needs a paint invalidation.

This broke in r497601, which made the scrolling contents layer's invalidation
conditional on BackgroundGeometryDependsOnLayoutOverflowRect.

Bug:  759436 ,  734187 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I6dd689dc6766bf76cc4c6d7a3915e0cd2202cda6
Reviewed-on: https://chromium-review.googlesource.com/639011
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498305}
[modify] https://crrev.com/14e906a67f9ae321535d917a486c14a17b7ea3cb/third_party/WebKit/Source/core/paint/BoxPaintInvalidator.cpp
[modify] https://crrev.com/14e906a67f9ae321535d917a486c14a17b7ea3cb/third_party/WebKit/Source/core/paint/BoxPaintInvalidatorTest.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 1 2017

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

commit 907b59e9ba819af5ef6802159dea8a40d114db40
Author: Steve Kobes <skobes@chromium.org>
Date: Sun Oct 01 03:20:45 2017

Support RLS low DPI fixed root backgrounds.

On low DPI displays, we wish to preserve subpixel antialiasing of text rendered
over fixed-attachment document backgrounds (at the cost of repaints on every
scroll).

To achieve this behavior with root layer scrolling, this patch makes the
following changes to the handling of such backgrounds:

(1) PaintLayer::GetBackgroundPaintLocation reports that the background should
    paint in the scrolling contents layer instead of the main GraphicsLayer.

(2) Document scrolls mark the BackgroundChangedSinceLastPaintInvalidation bit on
    the LayoutView, to force paint invalidation of the scrolling contents layer.

(3) BackgroundImageGeometry calculates the correct DestRect(), to be used in the
    paint op issued by BoxPainterBase to paint the background image.

Additionally, a presubmit check is relaxed to allow ViewPainterTest to examine
the cc::PaintOpBuffer.

Bug:  734187 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ic97750f7a4a9040f999dfaf7d011f826a8e9c2c1
Reviewed-on: https://chromium-review.googlesource.com/609531
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505493}
[modify] https://crrev.com/907b59e9ba819af5ef6802159dea8a40d114db40/third_party/WebKit/PRESUBMIT.py
[modify] https://crrev.com/907b59e9ba819af5ef6802159dea8a40d114db40/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/907b59e9ba819af5ef6802159dea8a40d114db40/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
[modify] https://crrev.com/907b59e9ba819af5ef6802159dea8a40d114db40/third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp
[modify] https://crrev.com/907b59e9ba819af5ef6802159dea8a40d114db40/third_party/WebKit/Source/core/paint/PaintLayer.cpp
[add] https://crrev.com/907b59e9ba819af5ef6802159dea8a40d114db40/third_party/WebKit/Source/core/paint/ViewPainterTest.cpp

Status: Fixed (was: Started)
A few notes about outstanding bugs in composited scroller backgrounds:

 Issue 754094  doesn't affect RLS thanks to r489471.

Issue 769096 doesn't seem to affect RLS.

 Issue 568847  still repros, but doesn't affect RLS.  This is because the LayoutView will end up in LocalFrameView::background_attachment_fixed_objects_, which triggers paint invalidation (again thanks to r489471).
FastMobileScrolling REF also works (i.e. with RLS we match current behavior of ignoring "fixed" on Android - issue 521555).

Sign in to add a comment