Bug 53942 – [Chromium] Select popup box does not align with select button when appearance is not "menulist".

• Component: New Bugs
• Status: RESOLVED FIXED

[xiyuan]

Created attachment 81520 [details]
Test case.

When a select element's appearance is not "menulist", we use the css padding to calculdate popup width (since https://bugs.webkit.org/show_bug.cgi?id=33235). And this caused the popup box is wider than the select button when appearance is not menulist. And use has no way to fix this via css padding.

[xiyuan]

Created attachment 81523 [details]
Proposed patch.

[xiyuan]

Created attachment 81525 [details]
Fix typo in manual test name.

[Tony Chang]

Comment on attachment 81525 [details]
Fix typo in manual test name.

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

just some small nits

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1272
> +const int minEndOfLinePadding = 2;

static?  Also, should this be at the top of the file with the other constants?  The other constants are named incorrectly (k prefix), but we can fix that later.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1275
> +    bool rightAligned = m_popupClient->menuStyle().textDirection() == RTL;

Nit: rightAligned -> isRightAligned

[Tony Chang]

Comment on attachment 81525 [details]
Fix typo in manual test name.

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

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1349
> +        // Use minEndOfLinePadding when there is a scrollbar.

Why does a scrollbar cause us to need 2 extra pixels?

[xiyuan]

Comment on attachment 81525 [details]
Fix typo in manual test name.

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

>> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1272

> 
> static?  Also, should this be at the top of the file with the other constants?  The other constants are named incorrectly (k prefix), but we can fix that later.

Done.

>> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1275

> 
> Nit: rightAligned -> isRightAligned

Done

>> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1349
>> +        // Use minEndOfLinePadding when there is a scrollbar.
> 
> Why does a scrollbar cause us to need 2 extra pixels?

Updated the comment to make it more clear.

It's not that we need the 2 extra pixels. We replace the lineEndPaddingWidth space with the minimum 2 pixel space when there is a scrollbar. In this way, scrollbar will stay as much as possible inside the line end padding space. User could use a big enough "padding-right" (>= scrollbarWidth + 2px) to make the popup listbox align with the select button.

This is because https://bugs.webkit.org/show_bug.cgi?id=33235 makes popup respect the "padding-right". It works well when there is no scrollbar. But when popup has a scrollbar, the popup is always wider than the select button no matter what padding-right value you use. You can see that from the test case page.

[xiyuan]

Created attachment 81550 [details]
Address tony's comments @3,4

[Tony Chang]

Comment on attachment 81550 [details]
Address tony's comments @3,4

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

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1353
> +        // Use kMinEndOfLinePadding when there is a scrollbar so that we use
> +        // as much as (lineEndPaddingWidth - kMinEndOfLinePadding) padding
> +        // space for scrollbar and allow user to use CSS padding to make the
> +        // popup listbox align with the select element.
> +        paddingWidth = paddingWidth - lineEndPaddingWidth + kMinEndOfLinePadding;

This comment is much more useful.  Thanks!

[WebKit Commit Bot]

Comment on attachment 81550 [details]
Address tony's comments @3,4

Clearing flags on attachment: 81550

Committed r77907: <http://trac.webkit.org/changeset/77907>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.