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

Issue 250514 link

Starred by 14 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug
RTL

Blocked on:
issue 353111



Sign in to add a comment

RTL document in <iframe> should have left-hand scrollbar

Reported by aharon.l...@gmail.com, Jun 16 2013

Issue description

Version: 27.0.1453.110
OS: Linux, Windows 7

What steps will reproduce the problem?
1. Extract the files in the attached zip.
2. Open iframe-scollbar-outer.html.
3. Check the scrollbar location on the second iframe.

What is the expected output?
The scrollbar should be on the left.

What do you see instead?
The scrollbar is on the right.

This is a copy of https://bugs.webkit.org/show_bug.cgi?id=84026, which is still open.
 
Test case.
iframe-scrollbar.zip
22.1 KB Download
Cc: aharon.l...@gmail.com
 Issue 290536  has been merged into this issue.

Comment 3 by tony@chromium.org, Sep 23 2013

 Issue 290536  has been merged into this issue.
This is basically the same as issue 249860.  It turns out that the code in Blink is different between top-level scrollbars and non-top-level scrollbars, but it really shouldn't be.

Comment 5 by le...@chromium.org, Oct 9 2013

According to the comments on issue 249860, we still want the top-level document's scrollbars on the right. Are you suggesting otherwise?
issue 249860 is about the main window scrollbar. This is about the scrollbar in an iframe. The two are different. IMO, the main window scrollbar should always be on the end side relative to the browser UI direction (for reasons explained there). The scrollbar in an iframe should definitely be on the end side relative to the directionality of the page in the iframe.

Comment 7 by le...@chromium.org, Oct 10 2013

Aharon, that was my understanding as well. I was just trying to clarify eseidel's comments.

Comment 8 by tony@chromium.org, Oct 14 2013

Cc: tony@chromium.org eseidel@chromium.org
 Issue 307185  has been merged into this issue.

Comment 9 by js...@chromium.org, Oct 14 2013

 Issue 307185  has been merged into this issue.

Comment 10 by js...@chromium.org, Oct 14 2013

