New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 854192 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 836886



Sign in to add a comment

[BlinkGenPropertyTrees] fast/multicol/composited-layer-nested.html is failing

Project Member Reported by pdr@chromium.org, Jun 19 2018

Issue description

This test is failing with blink gen property trees:
third_party/blink/tools/run_web_tests.py --debug --additional-driver-flag=--enable-blink-gen-property-trees fast/multicol/composited-layer-nested.html

The composited layer positions are actually different. We now match Safari so this could be a progression. This needs investigation.
 

Comment 1 by pdr@chromium.org, Jun 19 2018

This test is failing as well, likely for a similar reason:
fast/multicol/vertical-rl/composited-relpos-overlapping-will-change.html
(this test is actually failing though, and does not match Safari)

Comment 2 by pdr@chromium.org, Jun 19 2018

This test failure may also be related:
paint/pagination/composited-paginated-outlined-box.html
Labels: -Pri-3 Pri-2
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Available)
Cc: chrishtr@chromium.org trchen@chromium.org
For fast/multicol/composited-layer-nested.html, there are quirks about the fact that SPv1 doesn't fragment composited and squashed layers. The failure of the test is caused by different methods of handling the quirks.

The test can be simplified as the following:
<div style="columns: 4; height: 100px; column-rule: 1px solid black; border: 1px solid black">
  <div id="outer" style="will-change: transform">
    <div style="height: 100px">B1</div>
    <div style="height: 100px">B2</div>
    <div id="inner" style="will-change: transform">
      <div style="height: 100px">B3</div>
      <div style="height: 100px">B4</div>
    </div>
  </div>
</div>

In pre-BGPT, the result is like:
---------------------
|B1  |    |B3  |    |
|    |    |    |    |
---------------------
 B2        B4


Though we don't fragment "outer", we do place "inner" at its normal place regardless of compositing of "outer", so Block3 and Block4 appears in the third column.

However, PaintPropertyTreeBuilder handles the quicks differently. In the property trees, "inner" and their children are placed at different places. The PaintOffsetTranslation of "inner" is translate(0, 100) because PaintPropertyTreeBuilder now doesn't fragment "outer" as a whole including its descendants regardless of their compositing status.

In BGPT, we fully use the property trees to place the composited layers, so the result is like:
---------------------
|B1  |    |    |    |
|    |    |    |    |
---------------------
 B2


 B3


 B4

We may have 2 choices:
1. Change PaintPropertyTreeBuilder to mimic all the current quirks of Pre-BGPT to pass the test with the existing expectation.
2. Keep the current PaintPropertyTreeBuilder behavior and switch to the new quirks.

