New issue
Advanced search Search tips

Issue 853945 link

Starred by 2 users

Issue metadata

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

Blocking:
issue 836890



Sign in to add a comment

[BlinkGenPropertyTrees] fast/block/positioning/vertical-rl/002.html is failing

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

Issue description

This test is failing:
third_party/blink/tools/run_web_tests.py --debug --additional-driver-flag=--enable-blink-gen-property-trees fast/block/positioning/vertical-rl/002.html

It looks like we are getting the vertical-rl scrollbar offset wrong.
 
Labels: -Pri-3 Pri-2
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Available)
I briefly looked into this. I don't know where the bug is, but this may help.

The BlinkGenPropertyTree scroll node is created via FragmentPaintPropertyTreeBuilder::UpdateScrollAndScrollTranslation.

The current scroll node is created via GraphicsLayers which are setup in CompositedLayerMapping::UpdateScrollingLayerGeometry.

Some of the offsets/positions used in these two functions look different to me. In particular the HasFlippedBlocksWritingMode check in FragmentPaintPropertyTreeBuilder.
The following test seems to prove that the bug is related to scrolling:
1. Open the test in content_shell with --enable-blink-gen-property-trees;
2. Shrink the window width until the horizontal scrollbar appears;
3. Try to scroll horizontally.

Expected:
2. The thumb of the horizontal scrollbar is on the right;
3. The contents are scrollable.
Blocking: -836886 836890
Cc: bokan@chromium.org
I just noticed that my WIP CL https://chromium-review.googlesource.com/c/chromium/src/+/1181644 also affects some layout tests associated with bug 836890 (see the unexpected passes in https://test-results.appspot.com/data/layout_results/linux-blink-gen-property-trees/277/layout-test-results/results.html). I suspect what I'm working on may overlap with bokan@'s work on bug 836890.

What I'm trying in https://chromium-review.googlesource.com/c/chromium/src/+/1181644 is to change the coordinate space of scroll paint properties. Currently we issue ScrollTranslation to translate from the container space to the scrolling contents space, but in RTL or flipped block direction, this scrolling contents space is different from the cc's contents space because of scroll origin.

bokan@ are you working on anything related to BGPT scrolling issues about scroll origin and scroll-bar-on-the-right in flipped block direction?
I'm not actively working on it (pdr@ mentioned you were looking into the RTL issues).

Background: 

I added the expectations in https://chromium-review.googlesource.com/c/chromium/src/+/1173441 because I added a new clamp in CC that caused us to apply CC's scroll offset limits in some of these tests. The clamp itself is correct, but because CC's limits are wrong due to the RTL bug the tests were failing.
 Issue 874539  has been merged into this issue.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 31

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

commit d44db9636279ee5238407ea776dbaab82cfd5363
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Fri Aug 31 23:08:16 2018

[PE] Fix LayoutBox Client/Padding/Content boxes with scrollbars (especially vertical-rl)

This CL tries to correct the box model when there are scrollbars,
especially in vertical-rl mode. According to
https://www.w3.org/TR/css-overflow-3/#scrollbar-layout, scrollbars
"should be inserted between the inner border edge and the outer
padding edge".

Changes to the previous code:

- Padding|client box now excludes scrollbars, with the help of
  (Top|Left|Bottom|Right)ScrollbarWidth methods which can get the
  scrollbar widths in physical directions in various writing modes.

- Content box is now based on the new padding box by excluding the
  paddings.

- Layout of contents is now based on the correct box model. In
  vertical-rl mode, layout of contents in blocks direction starts
  from the inner edge of the new content box which has been properly
  adjusted for the scrollbar.

- Now LayoutBox::Location() and Location::PhysicalLocation() in the
  initial scroll state are correct in all writing-modes. Previously
  when they were incorrect in vertical-rl mode and some flex box
  directions, requiring an artificial scroll offset to paint the
  content at correct place.

- With the correct padding box, content box, Location(),
  PhysicalLocation(), we no longer need the band-aid code to create the
  correct painted result.

