Bug 38749 – [Chromium] Pipe RTL info into WebPopupMenuInfo

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Avi Drissman]

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.

[Avi Drissman]

Created attachment 55374 [details]
A patch to fill in a "right aligned" attr in the menu info

[Jeremy Moskovich]

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.

[Avi Drissman]

(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.

[David Levin]

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.

[Darin Fisher (:fishd, Google)]

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.

[Xiaomei Ji]

(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

[Avi Drissman]

Created attachment 55571 [details]
Update

[Avi Drissman]

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!)

[Xiaomei Ji]

@@ -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.

[Avi Drissman]

Created attachment 55599 [details]
Alignment it is.

[WebKit Commit Bot]

Comment on attachment 55599 [details]
Alignment it is.

Clearing flags on attachment: 55599

Committed r59179: <http://trac.webkit.org/changeset/59179>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.