Bug 59703 - input type=number doesn't render correctly in rtl
Summary: input type=number doesn't render correctly in rtl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 64108
  Show dependency treegraph
 
Reported: 2011-04-28 08:37 PDT by Tony Gentilcore
Modified: 2011-07-20 00:52 PDT (History)
7 users (show)

See Also:


Attachments
Testcase (28 bytes, text/html)
2011-04-28 08:37 PDT, Tony Gentilcore
no flags Details
Patch (20.18 KB, patch)
2011-07-19 23:01 PDT, Kent Tamura
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2011-04-28 08:37:06 PDT
Created attachment 91497 [details]
Testcase

Begin typing the in the rtl number input in the attached testcase. Notice that the input overlaps the spinner.
Comment 1 Alexey Proskuryakov 2011-04-28 15:57:26 PDT
The behavior is different between Safari 5.0.5 and ToT, but badly broken in both.
Comment 2 Kent Tamura 2011-07-19 23:01:53 PDT
Created attachment 101428 [details]
Patch
Comment 3 WebKit Review Bot 2011-07-19 23:04:59 PDT
Attachment 101428 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

ERROR: FAILURES FOR <, x86, release, cpu>
ERROR: Line:27 Test lacks BUG modifier. editing/selection/empty-cell-right-click.html
ERROR: Line:28 Test lacks BUG modifier. editing/selection/dump-as-markup.html
ERROR: Line:29 Test lacks BUG modifier. fast/js/array-sort-modifying-tostring.html
ERROR: Line:30 Test lacks BUG modifier. fast/overflow/lots-of-sibling-inline-boxes.html
ERROR: Line:31 Test lacks BUG modifier. inspector/cookie-resource-match.html
LayoutTests/platform/qt/test_expectations.txt:27:  Test lacks BUG modifier. editing/selection/empty-cell-right-click.html  [test/expectations] [2]
LayoutTests/platform/qt/test_expectations.txt:28:  Test lacks BUG modifier. editing/selection/dump-as-markup.html  [test/expectations] [2]
LayoutTests/platform/qt/test_expectations.txt:29:  Test lacks BUG modifier. fast/js/array-sort-modifying-tostring.html  [test/expectations] [2]
LayoutTests/platform/qt/test_expectations.txt:30:  Test lacks BUG modifier. fast/overflow/lots-of-sibling-inline-boxes.html  [test/expectations] [2]
LayoutTests/platform/qt/test_expectations.txt:31:  Test lacks BUG modifier. inspector/cookie-resource-match.html  [test/expectations] [2]
Total errors found: 5 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Jeremy Moskovich 2011-07-19 23:51:29 PDT
Thanks for posting this patch!

Not a reviewer but, here's my take:
I'd add a bit of text to the layout test so it's easy to tell what the desired behavior is.  e.g. "in this input box the button should be on the left of the field" otherwise if one looks at the test without context it's hard to tell what the desired behavior is.
Comment 5 Ryosuke Niwa 2011-07-19 23:53:37 PDT
Comment on attachment 101428 [details]
Patch

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

> LayoutTests/fast/forms/input-appearance-number-rtl.html:3
> +
> +<p dir=rtl><input type=number value=1>

I'd like to see <html><body> and </p>'s since we're not really testing HTML parser here.

> LayoutTests/fast/forms/input-appearance-number-rtl.html:14
> +document.getElementById('i5').dir = 'ltr';

Can we have another variant that sets dir to rtl?

> Source/WebCore/css/html.css:430
>  input::-webkit-textfield-decoration-container {
> -    direction: ltr;
>      display: -webkit-box;
>      -webkit-box-align: center;
>  }
>  
> +input[type="search"]::-webkit-textfield-decoration-container {
> +    direction: ltr;
> +}
> +

Does this fix input[type="speech"]?  e.g. <p dir="rtl"><input type="speech" ...>  If so, we should add a test for it.
Comment 6 Ryosuke Niwa 2011-07-19 23:54:23 PDT
(In reply to comment #5)
> > LayoutTests/fast/forms/input-appearance-number-rtl.html:14
> > +document.getElementById('i5').dir = 'ltr';
> 
> Can we have another variant that sets dir to rtl?

Also, it'll be nice to test style.direction = 'ltr'/'rtl';
Comment 7 Ryosuke Niwa 2011-07-19 23:56:30 PDT
Comment on attachment 101428 [details]
Patch

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

> LayoutTests/fast/forms/input-appearance-number-rtl.html:2
> +

Also, as Jeremy pointed out, it'll be nice to have some description as to what this test is testing what output is expected.  You can put it in a comment if you'd like to avoid random font-difference failures on the various bots.
Comment 8 Kent Tamura 2011-07-20 00:48:41 PDT
Comment on attachment 101428 [details]
Patch

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

>> LayoutTests/fast/forms/input-appearance-number-rtl.html:2
>> +
> 
> Also, as Jeremy pointed out, it'll be nice to have some description as to what this test is testing what output is expected.  You can put it in a comment if you'd like to avoid random font-difference failures on the various bots.

Done

>> LayoutTests/fast/forms/input-appearance-number-rtl.html:3
>> +<p dir=rtl><input type=number value=1>
> 
> I'd like to see <html><body> and </p>'s since we're not really testing HTML parser here.

Done.

>> LayoutTests/fast/forms/input-appearance-number-rtl.html:14
>> +document.getElementById('i5').dir = 'ltr';
> 
> Can we have another variant that sets dir to rtl?

Done.

>> Source/WebCore/css/html.css:430
>> +
> 
> Does this fix input[type="speech"]?  e.g. <p dir="rtl"><input type="speech" ...>  If so, we should add a test for it.

x-webkit-speech case is covered by LayoutTests/fast/speech/speech-bidi-rendering.html.
Comment 9 Kent Tamura 2011-07-20 00:52:53 PDT
Committed r91345: <http://trac.webkit.org/changeset/91345>