RESOLVED FIXED 38749
[Chromium] Pipe RTL info into WebPopupMenuInfo
https://bugs.webkit.org/show_bug.cgi?id=38749
Summary [Chromium] Pipe RTL info into WebPopupMenuInfo
Avi Drissman
Reported 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.
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-
Update (3.60 KB, patch)
2010-05-10 10:57 PDT, Avi Drissman
no flags
Alignment it is. (3.31 KB, patch)
2010-05-10 13:50 PDT, Avi Drissman
no flags
Avi Drissman
Comment 1 2010-05-07 07:57:01 PDT
Created attachment 55374 [details] A patch to fill in a "right aligned" attr in the menu info
Jeremy Moskovich
Comment 2 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.
Avi Drissman
Comment 3 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.
David Levin
Comment 4 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.
Darin Fisher (:fishd, Google)
Comment 5 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.
Xiaomei Ji
Comment 6 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
Avi Drissman
Comment 7 2010-05-10 10:57:25 PDT
Avi Drissman
Comment 8 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!)
Xiaomei Ji
Comment 9 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.
Avi Drissman
Comment 10 2010-05-10 13:50:12 PDT
Created attachment 55599 [details] Alignment it is.
WebKit Commit Bot
Comment 11 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>
WebKit Commit Bot
Comment 12 2010-05-11 16:04:53 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.