Correction to layoutOverflowRect in case of direction:rtl

Correction to layoutOverflowRect calculation in case of direction:rtl

BUG=179332
Webkit bug https://bugs.webkit.org/show_bug.cgi?id=85856

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161333

[pals, 2013-10-01 13:35:40]

PTAL. and could you please run the trybots for this CL? Thanks

[ojan, 2013-10-01 19:24:52]

This patch needs tests.

On 2013/10/01 13:35:40, sanjoyp wrote:
> PTAL. and could you please run the trybots for this CL? Thanks

I think you can run the trybots yourself by clicking the "try more bots" link,
no?

https://codereview.chromium.org/25373006/diff/3001/Source/core/rendering/Rend...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/25373006/diff/3001/Source/core/rendering/Rend...
Source/core/rendering/RenderBox.h:224: LayoutUnit clientLeft() const { return
borderLeft() + (style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ?
verticalScrollbarWidth() : 0); }
This changes the web exposed behavior of clientLeft. Does this not affect
existing test cases? What do other browsers do in this case?

[pals, 2013-10-03 10:54:06]

I tried running trybots using "Try more bots" but it doesn't run the bots
actually. I guess I dont have permissions.

Added a layout test for the change.

https://codereview.chromium.org/25373006/diff/3001/Source/core/rendering/Rend...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/25373006/diff/3001/Source/core/rendering/Rend...
Source/core/rendering/RenderBox.h:224: LayoutUnit clientLeft() const { return
borderLeft() + (style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ?
verticalScrollbarWidth() : 0); }
On 2013/10/01 19:24:52, ojan wrote:
> This changes the web exposed behavior of clientLeft. Does this not affect
> existing test cases? What do other browsers do in this case?

Removed the change from this patch. I shall create separate CL to fix
crbug.com/128500. Yes, this changes the web exposed behavior of clientLeft. It
seems we dont have a test case for clientLeft. Please see comment #4 in the bug
for more details on other browsers behavior.

[pals, 2013-10-07 06:42:46]

On 2013/10/03 10:54:06, sanjoyp wrote:
> I tried running trybots using "Try more bots" but it doesn't run the bots
> actually. I guess I dont have permissions.
> 
> Added a layout test for the change.
> 
>
https://codereview.chromium.org/25373006/diff/3001/Source/core/rendering/Rend...
> File Source/core/rendering/RenderBox.h (right):
> 
>
https://codereview.chromium.org/25373006/diff/3001/Source/core/rendering/Rend...
> Source/core/rendering/RenderBox.h:224: LayoutUnit clientLeft() const { return
> borderLeft() + (style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ?
> verticalScrollbarWidth() : 0); }
> On 2013/10/01 19:24:52, ojan wrote:
> > This changes the web exposed behavior of clientLeft. Does this not affect
> > existing test cases? What do other browsers do in this case?
> 
> Removed the change from this patch. I shall create separate CL to fix
> crbug.com/128500. Yes, this changes the web exposed behavior of clientLeft. It
> seems we dont have a test case for clientLeft. Please see comment #4 in the
bug
> for more details on other browsers behavior.

PTAL

[ojan, 2013-10-16 01:52:55]

Not sure I'm the best reviewer for this. Julien, Eric, Levi, do any of you feel
comfortable reviewing this?

[eseidel, 2013-10-16 02:32:10]

I would need to try the test on my machine to understand what's changing here. 
This is not an area of the code I have much familiarity with.

[pals, 2013-10-28 14:38:51]

Friendly ping.

[Julien - ping for review, 2013-10-30 02:03:41]

The code change looks fine, just one question.

https://codereview.chromium.org/25373006/diff/15001/LayoutTests/scrollbars/rt...
File LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
(right):

https://codereview.chromium.org/25373006/diff/15001/LayoutTests/scrollbars/rt...
LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html:4:
<title>Bug 85856</title>
I don't think this is bug 85856 anymore.

https://codereview.chromium.org/25373006/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/25373006/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1650: x -= verticalScrollbarWidth();
Is this code tested somewhere?

[pals, 2013-10-30 12:02:12]

Fixed the tests. Please review.

https://codereview.chromium.org/25373006/diff/15001/LayoutTests/scrollbars/rt...
File LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
(right):