Hope copying the contents of the webkit bug (even though it's a click away) here may help a bit...

Migrated from WebKit Bugzilla: https://bugs.webkit.org/show_bug.cgi?id=84026
Originally reported 2012-04-16 05:43 PST by Aharon (Vladimir) Lanin (aharon@google.com).


Description:
This is a continuation of   bug 54623   [https://bugs.webkit.org/show_bug.cgi?id=54623], for the case of scrollbar on an <iframe>.

Currently, it is always on the right, even if both the <iframe> and the document inside it have direction:rtl. For an <iframe>, the scrollbar should be on the start side of the document inside it (even though that is *not* the case when the same document is displayed in a separate tab, not an <iframe>). If the thing inside the <iframe> does not have a direction, e.g. it is an image, it should be on the start side of the <iframe>'s direction. (If the latter requirement is very difficult to achieve, though, it can be relaxed.)

As usual, behavior on Mac OS X is a special case.


Blocks:
[http://wkb.ug/50910] NEW - Master bug for HTML5 Bidi

Attachments:
2012-04-16 05:45 PST: Test case. Extract the files in the zip and open iframe-scollbar-outer.html. [https://bugs.webkit.org/attachment.cgi?id=137328&action=prettypatch]
2012-04-19 21:47 PST: A test result on IE9 [https://bugs.webkit.org/attachment.cgi?id=138040&action=prettypatch]



Comments:
================================

Comment #1
Posted on 2012-04-16 05:45:54 PST by Aharon (Vladimir) Lanin (aharon@google.com)

Created an attachment (id=137328) [https://bugs.webkit.org/attachment.cgi?id=137328] [details] [https://bugs.webkit.org/attachment.cgi?id=137328&action=edit]
Test case. Extract the files in the zip and open iframe-scollbar-outer.html.

================================

Comment #2
Posted on 2012-04-16 14:07:04 PST by Eric Seidel (eric@webkit.org)

Thanks!

================================

Comment #3
Posted on 2012-04-19 21:47:25 PST by Hironori Bono (hbono@chromium.org)

Created an attachment (id=138040) [https://bugs.webkit.org/attachment.cgi?id=138040] [details] [https://bugs.webkit.org/attachment.cgi?id=138040&action=edit]
A test result on IE9

Greetings Aharon,

Thanks for your bug report.
When I open your test file "iframe-scrollbar-outer.html" with IE9, it renders a scrollbar to the left side of its third iframe element, i.e. "<iframe src="iframe-scrollbar-inner.png"> as shown in the attached picture. Is it your expected one? (It seems IE9 does not allow an inner element of an <iframe> element to inherit the direction property from it.) If it is acceptable for you, it is not so hard to implement its fix. (As far as I have quickly investigated, WebKit does not allow it as well as IE9. Changing this behavior may need a discussion.)

Regards,

Hironori Bono

================================

Comment #4
Posted on 2012-04-22 06:16:21 PST by Aharon (Vladimir) Lanin (aharon@google.com)

(In reply to comment #3 [https://bugs.webkit.org/show_bug.cgi?id=84026#c3])
> Created an attachment (id=138040) [https://bugs.webkit.org/attachment.cgi?id=138040] [details] [https://bugs.webkit.org/attachment.cgi?id=138040&action=edit] [details]
> A test result on IE9
> 
> Greetings Aharon,
> 
> Thanks for your bug report.
> When I open your test file "iframe-scrollbar-outer.html" with IE9, it renders a scrollbar to the left side of its third iframe element, i.e. "<iframe src="iframe-scrollbar-inner.png"> as shown in the attached picture.

The picture you attached shows the third iframe with the scrollbar on the right, not the left. It is the same result that I get.

> Is it your expected one?

No, I want it to be on the left.

> (It seems IE9 does not allow an inner element of an <iframe> element to inherit the direction property from it.)

I do not want an HTML document in an <iframe> to inherit the direction of the iframe. I only think that it might be nice to have thaact happen for an image or other content types that can not specify their own direction. And even there, it is not a very important requirement. It's the first iframes in the test case that are important.
Cc: jchaffraix@chromium.org
I will be working on this issues. Can not assign it to me though.

Comment 13 by tony@chromium.org, Oct 30 2013

See issue 249860.  The code change will probably be in the same place in RenderView.
Owner: a1.go...@samsung.com
Status: Started
We were having our bi-weekly bidi meeting, and just wanted to inquire about the status of this bug (and  issue 250514 , but we can discuss that one separately).

Thanks!
Ok, restarting here, since my old patch has bitrotted by some.

Basically, ScrollView has a shouldPlaceVerticalScrollbarOnLeft method which just return false. Plumb that to what actually tells the text direction should fix the problem.
Project Member

Comment 17 by bugdroid1@chromium.org, Mar 14 2014

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=169180

------------------------------------------------------------------
r169180 | a1.gomes@sisa.samsung.com | 2014-03-14T00:00:26.621838Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/TestExpectations?r1=169180&r2=169179&pathrev=169180
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/frame/FrameView.cpp?r1=169180&r2=169179&pathrev=169180
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/frame/FrameView.h?r1=169180&r2=169179&pathrev=169180

Place inner frame scrollbars on the left side, if indicated by the frame-level
text direction.

CL overrides ScrollView::shouldPlaceVerticalScrollbarOnLeft() from FrameView in
order to determine if a Frame level scrollbar should be placed on the left.
Logic is simple: it obeys what ever is set to the body node, as it determines
the page text direction.

For now, it only covers inner frames, as main frame level scrollbar placement
logic should be dictated by the application/system (see crbug.com/249860).

Patch does not introduce new tests, but marks the existing affected tests as
NeedsManualRebaseline in order to get
results for all platforms from the bots, and perform rebaselines accordingly.
Once this is done, idea is to re-mark them as flaky.

Bug= 250514 

Review URL: https://codereview.chromium.org/150733002
-----------------------------------------------------------------
The layout is not proper on Linux and windows for this page with the above patch
http://jsfiddle.net/26LHv/1/
Yes, it is quite broken! The problem is that the part of the iframe displaying the content needs to be offset to the right by the width of the scrollbar, since the scrollbar is on the left.


Blockedon: chromium:353111
Agreed. Filed  bug 353111 . Will get to it today.
Project Member

Comment 21 by bugdroid1@chromium.org, May 27 2014

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=174894

------------------------------------------------------------------
r174894 | a1.gomes@sisa.samsung.com | 2014-05-27T20:47:05.419600Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/win/compositing/rtl/rtl-iframe-fixed-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-mountainlion/compositing/rtl/rtl-iframe-absolute-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/win/virtual/softwarecompositing/rtl/rtl-iframe-fixed-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/compositing/rtl/rtl-iframe-fixed-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/virtual/softwarecompositing/rtl/rtl-iframe-fixed-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/virtual/softwarecompositing/rtl/rtl-iframe-fixed-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/win/compositing/rtl/rtl-iframe-fixed-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/linux/compositing/rtl/rtl-iframe-fixed-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/virtual/softwarecompositing/rtl/rtl-iframe-fixed-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/virtual/softwarecompositing/rtl/rtl-iframe-absolute-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/win/compositing/rtl/rtl-iframe-absolute-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/compositing/rtl/rtl-iframe-absolute-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/virtual/softwarecompositing/rtl/rtl-iframe-absolute-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/virtual/softwarecompositing/rtl/rtl-iframe-absolute-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/compositing/rtl/rtl-iframe-fixed-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/virtual/softwarecompositing/rtl/rtl-iframe-fixed-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-retina/compositing/rtl/rtl-iframe-absolute-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/compositing/rtl/rtl-iframe-fixed-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/win-xp/virtual/softwarecompositing/rtl/rtl-iframe-absolute-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/win/virtual/softwarecompositing/rtl/rtl-iframe-fixed-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/compositing/rtl/rtl-iframe-fixed-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/TestExpectations?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-snowleopard/compositing/rtl/rtl-iframe-absolute-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/win/virtual/softwarecompositing/rtl/rtl-iframe-absolute-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-mountainlion/compositing/rtl/rtl-iframe-fixed-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/compositing/rtl/rtl-iframe-absolute-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/linux/virtual/softwarecompositing/rtl/rtl-iframe-absolute-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-retina/compositing/rtl/rtl-iframe-fixed-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/win/compositing/rtl/rtl-iframe-absolute-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/win/virtual/softwarecompositing/rtl/rtl-iframe-absolute-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/compositing/rtl/rtl-iframe-absolute-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac/virtual/softwarecompositing/rtl/rtl-iframe-absolute-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/linux-x86/compositing/rtl/rtl-iframe-fixed-overflow-scrolled-expected.txt?r1=174894&r2=174893&pathrev=174894
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/mac-mountainlion/compositing/rtl/rtl-iframe-absolute-overflow-expected.txt?r1=174894&r2=174893&pathrev=174894

Autorebaseline for r169180.

http://src.chromium.org/viewvc/blink?view=revision&revision=169180

TBR=ojan@chromium.org
BUG= 250514 

Review URL: https://codereview.chromium.org/302673002
-----------------------------------------------------------------
What's the status of this?
Status: Fixed
This bug itself it fixed, although I am finishing the patch that fixes the regression it caused (see  bug 353111 , finishing the layout tests now).

I prefer to leave this one opened 'till the regression is fixed, but maybe it was not needed.
Project Member

Comment 24 by bugdroid1@chromium.org, Jul 7 2014

Labels: merge-merged-2062
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=177601

------------------------------------------------------------------
r177601 | a1.gomes@sisa.samsung.com | 2014-07-07T18:55:54.345225Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/branches/chromium/2062/Source/core/frame/FrameView.h?r1=177601&r2=177600&pathrev=177601
   M http://src.chromium.org/viewvc/blink/branches/chromium/2062/LayoutTests/TestExpectations?r1=177601&r2=177600&pathrev=177601
   M http://src.chromium.org/viewvc/blink/branches/chromium/2062/Source/core/frame/FrameView.cpp?r1=177601&r2=177600&pathrev=177601

Revert 169180 "Place inner frame scrollbars on the left side, if..."

Patch cause layout problems as described in
http://code.google.com/p/chromium/issues/detail?id=353111

> Place inner frame scrollbars on the left side, if indicated by the frame-level
> text direction.
> 
> CL overrides ScrollView::shouldPlaceVerticalScrollbarOnLeft() from FrameView in
> order to determine if a Frame level scrollbar should be placed on the left.
> Logic is simple: it obeys what ever is set to the body node, as it determines
> the page text direction.
> 
> For now, it only covers inner frames, as main frame level scrollbar placement
> logic should be dictated by the application/system (see crbug.com/249860).
> 
> Patch does not introduce new tests, but marks the existing affected tests as
> NeedsManualRebaseline in order to get
> results for all platforms from the bots, and perform rebaselines accordingly.
> Once this is done, idea is to re-mark them as flaky.
> 
> Bug= 250514 
> 
> Review URL: https://codereview.chromium.org/150733002

TBR=a1.gomes@sisa.samsung.com

Review URL: https://codereview.chromium.org/370353003
-----------------------------------------------------------------
Labels: -merge-merged-2062
Status: Started
Reverted on trunk by https://codereview.chromium.org/384763002 and on M37 by https://codereview.chromium.org/370353003 .

M36 revert pending: https://codereview.chromium.org/377653003/

Comment 26 by phistuck@gmail.com, Jul 17 2014

Perhaps you should ping someone?
I am on a e-mail loop with the two release managers (Alex/Matthew). Hope will hear back from them today.
Project Member

Comment 28 by bugdroid1@chromium.org, Jul 22 2014

Labels: merge-merged-1985
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=178689

------------------------------------------------------------------
r178689 | a1.gomes@sisa.samsung.com | 2014-07-22T20:06:56.476995Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/branches/chromium/1985/Source/core/frame/FrameView.cpp?r1=178689&r2=178688&pathrev=178689
   M http://src.chromium.org/viewvc/blink/branches/chromium/1985/Source/core/frame/FrameView.h?r1=178689&r2=178688&pathrev=178689
   M http://src.chromium.org/viewvc/blink/branches/chromium/1985/LayoutTests/TestExpectations?r1=178689&r2=178688&pathrev=178689

Revert r169180 (https://codereview.chromium.org/150733002) in M36.1985 branch, due to layout problems on overflown RTL contents.

Content is cropped and shifts to the left in RTL iFrames with scrollbars

R=matthewyuan@chromium.org, jchaffraix@chromium.org
BUG= 353111 

> Place inner frame scrollbars on the left side, if indicated by the frame-level
> text direction.
>
> CL overrides ScrollView::shouldPlaceVerticalScrollbarOnLeft() from FrameView in
> order to determine if a Frame level scrollbar should be placed on the left.
> Logic is simple: it obeys what ever is set to the body node, as it determines
> the page text direction.
>
> For now, it only covers inner frames, as main frame level scrollbar placement
> logic should be dictated by the application/system (see crbug.com/249860).
>
> Patch does not introduce new tests, but marks the existing affected tests as
> NeedsManualRebaseline in order to get
> results for all platforms from the bots, and perform rebaselines accordingly.
> Once this is done, idea is to re-mark them as flaky.
>
> Bug= 250514 
>
> Review URL: https://codereview.chromium.org/150733002

Review URL: https://codereview.chromium.org/408143007
-----------------------------------------------------------------
Owner: toniki...@chromium.org

Comment 30 by tkent@chromium.org, May 23 2016

Components: -Blink>RTL Blink>Layout Blink>Scroll
Labels: rtl
Cc: bokan@chromium.org
Labels: -Pri-2 -merge-merged-1985 Merge-merged-1985 Pri-3
This is still an issue we'd like to see fixed. tonikitoo@ any updates? I'm also trying to push on issue 249860 to get some finality from UX folks there.
Cc: szager@chromium.org schenney@chromium.org vamshi.kommuri@chromium.org
 Issue 852830  has been merged into this issue.
Owner: ----
Status: Available (was: Started)
Project Member

Comment 34 by bugdroid1@chromium.org, Jul 25

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

commit e0a4d9cf005bf241cd06a6777196e12673c2eeee
Author: Stefan Zager <szager@chromium.org>
Date: Wed Jul 25 14:49:27 2018

Put vertical scrollbar on left in rtl iframes

This is an updated version of this previously-reverted patch:

https://codereview.chromium.org/150733002/

... along with some fixes to the treatment of left-hand scrollbars in the layout code (the need for
which was revealed by test failures).

BUG= 250514 
R=skobes@chromium.org,eae@chromium.org

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Icd2aa1b4d40aad1272c392366fdbde81a6830478
Reviewed-on: https://chromium-review.googlesource.com/1141332
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577885}
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/WebKit/LayoutTests/compositing/rtl/rtl-iframe-absolute-overflow-expected.png
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/WebKit/LayoutTests/compositing/rtl/rtl-iframe-absolute-overflow-expected.txt
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/WebKit/LayoutTests/compositing/rtl/rtl-iframe-absolute-overflow-scrolled-expected.png
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/WebKit/LayoutTests/compositing/rtl/rtl-iframe-absolute-overflow-scrolled-expected.txt
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/WebKit/LayoutTests/compositing/rtl/rtl-iframe-fixed-overflow-expected.png
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/WebKit/LayoutTests/compositing/rtl/rtl-iframe-fixed-overflow-expected.txt
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/WebKit/LayoutTests/compositing/rtl/rtl-iframe-fixed-overflow-scrolled-expected.png
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/WebKit/LayoutTests/compositing/rtl/rtl-iframe-fixed-overflow-scrolled-expected.txt
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/blink/renderer/core/layout/layout_box.cc
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/blink/renderer/core/layout/layout_view.cc
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/blink/renderer/core/layout/layout_view.h
[modify] https://crrev.com/e0a4d9cf005bf241cd06a6777196e12673c2eeee/third_party/blink/renderer/core/paint/compositing/composited_layer_mapping.cc

Owner: szager@chromium.org
Status: Fixed (was: Available)
Labels: -Pri-3 Merge-Request-69 Pri-2
Can the change in comment#34 be merged to M69? Thanks.
Project Member

Comment 37 by sheriffbot@chromium.org, Aug 2

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Is this M69 regression? Seems like this is very old bug reported back in 2013.
@govind: This change resolves a problem with the change from issue #832569, which made the cut for M69.
How safe is the change to merge to M69 which is already in Beta?
#34 - it would be great if you also made sure http://jsfiddle.net/0b5nptt5/ is fixed.
I'm not sure about merging this to M69. This patch is likely to have a broader impact compared to the fix for issue #832569 -- that patch addressed a fairly narrow corner case, but this patch affects every RTL iframe that scrolls.

The code in the patch is safe enough; but it's possible that the change in behavior will be surprising to some web developers. Ideally, I'd like to get an opinion from someone who knows more about the RTL web.
Cc: js...@chromium.org
Thank you szager@.

+jshin@ (i18n Eng) in case if he can help, ptal comment #42.

Pls note M69 being such an important release, we can't take any risk at all.I'm also leaning towards rejecting the merge based on comment #42.
Cc: e...@chromium.org
+eae@ could you ptal comment #42 & #43. Thank you.
I believe most browsers already do this, so this should probably happen at some point.
I have written feature detection code for the scroll bar position once (put an absolute span, check whether it is at 0, 0 or something). However, whether this has been a bad interoperability or display issue that affects a lot of websites (or bothers a lot of Israeli designers) - I do not think I have heard such complaints in the last decade I worked with the web in Israel.
Agreed, I would be very reluctant to merge this into M69.
Labels: -Merge-Review-69 Merge-Rejected-69
Thank you eae@. 
Rejecting merge to M69 based on comment #42 and #46.
Thanks for reviewing the merge request.
Hi, this seems to have broken again in latest master. The test case in #1 shows the broken state again.
Running the test case in comment #1, I see that the iframe with dir=rtl has the scrollbar on the left (as expected); but the iframe whose src is a png file has the scrollbar on the right. Is that what you're seeing as well?

I doubt the png case ever worked; I'm pretty sure my fix only addressed the case with dir=rtl in the iframe. I need to do a bit of digging into the spec and make sure what the correct behavior is.
You're right, I must have been on some version that didn't have the fix. All seems good, thanks!

Sign in to add a comment