New issue
Advanced search Search tips

Issue 876266 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug

Blocked on:
issue 836884



Sign in to add a comment

Incorrect positioning during opacity/transform animation in RTL containers

Reported by ganor.i...@gmail.com, Aug 21

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36

Steps to reproduce the problem:
1. Load the page. Note that during the animation, the div is slightly left-offsetted. When the animation ends, the div returns to its right position.
2. You can click the REFRESH button to see this in action again.
3. You can also change the animation name to transform/opacity, it happens with both of them.

What is the expected behavior?
The browser should have consider the scrollbar's position, side and width, and position the #container div properly during the animation.

What went wrong?
Seems like during the animation (of the opacity or the transform properties) in RTL containers, the browser positions #container as if its scrollbar was in the right side of the page. Since it's a RTL page, the scrollbar is on the left side, so things get buggy.

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 68.0.3440.106  Channel: stable
OS Version: 10.0
Flash Version: 

My SO thread about it:
https://stackoverflow.com/q/51885182
(with an another test case)
 
scrollbug.html
20.9 KB View Download
Cc: bokan@chromium.org szager@chromium.org
Components: -Blink>Layout Blink>Animation Blink>Scroll
Thanks for the detailed report and the test case!

bokan: Would you mind triaging this?
Labels: Needs-Bisect
Blockedon: 836884
Cc: pdr@chromium.org
Status: Available (was: Unconfirmed)
The "snap" doesn't happen on my high-DPI laptop, the content is just always covered by the scrollbar. When I run with --force-device-scale-factor=1 it snaps to the correct location at the end of the animation. This indicates an issue with composited scrollers mis-positioning RTL content.

Also of note, this appears to be fixed by --enable-blink-gen-property-trees. Given that's shipping soon I think it's best to wait for that to fix it.
The question is how soon is "soon", because it affects all websites in Hebrew, Arabic and other RTL languages.
Or maybe there is a workaround that can be implemented with HTML/CSS/JS for the current situation
Is this a regression (i.e. is this a new bug that worked correctly in an older version)? The current plan is to ship BGPT in M70 - that's the soonest we'd ship a non-regression fix anyway.

If this did regress, we could justify merging to the current beta branch but even that's not sure at this point.
From my checkings this happens since Chrome 41. I guess something has changed there that causes this behavior.

(I used Browserling to search the oldest version of chrome that have this issue, and it seems to be chrome 41).
Cc: swarnasree.mukkala@chromium.org
Labels: -OS-Windows -Pri-2 -Needs-Bisect hasbisect-per-revision RegressedIn-61 Triaged-ET Target-69 Target-70 M-70 FoundIn-70 Target-68 FoundIn-68 FoundIn-69 OS-Linux Pri-1
Owner: vmp...@chromium.org
Status: Assigned (was: Available)
Able to reproduce issue on reported chrome version 68.0.3497.42 using Ubuntu 17.10. Hence providing bisect information below.
Note: The issue is not reproduced on Windows 10 and Mac 10.13.3

Bisect Info:
================
Good build: 61.0.3141.0
Bad build: 61.0.3142.0

CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/7e6f303110c999354c0d343d40e5ac23be3f81d5..aee539101b28c16fc9e38fe648ab0f5d017c4aab
Suspect-on: https://chromium.googlesource.com/chromium/src/+/aee539101b28c16fc9e38fe648ab0f5d017c4aab

Reviewed-on: https://chromium-review.googlesource.com/538974

vmpstr@: Please confirm the issue and help in re-assigning if it is not related to your change.

Thanks!
Components: -Blink>Animation Blink>Compositing
The only reason animations affect this is that they force compositing temporarily, i.e. the content always has the incorrect position on a high DPI device. Removing animation and adding compositing instead.
Owner: pdr@chromium.org
I agree with flackr@, putting will-change: transform on the container persistently positions it slightly off the right edge, by the width of the scrollbar which is now composited. Also, adding overflow: hidden positions it propertly at the right edge with or without compositing, so it's almost certainly the scrollbar being composited that causes this shift. 

Since this is fixed by BGPT, I would agree with bokan@ that it should be the fix (and it should be coming pretty soon)
Labels: -Pri-1 Pri-2
I hate RTL brokenness like this. I'm happy we should have something shipped in the M70-M71 timeframe. Fixing the underlying issue (not implementing RTL handling in two separate places: blink and cc) is the long-term fix for this and many other related bugs. I don't think we should spend resources for a one-off fix for this sooner (e.g., M69).

Lets leave this as P2 and assigned to me for keeping track of it.
Project Member

Comment 11 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

Status: Fixed (was: Assigned)
Labels: TE-Verified-M71 TE-Verified-71.0.3541.0
Able to reproduce the issue on build without fix #68.0.3440.106 using Linux 17.10 as per comment #0.

Verified the fix on Ubuntu 17.10 as per comment#0 on chrome version #71.0.3541.0.
Attaching screen cast for reference.
Observed that browser considers the scrollbar's position, side and width, and position the #container div properly during the animation.

Adding the verified labels.

Thanks...!!
876266.webm
6.6 MB View Download

Sign in to add a comment