I'm inclined to 2 which is simpler. It may break some web pages that are based on the current quirks. I'll get stats to evaluate the impact. Do you know how to do a UMA experiment on cluster-telemetry or other ways to do such stat? (Anyway I could still use my ugly stat CL https://chromium-review.googlesource.com/c/chromium/src/+/864636.)

I think the following example can be another justification of choice 2 which produces "better" result:

<div style="columns: 4; height: 100px; column-rule: 1px solid black; border: 1px solid black">
  <div id="outer" style="will-change: transform">
    <div style="height: 100px">B1</div>
    <div style="height: 100px">B2</div>
    <div id="inner" style="will-change: transform">
      <div style="height: 100px">B3</div>
      <div style="height: 100px">B4</div>
    </div>
    <div style="height: 100px">B5</div>
    <div style="height: 100px">B6</div>
  </div>
</div>

For choice 1 (current behavior of pre-BGPT), the result is like:
-------------
|B1|  |B3|  |
-------------
 B2    B4





 B5

 B6


For choice 2 (current behavior of BGPT), the result is like:
-------------
|B1|  |  |  |
-------------
 B2

 B3

 B4

 B5

 B6

Stat data is available for top 100k desktop sites (https://ct.skia.org/results/cluster-telemetry/tasks/benchmark_runs/wangxianzhu-20180817184053/consolidated_outputs/wangxianzhu-20180817184053.output):

A. Percentage of LayoutObjects that are composited and under multicol: 0.002%
   Percentage of web pages containing A: 0.13%

B. Percentage of LayoutObjects that are composited and under another composited LayoutObject under multicol: 0.00018%
   Percentage of web pages containing B: 0.012%

C. Percentage of LayoutObjects that are actually affected by the different quirk methods: 0.00000014%  (2 out of 17393871)
   Percentage of web pages containing C: 0.001%  (1 out of 96135)

Based on the low numbers, I think we should choose Choice 2.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 21

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

commit 429c5dcef93750aae315c80cdbfaea74d43c965c
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue Aug 21 02:58:05 2018

[BGPT] Fix paint/pagination/composited-paginated-outlined-box.html

For SPv1, we don't fragment a composited or squashed layer, but should
create a single fragment for it under the parent fragment where the
layer's border box starts. Previously we used overflow box to determine
fragments causing the composited/squashed layer to be placed in wrong
parent fragment when the layer has overflows. Now use border box rect
in the case.

Bug:  854192 
Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ib05e8629f423b3a1960de1fc8f3989c42f54766f
Reviewed-on: https://chromium-review.googlesource.com/1180377
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584615}
[modify] https://crrev.com/429c5dcef93750aae315c80cdbfaea74d43c965c/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees
[modify] https://crrev.com/429c5dcef93750aae315c80cdbfaea74d43c965c/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 21

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

commit 380436db198ac23ee1a901f74407a58bf7829c64
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue Aug 21 05:40:46 2018

[BGPT] Rebaseline two multicol tests

- fast/multicol/composited-layer-nested.html
  According to discussions and data in  crbug.com/854192 , we should not
  fragment a composited/squashed layer as a whole no matter if there
  are nested composited layers under it (as current BGPT code does).

- fast/multicol/vertical-rl/composited-relpos-overlapping-will-change.html
  The new result looks more correct (the composited layer is placed
  in the second column where the layer's border box rect starts). Also
  updated the test to show more clearly.

Bug:  854192 
Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ie1ad00980034c6c991fa82d2ee4f675010292cdb
Reviewed-on: https://chromium-review.googlesource.com/1180405
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584661}
[modify] https://crrev.com/380436db198ac23ee1a901f74407a58bf7829c64/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees
[modify] https://crrev.com/380436db198ac23ee1a901f74407a58bf7829c64/third_party/WebKit/LayoutTests/fast/multicol/vertical-rl/composited-relpos-overlapping-will-change.html
[add] https://crrev.com/380436db198ac23ee1a901f74407a58bf7829c64/third_party/WebKit/LayoutTests/flag-specific/enable-blink-gen-property-trees/fast/multicol/composited-layer-nested-expected.png
[add] https://crrev.com/380436db198ac23ee1a901f74407a58bf7829c64/third_party/WebKit/LayoutTests/flag-specific/enable-blink-gen-property-trees/fast/multicol/vertical-rl/composited-relpos-overlapping-will-change-expected.png
[modify] https://crrev.com/380436db198ac23ee1a901f74407a58bf7829c64/third_party/WebKit/LayoutTests/platform/linux/fast/multicol/vertical-rl/composited-relpos-overlapping-will-change-expected.png
[modify] https://crrev.com/380436db198ac23ee1a901f74407a58bf7829c64/third_party/WebKit/LayoutTests/platform/mac/fast/multicol/vertical-rl/composited-relpos-overlapping-will-change-expected.png
[modify] https://crrev.com/380436db198ac23ee1a901f74407a58bf7829c64/third_party/WebKit/LayoutTests/platform/win/fast/multicol/vertical-rl/composited-relpos-overlapping-will-change-expected.png

Status: Fixed (was: Assigned)

Sign in to add a comment