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.
Created attachment 55374 [details] A patch to fill in a "right aligned" attr in the menu info
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.
(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 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 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.
(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
Created attachment 55571 [details] Update
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!)
@@ -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.
Created attachment 55599 [details] Alignment it is.
Comment on attachment 55599 [details] Alignment it is. Clearing flags on attachment: 55599 Committed r59179: <http://trac.webkit.org/changeset/59179>
All reviewed patches have been landed. Closing bug.