The changed code is mostly in LegacyLayout code. Some changed code is
in LayoutNG that previously converted correct LayoutNG geometries into
the problematic geometries that were previously expected by
LegacyLayout.

The correct box model is required by blink-gen-property-trees because
we can't band-aid the incorrect results in paint properties after
painting.

Bug:  833167 , 853945 , 858843 , 878809 , 876266 

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_layout_ng;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I41faf1ca0bfb95cb287c72703f08c8bd44e9e752
Reviewed-on: https://chromium-review.googlesource.com/1185901
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588201}
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/WebKit/LayoutTests/external/wpt/2dcontext/scroll/2d.scrollPathIntoView.verticalRL-expected.txt
[add] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/WebKit/LayoutTests/external/wpt/css/cssom-view/cssom-getBoundingClientRect-vertical-rl-ref.html
[add] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/WebKit/LayoutTests/external/wpt/css/cssom-view/cssom-getBoundingClientRect-vertical-rl.html
[add] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/WebKit/LayoutTests/fast/block/positioning/vertical-rl/absolute-in-vertical-rl-iframe-expected.html
[add] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/WebKit/LayoutTests/fast/block/positioning/vertical-rl/absolute-in-vertical-rl-iframe.html
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/README.md
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_block.h
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_block_flow.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_block_flow_line.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_box.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_box.h
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_box_test.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_flexible_box.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_flexible_box.h
[add] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_flexible_box_test.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_view.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/layout_view.h
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/layout/ng/ng_block_node.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/paint/compositing/compositing_layer_property_updater.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
[modify] https://crrev.com/d44db9636279ee5238407ea776dbaab82cfd5363/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 6

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

commit 7b0c04464aec2a3dc0f059d619e8f3552fc035bb
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Thu Sep 06 20:31:00 2018

[BGPT] Let ScrollTranslation translate by scroll position instead of scroll offset

See core/layout/README.md for the definitions of scroll position and
scroll offset.

Previously ScrollTranslation used -LayoutBox::ScrolledContentOffset()
which corresponds scroll offset. This caused trouble in
PropertyTreeManager for blink-gen-property-trees which expects
ScrollTranslation to translate by scroll position (the scroll offset
seen by cc which knows nothing about scroll origin).

Now let ScrollTranslation translate by scroll position (scroll offset
+ scroll origin), so it translates the container's content box space
to the scrolling contents space, and both spaces are originated from
the top-left corner. At places where the code expect the coordinate
space relative to scroll origin, we need to add scroll origin to the
offset to convert to the new space of ScrollTranslation.

Bug:  853945 
Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I9c7d5925a25d42365c8a6fe6951383af8eab127e
Reviewed-on: https://chromium-review.googlesource.com/1181644
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589275}
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees
[add] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/WebKit/LayoutTests/compositing/overflow/nested-vertical-rl-overflow-expected.html
[add] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/WebKit/LayoutTests/compositing/overflow/nested-vertical-rl-overflow.html
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/paint/invalidation/outline/outline-change-vertical-rl-expected.txt
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/frame/visual_viewport.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/frame/visual_viewport_test.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/layout/api/line_layout_item.h
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/layout/layout_box.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/layout/layout_box.h
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/layout/layout_object.h
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/layout/line/inline_box.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/paint/block_painter.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/paint/block_painter_test.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/paint/box_painter.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/paint/compositing/compositing_layer_property_updater.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/paint/object_paint_properties.h
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/paint/paint_property_tree_update_tests.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/core/paint/view_painter.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor_test.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/platform/graphics/compositing/property_tree_manager.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/platform/graphics/paint/display_item_client.h
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/platform/graphics/paint/scroll_paint_property_node.cc
[modify] https://crrev.com/7b0c04464aec2a3dc0f059d619e8f3552fc035bb/third_party/blink/renderer/platform/graphics/paint/scroll_paint_property_node.h

Status: Fixed (was: Assigned)

Sign in to add a comment