Bug 156007 – REGRESSION (197234): [RTL] Search field cancel buttons are cut off.

• Status: NEW
• Component: Layout and Rendering

[alan]

SSIA.

[alan]

rdar://problem/25346475

[Simon Fraser (smfr)]

I don't think the SSIA in this case!

[alan]

Created attachment 275172 [details]
Patch

[alan]

Created attachment 275173 [details]
Cut off cancel button

[alan]

(In reply to comment #2)
> I don't think the SSIA in this case!
ASIA

[alan]

Created attachment 275175 [details]
Correct rendering

[alan]

Created attachment 275176 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 275176 [details]
Patch

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

> Source/WebCore/html/TextFieldInputType.cpp:650
> +    m_container->setAttribute(dirAttr, systemLanguageIsRTL() ? "rtl" : "ltr");

What happens to a search field inside dir="rtl" content now? What will happen to a dir="ltr" search field when systemLanguageIsRTL?

> Source/WebCore/platform/mac/Language.mm:146
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200
> +    static bool result = [[NSScrollerImpPair class] respondsToSelector:@selector(scrollerLayoutDirection)] && [NSScrollerImpPair scrollerLayoutDirection] == NSUserInterfaceLayoutDirectionRightToLeft;
> +    return result;
> +#else
> +    return false;
> +#endif

Is this the best way? Maybe we should read the defaults that affect NSScrollerImpPair behavior?

[Build Bot]

Comment on attachment 275176 [details]
Patch

Attachment 275176 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1067064

New failing tests:
fast/forms/number/number-appearance-rtl.html

[Build Bot]

Created attachment 275177 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275176 [details]
Patch

Attachment 275176 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1067123

New failing tests:
fast/forms/number/number-appearance-rtl.html

[Build Bot]

Created attachment 275179 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275176 [details]
Patch

Attachment 275176 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1067115

New failing tests:
fast/forms/number/number-appearance-rtl.html

[Build Bot]

Created attachment 275181 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[alan]

Created attachment 275198 [details]
Patch

[Jon Lee]

Comment on attachment 275198 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Partially covered by existing tests.

Can we add one for search?

[Jon Lee]

Comment on attachment 275198 [details]
Patch

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

> Source/WebCore/html/TextFieldInputType.cpp:650
> +    m_container->setAttribute(dirAttr, systemLanguageIsRTL() ? "rtl" : "ltr");

Is it better to set the style inline rather than attaching a class, and adding the rule to the base stylesheet?

> LayoutTests/platform/mac/fast/forms/number/number-appearance-rtl-expected.txt:9
> +            RenderBlock {DIV} at (0,0) size 118x13

Actually, why should this test change at all? The render tree should not be changing for any of these tests.

[Brent Fulgham]

Comment on attachment 275198 [details]
Patch

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

> Source/WebCore/platform/mac/Language.mm:145
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200

Do we need #if PLATFORM(MAC) in a mac-specific file?

[Myles C. Maxfield]

Please wait to land anything that is relevant to detection of an RTL environment. I'm still figuring out the best way to do that on OS X.

[alan]

(In reply to comment #19)
> Please wait to land anything that is relevant to detection of an RTL
> environment. I'm still figuring out the best way to do that on OS X.
ofc. There's not even a valid patch on this to land.

[Darin Adler]

Comment on attachment 275198 [details]
Patch

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

>> Source/WebCore/html/TextFieldInputType.cpp:650
>> +    m_container->setAttribute(dirAttr, systemLanguageIsRTL() ? "rtl" : "ltr");
> 
> Is it better to set the style inline rather than attaching a class, and adding the rule to the base stylesheet?

The line above goes out of its way to use AtomicString::ConstructFromLiteral. This line of code should consider doing the same thing. Or use some other technique to lower the cost of looking up these constant strings in the atomic string table and then allocating new strings. Strangely, toValidDirValue does the NeverDestroyed trick to make an efficient global variable to avoid the overhead, but the many other call sites that use these strings don’t bother.

> Source/WebCore/platform/Language.cpp:170
> +#endif
>  }

Missing blank line here.

> Source/WebCore/platform/Language.h:53
> +bool systemLanguageIsRTL();
>  }

Missing blank line here.