https://codereview.chromium.org/25373006/diff/15001/LayoutTests/scrollbars/rt...
LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html:4:
<title>Bug 85856</title>
On 2013/10/30 02:03:41, Julien Chaffraix - PST wrote:
> I don't think this is bug 85856 anymore.

Done. Removed.

https://codereview.chromium.org/25373006/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/25373006/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1650: x -= verticalScrollbarWidth();
On 2013/10/30 02:03:41, Julien Chaffraix - PST wrote:
> Is this code tested somewhere?
Are you talking about LayoutTests? I have written a layout test which tests this
code and there are few existing tests need to be rebaselined.

[Julien - ping for review, 2013-11-01 00:29:13]

lgtm

https://codereview.chromium.org/25373006/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/25373006/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:1650: x -= verticalScrollbarWidth();
On 2013/10/30 12:02:13, sanjoyp wrote:
> On 2013/10/30 02:03:41, Julien Chaffraix - PST wrote:
> > Is this code tested somewhere?
> Are you talking about LayoutTests? I have written a layout test which tests
this
> code and there are few existing tests need to be rebaselined.

Yes, I was talking about LayoutTests. And no, your new test cannot cover this
code path: you don't have positioned elements. I don't think any of the
rebaselined test do cover this code path either.

However scrollbars/rtl/div-absolute.html clearly fails if you don't have this
change so yes we have some coverage.

[commit-bot: I haz the power, 2013-11-01 00:29:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/25373006/106001

[commit-bot: I haz the power, 2013-11-01 02:34:11]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-11-05 08:41:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/25373006/276001

[commit-bot: I haz the power, 2013-11-05 08:41:33]

Failed to apply patch for
LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A        
LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
  Copied LayoutTests/scrollbars/rtl/div-horizontal.html ->
LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
  patching file
LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
  Hunk #1 FAILED at 1.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html.rej

Patch:   NR 
LayoutTests/scrollbars/rtl/div-horizontal.html->LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
Index: LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
diff --git a/LayoutTests/scrollbars/rtl/div-horizontal.html
b/LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
similarity index 76%
copy from LayoutTests/scrollbars/rtl/div-horizontal.html
copy to LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
index
a5e44300c7dbf251b911dca488aa00780a0ac624..5613ee5a66888ec2f78b174ff217cd96cfb77cc4
100644
--- a/LayoutTests/scrollbars/rtl/div-horizontal.html
+++ b/LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
@@ -1,18 +1,17 @@
 <!DOCTYPE html>
 <html>
 <head>
-<title>Bug 85856</title>
 <script src="../../fast/js/resources/js-test-pre.js"></script>
 </head>
 <body dir="ltr">
-<div dir="ltr" id="outer-ltr" style="width: 200px; height: 200px; overflow:
scroll;">
-<div id="inner-ltr" style="text-align: left; width: 300px; height:
300px;">ABC</div>
+<div dir="ltr" id="outer-ltr" style="width: 200px; height: 200px; overflow:
auto;">
+<div id="inner-ltr" style="text-align: left; width: 195px; height:
300px;">ABC</div>
 </div>
-<div dir="rtl" id="outer-rtl" style="width: 200px; height: 200px; overflow:
scroll;">
-<div id="inner-rtl" style="text-align: left; width: 300px; height:
300px;">ABC</div>
+<div dir="rtl" id="outer-rtl" style="width: 200px; height: 200px; overflow:
auto;">
+<div id="inner-rtl" style="text-align: left; width: 195px; height:
300px;">ABC</div>
 </div>
 <script type="text/javascript">
-description('Test if WebKit can show the left side of the inner elements
regardless of the position of its scrollbars. To test manually, open this
document and verify we can see "ABC" both in the LTR element and in the RTL
element.');
+description('Test that horizontal scrollbar should appear as the content width
encroaches upon the vertical scrollbar and also test if WebKit can show the left
side of the inner elements regardless of the position of its scrollbars. To test
manually, open this document and verify we can see "ABC" both in the LTR element
and in the RTL element.');
 
 var outerLTR = document.getElementById('outer-ltr');
 var innerLTR = document.getElementById('inner-ltr');

[commit-bot: I haz the power, 2013-11-05 11:11:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/25373006/366001

[commit-bot: I haz the power, 2013-11-05 11:44:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/25373006/566001

[commit-bot: I haz the power, 2013-11-05 12:37:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/25373006/686001

[commit-bot: I haz the power, 2013-11-05 13:57:02]

Change committed as 161333