Bug 97555 – SuggestionPicker should support rtl

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

SuggestionPicker needs to support rtl

[Keishi Hattori]

Created attachment 165602 [details]
Patch

[Kent Tamura]

Comment on attachment 165602 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        * platform/chromium-mac/platform/chromium/fast/forms/date/date-suggestion-picker-appearance-rtl-expected.png: Added.

Does this result  match to your expectation?  The content of the popup looks not RTL, and looks same with date-suggestion-picker-appearance-expected.png in Bug 97551.

[Keishi Hattori]

Created attachment 165927 [details]
Patch

[Kent Tamura]

Comment on attachment 165927 [details]
Patch

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

> Source/WebCore/platform/DateTimeChooser.h:50
> +    bool isValueRTL;

"value" sounds this represents the direction of each of <option> values.
isAnchorElementRTL ?

> LayoutTests/ChangeLog:10
> +        * platform/chromium-mac/platform/chromium/fast/forms/date/date-suggestion-picker-appearance-rtl-expected.png: Added.

The content of the popup still looks LTR.

[Keishi Hattori]

Created attachment 165933 [details]
Patch

[Kent Tamura]

(In reply to comment #5)
> Created an attachment (id=165933) [details]
> Patch

What's wrong with the previous patch?

[Kent Tamura]

Comment on attachment 165933 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

OOPS!

> LayoutTests/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

OOPS!

[Keishi Hattori]

Created attachment 165938 [details]
Patch

[Keishi Hattori]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=165933) [details] [details]
> > Patch
> 
> What's wrong with the previous patch?

I renamed to isAnchorElementRTL. And I changed the test to set rtl to the body because that is more common and we can align popup position to the right in the future.

[Kent Tamura]

(In reply to comment #9)
> I renamed to isAnchorElementRTL. And I changed the test to set rtl to the body because that is more common and we can align popup position to the right in the future.

They shouldn't affect the direction of the popup content.  Why was it corrected?

[Keishi Hattori]

(In reply to comment #10)
> (In reply to comment #9)
> > I renamed to isAnchorElementRTL. And I changed the test to set rtl to the body because that is more common and we can align popup position to the right in the future.
> 
> They shouldn't affect the direction of the popup content.  Why was it corrected?

I forgot to reset the expected image in the previous patch(165927).

[Kent Tamura]

(In reply to comment #11)
> I forgot to reset the expected image in the previous patch(165927).

I see. I recommend you explain difference from the previous patch when you post a new patch. Silent update confuses reviewers.

[Kent Tamura]

Comment on attachment 165938 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 165938 [details]
Patch

Clearing flags on attachment: 165938

Committed r129738: <http://trac.webkit.org/changeset/129738>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.