Bug 97555 - SuggestionPicker should support rtl
Summary: SuggestionPicker should support rtl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-25 05:02 PDT by Keishi Hattori
Modified: 2012-09-27 01:17 PDT (History)
2 users (show)

See Also:


Attachments
Patch (35.74 KB, patch)
2012-09-25 06:36 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (37.56 KB, patch)
2012-09-26 22:22 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (37.41 KB, patch)
2012-09-26 23:03 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (37.42 KB, patch)
2012-09-26 23:37 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2012-09-25 05:02:06 PDT
SuggestionPicker needs to support rtl
Comment 1 Keishi Hattori 2012-09-25 06:36:53 PDT
Created attachment 165602 [details]
Patch
Comment 2 Kent Tamura 2012-09-25 20:03:50 PDT
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.
Comment 3 Keishi Hattori 2012-09-26 22:22:37 PDT
Created attachment 165927 [details]
Patch
Comment 4 Kent Tamura 2012-09-26 22:33:20 PDT
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.
Comment 5 Keishi Hattori 2012-09-26 23:03:03 PDT
Created attachment 165933 [details]
Patch
Comment 6 Kent Tamura 2012-09-26 23:09:33 PDT
(In reply to comment #5)
> Created an attachment (id=165933) [details]
> Patch

What's wrong with the previous patch?
Comment 7 Kent Tamura 2012-09-26 23:18:07 PDT
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!
Comment 8 Keishi Hattori 2012-09-26 23:37:31 PDT
Created attachment 165938 [details]
Patch
Comment 9 Keishi Hattori 2012-09-26 23:42:52 PDT
(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.
Comment 10 Kent Tamura 2012-09-26 23:45:26 PDT
(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?
Comment 11 Keishi Hattori 2012-09-26 23:57:26 PDT
(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).
Comment 12 Kent Tamura 2012-09-26 23:59:23 PDT
(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.
Comment 13 Kent Tamura 2012-09-27 00:00:02 PDT
Comment on attachment 165938 [details]
Patch

ok
Comment 14 WebKit Review Bot 2012-09-27 01:17:09 PDT
Comment on attachment 165938 [details]
Patch

Clearing flags on attachment: 165938

Committed r129738: <http://trac.webkit.org/changeset/129738>
Comment 15 WebKit Review Bot 2012-09-27 01:17:12 PDT
All reviewed patches have been landed.  Closing bug.