Bug 64904 - REGRESSION(r88757): input[type=range] renders incorrectly in RTL context
Summary: REGRESSION(r88757): input[type=range] renders incorrectly in RTL context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-20 16:50 PDT by Rachel Blum
Modified: 2011-07-21 05:31 PDT (History)
8 users (show)

See Also:


Attachments
repro case (239 bytes, text/html)
2011-07-20 16:52 PDT, Rachel Blum
no flags Details
Patch (4.44 KB, patch)
2011-07-21 02:05 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch for landing (4.63 KB, patch)
2011-07-21 02:31 PDT, Kent Tamura
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rachel Blum 2011-07-20 16:50:09 PDT
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
Comment 1 Rachel Blum 2011-07-20 16:52:57 PDT
Created attachment 101527 [details]
repro case
Comment 2 Jeremy Moskovich 2011-07-21 00:29:55 PDT
Regressions are P1.

groby: Can you bisect to find where this broke?
Comment 3 Yair Yogev 2011-07-21 00:31:29 PDT
i'll have one in a few minutes
Comment 4 Kent Tamura 2011-07-21 00:35:04 PDT
r88757 is suspective.
Comment 5 Yair Yogev 2011-07-21 00:41:31 PDT
yes, i got as far as WebKit 88697:88789. (and r88757 seems likely)
Comment 6 Yair Yogev 2011-07-21 00:48:48 PDT
It breaks RTL DOMUI-Options Fonts pane in Chromium ( http://crbug.com/87690 )
Comment 7 Kent Tamura 2011-07-21 02:05:02 PDT
Created attachment 101562 [details]
Patch
Comment 8 Ryosuke Niwa 2011-07-21 02:18:51 PDT
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?
Comment 9 Kent Tamura 2011-07-21 02:25:02 PDT
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.
Comment 10 Ryosuke Niwa 2011-07-21 02:26:41 PDT
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?
Comment 11 Kent Tamura 2011-07-21 02:31:31 PDT
Created attachment 101564 [details]
Patch for landing

Add a vertical case
Comment 12 WebKit Review Bot 2011-07-21 04:07:56 PDT
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
Comment 13 Kent Tamura 2011-07-21 04:17:53 PDT
(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.
Comment 14 Kent Tamura 2011-07-21 04:21:55 PDT
Committed r91460: <http://trac.webkit.org/changeset/91460>
Comment 15 Kent Tamura 2011-07-21 05:31:45 PDT
(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