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

commit a12e7a002f85301bf013b45e6e8bb61455304a76
Author: flackr <flackr@chromium.org>
Date: Tue Sep 13 21:29:55 2016

Disable local background equivalence when we have a box shadow due to painting bug.

BUG= 646464 , 646341 

Review-Url: https://codereview.chromium.org/2337273002
Cr-Commit-Position: refs/heads/master@{#418374}

[modify] https://crrev.com/a12e7a002f85301bf013b45e6e8bb61455304a76/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp

Cannot reproduce with Linux 57.0.2945.0 (64-bit):

1. Reverted a12e7a002f85301bf013b45e6e8bb61455304a76

2. Opened http://output.jsbin.com/vucefi (NOTE: sub-pixel anti-aliasing is getting disabled in the iframe in http://jsbin.com/vucefi/edit?html,css,output, so you have to use the pure output version).

3. Confirmed that the first box is in a composited layer, has a shadow, and has sub-pixel anti-aliased text.


I believe we can just revert flackr's override, this must have been fixed between M55 and ToT.

Xianzhu landed a patch in the last day that might have been the reason you can't repro?

I think #1 has worked around the bug. This is also why changing box-shadow also changes background painting layer and triggered  bug 672149 .

re #5: Do you know which patch you are referring to? The most recent patch my checkout has from Xianzhu is d4803657efd7d006ff8bf6f17091fb45e2e8ef34, which doesn't seem like it would be related.

re $6 : #1 was reverted for the (failed) reproduction in #4


It appears that reverting flackr's CL does cause test failures (see https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/352714), but unclear atm if this means the revert is bad or it means those tests need updated.

> re #5: Do you know which patch you are referring to? The most recent patch my checkout has from Xianzhu is d4803657efd7d006ff8bf6f17091fb45e2e8ef34, which doesn't seem like it would be related.

It's https://codereview.chromium.org/2559863002.

> re $6 : #1 was reverted for the (failed) reproduction in #4

Sorry I overlooked that. Can you check which layer the background is painting on (see CompositedLayerMapping::updateBackgroundPaintsOntoScrollingContentsLayer)? If it's painting on the main layer with #1 CL reverted, there might be some other reason (border?) causing that, and we need to remove the reason from the test case to reproduce the issue.

> It appears that reverting flackr's CL does cause test failures

I think this is because the backgrounds are painted onto different layers with the CL reverted, causing change of the opaqueness of the some layer. This also means that some backgrounds are painted onto wrong layer causing this bug.

> Can you check which layer the background is painting on (see CompositedLayerMapping::updateBackgroundPaintsOntoScrollingContentsLayer)? If it's painting on the main layer with #1 CL reverted, there might be some other reason (border?) causing that, and we need to remove the reason from the test case to reproduce the issue.

Log line is from below diff. (Test: http://output.jsbin.com/vazeces) Looks like it is painting onto the scrolling contents.

updateBackgroundPaintsOntoScrollingContentsLayer() for "LayoutBlockFlow DIV id='scroller1' class='scroller'": m_backgroundPaintsOntoScrollingContentsLayer = 1, m_backgroundPaintsOntoGraphicsLayer = 0
[1:1:1209/141503.645677:INFO:CompositedLayerMapping.cpp(386)] updateBackgroundPaintsOntoScrollingContentsLayer() for "LayoutBlockFlow DIV id='scroller2' class='scroller'": m_backgroundPaintsOntoScrollingContentsLayer = 0, m_backgroundPaintsOntoGraphicsLayer = 1

And the text is LCD AA.



Without flackr's change reverted, this logs:

[1:1:1209/141112.883918:INFO:CompositedLayerMapping.cpp(386)] updateBackgroundPaintsOntoScrollingContentsLayer() for "LayoutBlockFlow DIV id='scroller1' class='scroller'": m_backgroundPaintsOntoScrollingContentsLayer = 0, m_backgroundPaintsOntoGraphicsLayer = 1
[1:1:1209/141209.483333:INFO:CompositedLayerMapping.cpp(386)] updateBackgroundPaintsOntoScrollingContentsLayer() for "LayoutBlockFlow DIV id='scroller2' class='scroller'": m_backgroundPaintsOntoScrollingContentsLayer = 0, m_backgroundPaintsOntoGraphicsLayer = 1

And there is obviously no LCD AA text.



Logging diff:

smcgruer@stiglet2:~/chromium/src$ git diff
diff --git a/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp b/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
index 7f3fb6d..7bc8c8b 100644
--- a/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
+++ b/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
@@ -382,6 +382,13 @@ void CompositedLayerMapping::
   m_backgroundPaintsOntoGraphicsLayer =
       !m_backgroundPaintsOntoScrollingContentsLayer ||
       paintLocation & BackgroundPaintInGraphicsLayer;
+
+  LOG(INFO) << "updateBackgroundPaintsOntoScrollingContentsLayer() for "
+            << m_graphicsLayer->debugName().ascii() << ": "
+            << "m_backgroundPaintsOntoScrollingContentsLayer = "
+            << m_backgroundPaintsOntoScrollingContentsLayer
+            << ", m_backgroundPaintsOntoGraphicsLayer = "
+            << m_backgroundPaintsOntoGraphicsLayer;
 }

It seems that we can already paint box-shadow and background onto different layers, so we can revert the #1 workaround.

flackr@ do you know which CL achieved that?

We have always painted the box-shadow, borders and other decorations into the graphics layer. The only thing that I have moved to the scrolling contents layer is the painting of the actual background.

Why did we need the #1 workaround?

I rolled back to 55.0.2859.0, and can definitely reproduce (see attached image). So there was a need for the #1 workaround.

Why and when this workaround was no longer needed, I don't know, but I'm not sure it's worth the bisecting effort (would need to patch in some sort of revert of #1 every bisect).

Deleted: Screenshot from 2016-12-12 09:22:16.png 69.8 KB

	Screenshot from 2016-12-12 09:22:16.png 69.8 KB View Download

FYI did some manual bisecting this morning. Problem was fixed between 2909 and 2910, some more bisecting on that range gave me 763c0f72578867e14acc7e8bb1e8f47e78fc9ad7..3ea42b21339ebf028e0ff6e368ca60d463386dd2

From that list, eac477641ec5fdf9e3b9ec52d46fc3a908e8503e jumped out as a likely fix, I suspect wangxianzhu@ should be able to confirm:

commit eac477641ec5fdf9e3b9ec52d46fc3a908e8503e
Author: wangxianzhu <wangxianzhu@chromium.org>
Date:   Thu Nov 3 17:38:19 2016 -0700

    Always paint background and shadow separately
    
    The logic to piggyback shadow on background painting is complex and
    fragile. For the bug, we skip painting background-color which is
    obscured by background-image but we also slip shadow piggybacked on
    painting background. The logic was added in WebKit for some graphics
    library that can draw combined background and shadow faster.
    
    It's confirmed that Skia doesn't benefit from the logic. Local perf
    test (containing hundreds of divs with simple background and shadow)
    showed this CL doesn't affect rasterization performance, and degrade
    record performance by 0% to 40% because of more drawing operations to
    handle. Considering that the test is an extreme example of background+
    shadow which is unlikely to exist in real pages, the performance
    impact of this CL on real pages will be acceptable.
    
    Telemetry-cluster showed no regression on 10k web pages.
    
    Remove the logic to simply code and fix the bug.
    
    BUG= 660187 
    CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
    
    Review-Url: https://codereview.chromium.org/2464053003
    Cr-Commit-Position: refs/heads/master@{#429738}

Thanks smcgruer@ for the investigation. Sorry I almost forgot that I had made this change which affected how background and shadow are painted. Before this change we painted background and shadow together in some cases and if the background was painted on the scrolled contents layer we also painted the shadow on the same layer which was incorrect. After this CL the #1 workaround can be reverted.

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

commit c765a85995920f79d3132e06a2c146316b823c3f
Author: smcgruer <smcgruer@chromium.org>
Date: Tue Dec 13 03:20:43 2016

Revert "Disable local background equivalence when we have a box shadow due to painting bug."

This reverts commit a12e7a002f85301bf013b45e6e8bb61455304a76.

BUG= 646464 

TEST=Visit http://output.jsbin.com/vucefi, confirm that both boxes have
box shadows and the first box has sub-pixel AA text

Review-Url: https://codereview.chromium.org/2561183002
Cr-Commit-Position: refs/heads/master@{#438020}

[modify] https://crrev.com/c765a85995920f79d3132e06a2c146316b823c3f/third_party/WebKit/LayoutTests/compositing/overflow/scrollbar-layer-placement-expected.txt
[delete] https://crrev.com/6140d6c6a50690dbc0c7d30e753104883a9873a0/third_party/WebKit/LayoutTests/platform/android/compositing/overflow/scrollbar-layer-placement-expected.txt
[delete] https://crrev.com/6140d6c6a50690dbc0c7d30e753104883a9873a0/third_party/WebKit/LayoutTests/platform/mac-mac10.10/virtual/prefer_compositing_to_lcd_text/compositing/overflow/scrollbar-layer-placement-expected.txt
[delete] https://crrev.com/6140d6c6a50690dbc0c7d30e753104883a9873a0/third_party/WebKit/LayoutTests/platform/mac-retina/virtual/prefer_compositing_to_lcd_text/compositing/overflow/scrollbar-layer-placement-expected.txt
[delete] https://crrev.com/6140d6c6a50690dbc0c7d30e753104883a9873a0/third_party/WebKit/LayoutTests/platform/win/virtual/prefer_compositing_to_lcd_text/compositing/overflow/scrollbar-layer-placement-expected.txt
[modify] https://crrev.com/c765a85995920f79d3132e06a2c146316b823c3f/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp

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

commit 4baddb7eb1745ba1f39697e89e303805f90efe16
Author: smcgruer <smcgruer@chromium.org>
Date: Thu Dec 15 19:48:37 2016

Revert of Revert "Disable local background equivalence when we have a box shadow due to painting bug." (patchset #5 id:80001 of https://codereview.chromium.org/2561183002/ )

Reason for revert:
Caused a regression in drawing backgrounds when the window was sized around 1280x1024. Regression seen in at least Windows and Linux - see  http://crbug.com/674429 

BUG= 674429 

Original issue's description:
> Revert "Disable local background equivalence when we have a box shadow due to painting bug."
>
> This reverts commit a12e7a002f85301bf013b45e6e8bb61455304a76.
>
> BUG= 646464 
>
> TEST=Visit http://output.jsbin.com/vucefi, confirm that both boxes have
> box shadows and the first box has sub-pixel AA text
>
> Committed: https://crrev.com/c765a85995920f79d3132e06a2c146316b823c3f
> Cr-Commit-Position: refs/heads/master@{#438020}

TBR=wangxianzhu@chromium.org,chrishtr@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 646464 

Review-Url: https://codereview.chromium.org/2576423002
Cr-Commit-Position: refs/heads/master@{#438897}

[modify] https://crrev.com/4baddb7eb1745ba1f39697e89e303805f90efe16/third_party/WebKit/LayoutTests/compositing/overflow/scrollbar-layer-placement-expected.txt
[add] https://crrev.com/4baddb7eb1745ba1f39697e89e303805f90efe16/third_party/WebKit/LayoutTests/platform/android/compositing/overflow/scrollbar-layer-placement-expected.txt
[add] https://crrev.com/4baddb7eb1745ba1f39697e89e303805f90efe16/third_party/WebKit/LayoutTests/platform/mac-mac10.10/virtual/prefer_compositing_to_lcd_text/compositing/overflow/scrollbar-layer-placement-expected.txt
[add] https://crrev.com/4baddb7eb1745ba1f39697e89e303805f90efe16/third_party/WebKit/LayoutTests/platform/mac-retina/virtual/prefer_compositing_to_lcd_text/compositing/overflow/scrollbar-layer-placement-expected.txt
[add] https://crrev.com/4baddb7eb1745ba1f39697e89e303805f90efe16/third_party/WebKit/LayoutTests/platform/win/virtual/prefer_compositing_to_lcd_text/compositing/overflow/scrollbar-layer-placement-expected.txt
[modify] https://crrev.com/4baddb7eb1745ba1f39697e89e303805f90efe16/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp

Sadly the re-enabling CL had to be reverted due to  http://crbug.com/674429 , which caused a paint bug when drawing scroll backgrounds. Specific reproduction steps were:

(1) Launch chrome *size 1282x1024* and navigate to https://support.google.com/chrome/?p=help&ctx=menu#topic=3227046
(2) Scroll at the bottom of the page and click on 'Language drop down list'(Kindly refer the video)
(3) Observe the scroll bar for 'Language drop down list'.

The scrollbar background was not fully painted.

So before we re-enable this, we need to fix whatever caused that :)

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This is working now. It might come back later, but since it's been a year I figure we close it.