Bug 64904 – REGRESSION(r88757): input[type=range] renders incorrectly in RTL context

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Rachel Blum]

See sample page - the slider 'knob' is rendered outside the actual slider. This is true for both latest nightly (r91186) and a nightly as far back as r89741 - haven't tried earlier nightlies.

Safari Version 5.0.5 (6533.21.1) renders this correctly.

Reduced error case attached

[Rachel Blum]

Created attachment 101527 [details]
repro case

[Jeremy Moskovich]

Regressions are P1.

groby: Can you bisect to find where this broke?

[Yair Yogev]

i'll have one in a few minutes

[Kent Tamura]

r88757 is suspective.

[Yair Yogev]

yes, i got as far as WebKit 88697:88789. (and r88757 seems likely)

[Yair Yogev]

It breaks RTL DOMUI-Options Fonts pane in Chromium ( http://crbug.com/87690 )

[Kent Tamura]

Created attachment 101562 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 101562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101562&action=review

> Source/WebCore/html/shadow/SliderThumbElement.cpp:243
> +    if (isVertical || !renderBox()->style()->isLeftToRightDirection())

Would this work if isVertical was true and isLeftToRightDirection() was false?

[Kent Tamura]

Comment on attachment 101562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101562&action=review

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:243
>> +    if (isVertical || !renderBox()->style()->isLeftToRightDirection())
> 
> Would this work if isVertical was true and isLeftToRightDirection() was false?

It works.
If isVertical is true, isLeftToRightDirection() doesn't matter. We have no LTR/RTL differences in vertical sliders.

[Ryosuke Niwa]

Comment on attachment 101562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101562&action=review

ok, looks sane to me.

>>> Source/WebCore/html/shadow/SliderThumbElement.cpp:243
>>> +    if (isVertical || !renderBox()->style()->isLeftToRightDirection())
>> 
>> Would this work if isVertical was true and isLeftToRightDirection() was false?
> 
> It works.
> If isVertical is true, isLeftToRightDirection() doesn't matter. We have no LTR/RTL differences in vertical sliders.

Should we at least add a test case for it?

[Kent Tamura]

Created attachment 101564 [details]
Patch for landing

Add a vertical case

[WebKit Review Bot]

Comment on attachment 101564 [details]
Patch for landing

Rejecting attachment 101564 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:

Unexpected flakiness: text diff mismatch (1)
  fast/forms/implicit-submission.html = TEXT PASS


Regressions: Unexpected image mismatch : (6)
  fast/forms/input-appearance-range-rtl.html = IMAGE
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE



Full output: http://queues.webkit.org/results/9191618

[Kent Tamura]

(In reply to comment #12)
> Regressions: Unexpected image mismatch : (6)
>   fast/forms/input-appearance-range-rtl.html = IMAGE

I have no idea what's wrong on Chromium-linux.  Anyway, it must be a test problem, not a C++ code problem.  I'll add the test to test_expectations.txt, land the patch, and see results on Chromium buildbots.

[Kent Tamura]

Committed r91460: <http://trac.webkit.org/changeset/91460>

[Kent Tamura]

(In reply to comment #13)
> (In reply to comment #12)
> > Regressions: Unexpected image mismatch : (6)
> >   fast/forms/input-appearance-range-rtl.html = IMAGE
> 
> I have no idea what's wrong on Chromium-linux.  Anyway, it must be a test problem, not a C++ code problem.  I'll add the test to test_expectations.txt, land the patch, and see results on Chromium buildbots.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showLargeExpectations=true&showExpectations=true&tests=range-rtl

It was due to a Chromium bug: http://code.google.com/p/chromium/issues/detail?id=89616