Bug 38749 - [Chromium] Pipe RTL info into WebPopupMenuInfo
Summary: [Chromium] Pipe RTL info into WebPopupMenuInfo
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-07 07:54 PDT by Avi Drissman
Modified: 2010-05-11 16:04 PDT (History)
4 users (show)

See Also:


Attachments
A patch to fill in a "right aligned" attr in the menu info (7.01 KB, patch)
2010-05-07 07:57 PDT, Avi Drissman
fishd: review-
Details | Formatted Diff | Diff
Update (3.60 KB, patch)
2010-05-10 10:57 PDT, Avi Drissman
no flags Details | Formatted Diff | Diff
Alignment it is. (3.31 KB, patch)
2010-05-10 13:50 PDT, Avi Drissman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Avi Drissman 2010-05-07 07:54:47 PDT
Bug 29612 fixed right-alignment for RTL <select> widgets. This extends that work to pipe right-alignment status out to the API level in WebPopupMenuInfo so that Chromium Mac can properly right-align popups in the correct circumstances.
Comment 1 Avi Drissman 2010-05-07 07:57:01 PDT
Created attachment 55374 [details]
A patch to fill in a "right aligned" attr in the menu info
Comment 2 Jeremy Moskovich 2010-05-07 08:04:23 PDT
Comment on attachment 55374 [details]
A patch to fill in a "right aligned" attr in the menu info

Ob-Disclaimer: I'm not a WebKit reviewer.

WebKit/chromium/src/ChromeClientImpl.cpp:694
 +      info->rightAligned = popupClient->menuStyle().textDirection() == RTL;
It's not really text alignment, but directionality - IMHO rename to rtl, directionality [and use an enum] or somesuch.

Otherwise, lgtm.
Comment 3 Avi Drissman 2010-05-07 08:27:16 PDT
(In reply to comment #2)
> It's not really text alignment

It is really text alignment. This is intended to be an alignment flag, not a directionality flag. In the client end I'm using it to signal to the system alignment (NSRightTextAlignment, as seen in http://codereview.chromium.org/1992006 ). Yes, it's determined from directionality, but that's not what it is.
Comment 4 David Levin 2010-05-07 10:09:55 PDT
Comment on attachment 55374 [details]
A patch to fill in a "right aligned" attr in the menu info

Since this has a change to a public api file, I'll leave it for Darin Fisher to give a r+/r-, but here's a comment on a minor WebKit style issue that it would be good to fix.

> Index: WebCore/page/chromium/ChromeClientChromium.h
>      virtual void popupOpened(PopupContainer* popupContainer, const IntRect& bounds,
> +                             bool handleExternal, PopupMenuClient* popupClient) = 0;

Style nit: Since the parameter name "popupClient" provide no new information, it shouldn't be here.
(The same is true for popupContainer but that itsn't part of your change.)


> Index: WebKit/chromium/src/ChromeClientImpl.h
>      virtual void popupOpened(WebCore::PopupContainer* popupContainer,
>                               const WebCore::IntRect& bounds,
> +                             bool handleExternally,
> +                             WebCore::PopupMenuClient* popupClient);

Ditto.

> +    void getPopupMenuInfo(WebCore::PopupContainer*, WebPopupMenuInfo*, WebCore::PopupMenuClient* popupClient);

Ditto.
Comment 5 Darin Fisher (:fishd, Google) 2010-05-07 11:30:12 PDT
Comment on attachment 55374 [details]
A patch to fill in a "right aligned" attr in the menu info

WebKit/chromium/public/WebPopupMenuInfo.h:47
 +      bool rightAligned;
Use the WebTextDirection enum here?

WebKit/chromium/src/ChromeClientImpl.cpp:694
 +      info->rightAligned = popupClient->menuStyle().textDirection() == RTL;
Why not just get the PopupMenuStyle object from the PopupContainer?
You could add a method on PopupContainer that returns PopupMenuStyle*.
It doesn't seem necessary to plumb PopupMenuClient* through the layers
when you have PopupContainer at your disposal.
Comment 6 Xiaomei Ji 2010-05-07 16:02:04 PDT
(In reply to comment #2)
> (From update of attachment 55374 [details])
> Ob-Disclaimer: I'm not a WebKit reviewer.
> 
> WebKit/chromium/src/ChromeClientImpl.cpp:694
>  +      info->rightAligned = popupClient->menuStyle().textDirection() == RTL;
> It's not really text alignment, but directionality - IMHO rename to rtl,
> directionality [and use an enum] or somesuch.
> 
> Otherwise, lgtm.

We are currently using popup's text direction to direct the alignment (align right if the <select> is RTL, align right if the <input> is RTL).

Webkit always align popup left (at least in Mac, when writing direction is natural) in RenderMenuList::adjustInnerStyle() (refer to Bug 19785).

We need to add text alignment in PopupMenuStyle, so that we can display the text in <select> box using natural writing direction, and draw the texts in popup box in desired alignment.

I am opening another Bug 38780
Comment 7 Avi Drissman 2010-05-10 10:57:25 PDT
Created attachment 55571 [details]
Update
Comment 8 Avi Drissman 2010-05-10 10:59:12 PDT
If everyone wants it to be text direction, then I'll make it text direction. Otherwise it's simplified as per fishd's recommendation (which makes it work much better; thanks!)
Comment 9 Xiaomei Ji 2010-05-10 11:17:37 PDT
@@ -44,6 +45,7 @@ struct WebPopupMenuInfo {
     int itemFontSize;
     int selectedIndex;
     WebVector<WebMenuItemInfo> items;
+    WebTextDirection textDirection;
 };

Maybe I made you confused.
You were right in your comment #3. It is a alignment, not text direction, and it is used in client side as alignment. 

We are currently using the text direction to guide the alignment, which is not totally correct. Hence Bug 38780 is filed.
Comment 10 Avi Drissman 2010-05-10 13:50:12 PDT
Created attachment 55599 [details]
Alignment it is.
Comment 11 WebKit Commit Bot 2010-05-11 16:04:46 PDT
Comment on attachment 55599 [details]
Alignment it is.

Clearing flags on attachment: 55599

Committed r59179: <http://trac.webkit.org/changeset/59179>
Comment 12 WebKit Commit Bot 2010-05-11 16:04:53 PDT
All reviewed patches have been landed.  Closing bug.