Fix clientLeft value for RTL direction, while the element have vertical scrollbar in left side.

Fix clientLeft value for RTL direction, while the element have vertical scrollbar in left side.

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

[pals, 2013-11-13 13:59:09]

Please review.

[eseidel, 2013-11-13 16:47:08]

I doubt you need the needsrebaseline changes for this text test. Does your
test pass in ie/ff?
On Nov 13, 2013 5:59 AM, <sanjoy.pal@samsung.com> wrote:

> Reviewers: eseidel, ojan,
>
> Message:
> Please review.
>
> Description:
> Fix clientLeft value for RTL direction, while the element have vertical
> scrollbar in left side.
>
> BUG=128500
>
> Please review this at https://codereview.chromium.org/70163005/
>
> SVN Base: https://chromium.googlesource.com/chromium/blink.git@master
>
> Affected files (+4, -1 lines):
>   M LayoutTests/TestExpectations
>   M LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-
> scrollbar-expected.txt
>   M LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
>   M Source/core/rendering/RenderBox.h
>
>
> Index: LayoutTests/TestExpectations
> diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
> index 79a38ef4d6935c12da4443ce587f1df3056a469e..
> adfab42ab4b7924304faf5921810c42bc05a1075 100644
> --- a/LayoutTests/TestExpectations
> +++ b/LayoutTests/TestExpectations
> @@ -1,3 +1,4 @@
> +crbug.com/128500 [ Win Mac ]
scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
> [ NeedsRebaseline ]
>
>  # This test can't just be rebaselined because it has absolute file paths
> in the output.
>  crbug.com/257132 http/tests/inspector/network/network-xhr-replay.html [
> Failure ]
> Index: LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-
> scrollbar-expected.txt
> diff --git
a/LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar-expected.txt
> b/LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-
> scrollbar-expected.txt
> index eda6b7323ff60bd1e97a0d5d15c6298258243d22..
> 1443d10bed3e78f33b0e07e5af5719105ec2a632 100644
> --- a/LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-
> scrollbar-expected.txt
> +++ b/LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-
> scrollbar-expected.txt
> @@ -13,6 +13,7 @@ PASS innerLTR.clientWidth == innerRTL.clientWidth is true
>  PASS innerLTR.scrollWidth == innerRTL.scrollWidth is true
>  Verify the width of the vertical scrollbar of the outer RTL element is
> the same as the one of the outer LTR element regardless of their scrollbar
> positions.
>  PASS scrollbarWidthLTR == scrollbarWidthRTL is true
> +PASS outerRTL.clientLeft == scrollbarWidthRTL is true
>  PASS successfullyParsed is true
>
>  TEST COMPLETE
> Index: LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-
> scrollbar.html
> diff --git
a/LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
> b/LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-scrollbar.html
> index 5c59fcbf7d4950e77e5f05904d60c7d19e80dfce..
> ba10654e119091dc97cfcd2daa8a431155afd965 100644
> --- a/LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-
> scrollbar.html
> +++ b/LayoutTests/scrollbars/rtl/div-horizontal-with-vertical-
> scrollbar.html
> @@ -34,6 +34,7 @@ debug('Verify the width of the vertical scrollbar of the
> outer RTL element is th
>  var scrollbarWidthLTR = outerLTR.offsetWidth - outerLTR.clientWidth;
>  var scrollbarWidthRTL = outerRTL.offsetWidth - outerRTL.clientWidth;
>  shouldBeTrue('scrollbarWidthLTR == scrollbarWidthRTL');
> +shouldBeTrue('outerRTL.clientLeft == scrollbarWidthRTL');
>  </script>
>  </body>
>  </html>
> Index: Source/core/rendering/RenderBox.h
> diff --git a/Source/core/rendering/RenderBox.h b/Source/core/rendering/
> RenderBox.h
> index 2aa00d4c0ee9a3e09b265c47561349e373568e6c..
> 6210a054ccda00080e1d6ea1167a9fe21a72ce98 100644
> --- a/Source/core/rendering/RenderBox.h
> +++ b/Source/core/rendering/RenderBox.h
> @@ -228,7 +228,7 @@ public:
>
>      // More IE extensions.  clientWidth and clientHeight represent the
> interior of an object
>      // excluding border and scrollbar.  clientLeft/Top are just the
> borderLeftWidth and borderTopWidth.
> -    LayoutUnit clientLeft() const { return borderLeft(); }
> +    LayoutUnit clientLeft() const { return borderLeft() + (style()->
> shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ?
> verticalScrollbarWidth() : 0); }
>      LayoutUnit clientTop() const { return borderTop(); }
>      LayoutUnit clientWidth() const;
>      LayoutUnit clientHeight() const;
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[pals, 2013-11-14 08:54:00]

Please check the comment#4 on the bug#125500
http://code.google.com/p/chromium/issues/detail?id=128500#c4. The test passes on
FF but fails on IE 10. Updated the patch, removed the TestExpectations changes.

[eseidel, 2013-11-14 08:56:13]

To be clear: Blink/WebKit's current behavior matches neither FF or IE, correct? 
This patch makes us match FF and old IE, but not new IE, is that correct?

[pals, 2013-11-14 09:01:27]

On 2013/11/14 08:56:13, eseidel wrote:
> To be clear: Blink/WebKit's current behavior matches neither FF or IE,
correct? 
> This patch makes us match FF and old IE, but not new IE, is that correct?

Yes. that is correct.

[eseidel, 2013-11-18 17:21:15]

lgtm

[commit-bot: I haz the power, 2013-11-18 17:21:30]

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

[commit-bot: I haz the power, 2013-11-18 18:53:28]

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

[commit-bot: I haz the power, 2013-12-04 17:40:57]

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

[eseidel, 2013-12-04 17:41:03]

It looks like this patch just failed to land, may need an update to land.

[commit-bot: I haz the power, 2013-12-04 18:56:03]

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

[leviw_travelin_and_unemployed, 2014-02-19 23:04:56]

On 2013/12/04 18:56:03, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

This still hasn't landed... Likely needs to be updated?

[pals, 2014-02-20 11:04:52]

On 2014/02/19 23:04:56, Levi wrote:
> On 2013/12/04 18:56:03, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_blink_rel for step(s) webkit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
> 
> This still hasn't landed... Likely needs to be updated?

There are few layout test failures for this patch. While investigating the
failure, i came to know that its not simple to fix, few more things needs to be
corrected. I shall look into this again.

[pals, 2014-02-21 09:45:27]

Fixed the layout tests failures. Please review the changes.

[pals, 2014-03-06 06:40:32]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2014-03-06 06:40:44]

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

[commit-bot: I haz the power, 2014-03-06 06:41:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 06:41:53]

Try jobs failed on following builders: mac_blink_compile

[pals, 2014-03-06 11:37:41]

The CQ bit was checked by sanjoy.pal@samsung.com

[commit-bot: I haz the power, 2014-03-06 11:37:51]

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

[commit-bot: I haz the power, 2014-03-06 11:50:04]

Change committed as 168625