Bug 156007 - REGRESSION (197234): [RTL] Search field cancel buttons are cut off.
Summary: REGRESSION (197234): [RTL] Search field cancel buttons are cut off.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-29 20:31 PDT by zalan
Modified: 2016-04-25 06:59 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.59 KB, text/plain)
2016-03-29 20:41 PDT, zalan
no flags Details
Cut off cancel button (8.74 KB, image/png)
2016-03-29 20:43 PDT, zalan
no flags Details
Correct rendering (7.70 KB, image/png)
2016-03-29 20:47 PDT, zalan
no flags Details
Patch (8.57 KB, patch)
2016-03-29 20:49 PDT, zalan
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.01 MB, application/zip)
2016-03-29 21:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (772.79 KB, application/zip)
2016-03-29 21:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (931.64 KB, application/zip)
2016-03-29 21:44 PDT, Build Bot
no flags Details
Patch (15.79 KB, patch)
2016-03-30 10:30 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2016-03-29 20:31:26 PDT
SSIA.
Comment 1 zalan 2016-03-29 20:31:59 PDT
rdar://problem/25346475
Comment 2 Simon Fraser (smfr) 2016-03-29 20:32:47 PDT
I don't think the SSIA in this case!
Comment 3 zalan 2016-03-29 20:41:28 PDT
Created attachment 275172 [details]
Patch
Comment 4 zalan 2016-03-29 20:43:29 PDT
Created attachment 275173 [details]
Cut off cancel button
Comment 5 zalan 2016-03-29 20:43:57 PDT
(In reply to comment #2)
> I don't think the SSIA in this case!
ASIA
Comment 6 zalan 2016-03-29 20:47:13 PDT
Created attachment 275175 [details]
Correct rendering
Comment 7 zalan 2016-03-29 20:49:31 PDT
Created attachment 275176 [details]
Patch
Comment 8 Simon Fraser (smfr) 2016-03-29 21:22:54 PDT
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?
Comment 9 Build Bot 2016-03-29 21:25:02 PDT
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
Comment 10 Build Bot 2016-03-29 21:25:05 PDT
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
Comment 11 Build Bot 2016-03-29 21:36:35 PDT
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
Comment 12 Build Bot 2016-03-29 21:36:37 PDT
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
Comment 13 Build Bot 2016-03-29 21:44:38 PDT
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
Comment 14 Build Bot 2016-03-29 21:44:41 PDT
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
Comment 15 zalan 2016-03-30 10:30:11 PDT
Created attachment 275198 [details]
Patch
Comment 16 Jon Lee 2016-03-30 10:39:25 PDT
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?
Comment 17 Jon Lee 2016-03-30 10:44:36 PDT
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.
Comment 18 Brent Fulgham 2016-03-31 20:18:35 PDT
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?
Comment 19 Myles C. Maxfield 2016-03-31 20:46:42 PDT
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.
Comment 20 zalan 2016-03-31 20:48:33 PDT
(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.
Comment 21 Darin Adler 2016-04-03 15:38:18 PDT
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.