Bug 53632 - [Chromium] Option text in select popup does not align with menulist button text
Summary: [Chromium] Option text in select popup does not align with menulist button text
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-02 15:24 PST by xiyuan
Modified: 2011-02-04 19:29 PST (History)
4 users (show)

See Also:


Attachments
Screenshot of problem. (2.95 KB, image/png)
2011-02-02 15:24 PST, xiyuan
no flags Details
Proposed patch. (1.65 KB, patch)
2011-02-02 15:54 PST, xiyuan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description xiyuan 2011-02-02 15:24:03 PST
Created attachment 80978 [details]
Screenshot of problem.

Menulist button has addition paddings from m_innerBlock besides css paddings and PopupListBox should include those paddings as well.
Comment 1 xiyuan 2011-02-02 15:54:52 PST
Created attachment 80987 [details]
Proposed patch.
Comment 2 xiyuan 2011-02-02 16:18:44 PST
Comment on attachment 80987 [details]
Proposed patch.

This patch has side effect that because the m_innerBlock padding include the space for scrollbar and PopupListBox::layout might add space for scrollbar again.
Comment 3 xiyuan 2011-02-02 16:50:43 PST
Comment on attachment 80987 [details]
Proposed patch.

Resurrect the patch as we are protected from counting scrollbar width twice in RenderMenuList::clientPaddingRight which returns fixed endOfLinePadding.
Comment 4 Tony Chang 2011-02-04 09:51:57 PST
Comment on attachment 80987 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=80987&action=review

> Source/WebCore/rendering/RenderMenuList.cpp:489
> +    return paddingLeft() + m_innerBlock->paddingLeft();

Won't this change the offset for all webkit platforms?  Is that intentional?
Comment 5 xiyuan 2011-02-04 10:32:13 PST
(In reply to comment #4)
> (From update of attachment 80987 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80987&action=review
> 
> > Source/WebCore/rendering/RenderMenuList.cpp:489
> > +    return paddingLeft() + m_innerBlock->paddingLeft();
> 
> Won't this change the offset for all webkit platforms?  Is that intentional?

This should affect Windows and Linux. Both platforms have the alignment problem.

Mac probably will not be affected by this because its popup shows up via showExternal which should use native widget for the popup.
Comment 6 Tony Chang 2011-02-04 11:05:18 PST
Comment on attachment 80987 [details]
Proposed patch.

If it only affects Windows and Linux, this is fine.

For reference, IE & Firefox Win aligns the text in popups.  This is consistent with native menu lists on Windows. GTK+ menu lists also align the text (actually, GTK+ drop downs are like OSX menu lists where the text doesn't move but the options appear above/below the currently selected item).
Comment 7 WebKit Commit Bot 2011-02-04 18:19:56 PST
Comment on attachment 80987 [details]
Proposed patch.

Clearing flags on attachment: 80987

Committed r77716: <http://trac.webkit.org/changeset/77716>
Comment 8 WebKit Commit Bot 2011-02-04 18:20:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Commit Bot 2011-02-04 19:29:07 PST
The commit-queue encountered the following flaky tests while processing attachment 80987 [details]:

inspector/elements-panel-xhtml-structure.xhtml bug 53835 (authors: apavlov@chromium.org and pfeldman@chromium.org)
The commit-queue is continuing to process your patch.