WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 55571
[details]
Update
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.
Top of Page
Format For Printing
XML
Clone This Bug