Bug 33235 – Padding in popup menu gets lost with styled <select> in Windows

• Component: Forms
• Status: RESOLVED FIXED

[Brian Weinstein]

If a select has -webkit-appearance off, then the padding will be lost, and the width of the pop up menu will be only as wide as the text, with no padding.

<rdar://7285538>.

[Brian Weinstein]

Created attachment 45938 [details]
[PATCH] Fix

[Eric Seidel (no email)]

Comment on attachment 45938 [details]
[PATCH] Fix

Where's the test?

Looks sane to me, but I'm probably not the perfect reviewer for this.

Seems we need at least a manual test.  Eventually we need to invent a way to layout-test popup windows as they are a large source of manual tests. :(

[Brian Weinstein]

Comment on attachment 45938 [details]
[PATCH] Fix

D'oh, forgot about the manual test, clearing review flag, will post a new patch with manual test.

[Brian Weinstein]

Created attachment 45971 [details]
[PATCH] Fix with manual test

[Brian Weinstein]

Created attachment 45972 [details]
[PATCH] Typo Fix

[Eric Seidel (no email)]

Comment on attachment 45972 [details]
[PATCH] Typo Fix

This still looks sane to me.  I think this probably still should have Adam Roben, or someone who is more familiar with the windows build than I give this a thumbs up.

[Adam Roben (:aroben)]

Comment on attachment 45972 [details]
[PATCH] Typo Fix

There's a comment in PopupMenu::calculatePositionAndSize:

    // Add padding to align the popup text with the <select> text

Does your patch preserve that attribute?

[Brian Weinstein]

(In reply to comment #7)
> (From update of attachment 45972 [details])
> There's a comment in PopupMenu::calculatePositionAndSize:
> 
>     // Add padding to align the popup text with the <select> text
> 
> Does your patch preserve that attribute?

Yes, this only adds additional padding if the width of the text is smaller than the control, which isn't the default select case. The text is still aligned.

[Adam Roben (:aroben)]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 45972 [details] [details])
> > There's a comment in PopupMenu::calculatePositionAndSize:
> > 
> >     // Add padding to align the popup text with the <select> text
> > 
> > Does your patch preserve that attribute?
> 
> Yes, this only adds additional padding if the width of the text is smaller than
> the control, which isn't the default select case. The text is still aligned.

It seems like the text will not be aligned in a case like this:

<select style="width:100px">
<option>some really long text, longer than 100px</option>
</select>

[Adam Roben (:aroben)]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 45972 [details] [details] [details])
> > > There's a comment in PopupMenu::calculatePositionAndSize:
> > > 
> > >     // Add padding to align the popup text with the <select> text
> > > 
> > > Does your patch preserve that attribute?
> > 
> > Yes, this only adds additional padding if the width of the text is smaller than
> > the control, which isn't the default select case. The text is still aligned.
> 
> It seems like the text will not be aligned in a case like this:
> 
> <select style="width:100px">
> <option>some really long text, longer than 100px</option>
> </select>

(Might need a "padding:0" in that style attribute to get the text out of alignment.)

[Brian Weinstein]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (From update of attachment 45972 [details] [details] [details] [details])
> > > > There's a comment in PopupMenu::calculatePositionAndSize:
> > > > 
> > > >     // Add padding to align the popup text with the <select> text
> > > > 
> > > > Does your patch preserve that attribute?
> > > 
> > > Yes, this only adds additional padding if the width of the text is smaller than
> > > the control, which isn't the default select case. The text is still aligned.
> > 
> > It seems like the text will not be aligned in a case like this:
> > 
> > <select style="width:100px">
> > <option>some really long text, longer than 100px</option>
> > </select>
> 
> (Might need a "padding:0" in that style attribute to get the text out of
> alignment.)

The text is in alignment in this case as well, I will upload screenshots.

[Brian Weinstein]

Created attachment 45986 [details]
Screenshot of Example (ToT w/ Patch)

[Brian Weinstein]

Created attachment 45987 [details]
Screenshot of Example (r52733 - Before Patch)

[Brian Weinstein]

Created attachment 45989 [details]
Screenshot of Example w/ WebKit Appearance Off (ToT with Patch)

[Brian Weinstein]

Created attachment 45990 [details]
Screenshot of Example w/ WebKit Appearance Off (r52733)

[Brian Weinstein]

Created attachment 46087 [details]
[Patch] Different Fix

Instead of forcing padding left and right, support padding-right when webkit-appearance is off.

[Adam Roben (:aroben)]

Comment on attachment 46087 [details]
[Patch] Different Fix

What effect does this patch have on <input type=search results=10>? What effect does it have on Mac?

Maybe we should only return paddingRight() when our appearance in MenulistButtonPart?

[Brian Weinstein]

(In reply to comment #17)
> (From update of attachment 46087 [details])
> What effect does this patch have on <input type=search results=10>? What effect
> does it have on Mac?
> 
> Maybe we should only return paddingRight() when our appearance in
> MenulistButtonPart?

If webkit-appearance is off, style()->appearance() returns NoControlPart.

[Brian Weinstein]

(In reply to comment #17)
> (From update of attachment 46087 [details])
> What effect does this patch have on <input type=search results=10>? What effect
> does it have on Mac?
> 
> Maybe we should only return paddingRight() when our appearance in
> MenulistButtonPart?

And Mac never calls clientPaddingRight.

[Adam Roben (:aroben)]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 46087 [details] [details])
> > Maybe we should only return paddingRight() when our appearance in
> > MenulistButtonPart?
> 
> If webkit-appearance is off, style()->appearance() returns NoControlPart.

I see, I was confused by the comment in the code.

[Adam Roben (:aroben)]

Comment on attachment 46087 [details]
[Patch] Different Fix

> +    <script>
> +        // Never have the selects show the selected index, because it garbles the select.
> +        window.addEventListener('load', function() {
> +            var selects = document.querySelectorAll('select');
> +            for (i = 0; i < selects.length; i++) {
> +                selects[i].options.selectedIndex = -1;
> +                selects[i].addEventListener('change', function() {
> +                    this.options.selectedIndex = -1;
> +                });
> +            }
> +        });
> +    </script>

Why is this script needed?

> +    <div class="multi-button">
> +        <p style="display: inline">Select: </p>

Why are the div and p elements needed? (In general, I think it's good to have test cases be as minimal and focused as possible.)

>  int RenderMenuList::clientPaddingRight() const
>  {
> +    if (style()->appearance() == MenulistPart)
> +        // If the appearance is MenulistPart, then the select isn't styled, so
> +        // we want to return the standard endOfLinePadding. We can't add paddingRight() 
> +        // because that value includes the width of the dropdown button so we must use 
> +        // our own endOfLinePadding constant.
> +        return endOfLinePadding;

You need braces around this if, since it contains more than a single line.

I think you need to also check to see if style()->appearance() == MenulistButtonPart. We should return endOfLinePadding in that case, too. You can test this using <select style="border:1px solid black; width:40px">. You should add that to your test case.

We also need to make a similar change to clientPaddingLeft(). For RTL <select>s, the button is on the left! I think we'll need to detect what direction the <select> is following (using style->direction()) and act accordingly. (You should add a test for an RTL <select>, too.)

I think the comment needs to be reworded a little more from its original form in PopupMenuWin. I'd say something like:

// For these appearance values, the theme applies padding to leave room for the drop-down button.
// But leaving room for the button inside the popup menu itself looks strange, so we return a
// small default padding to avoid having a large empty space appear on the side of the popup menu.

r- so we can fix the MenulistButtonPart and RTL issues.

[Brian Weinstein]

(In reply to comment #21)
> (From update of attachment 46087 [details])
> > +    <script>
> > +        // Never have the selects show the selected index, because it garbles the select.
> > +        window.addEventListener('load', function() {
> > +            var selects = document.querySelectorAll('select');
> > +            for (i = 0; i < selects.length; i++) {
> > +                selects[i].options.selectedIndex = -1;
> > +                selects[i].addEventListener('change', function() {
> > +                    this.options.selectedIndex = -1;
> > +                });
> > +            }
> > +        });
> > +    </script>
> 
> Why is this script needed?

It was so the selected option never shows up in the select, just because the select is too small, not really needed though, I can get rid of it.

> > +    <div class="multi-button">
> > +        <p style="display: inline">Select: </p>
> 
> Why are the div and p elements needed? (In general, I think it's good to have
> test cases be as minimal and focused as possible.)

This was a reduction from a test case that was given to me, and I was able to do a lot more reducing than I thought. I'll fix this.

> 
> >  int RenderMenuList::clientPaddingRight() const
> >  {
> > +    if (style()->appearance() == MenulistPart)
> > +        // If the appearance is MenulistPart, then the select isn't styled, so
> > +        // we want to return the standard endOfLinePadding. We can't add paddingRight() 
> > +        // because that value includes the width of the dropdown button so we must use 
> > +        // our own endOfLinePadding constant.
> > +        return endOfLinePadding;
> 
> You need braces around this if, since it contains more than a single line.

I forget the rule when some of the lines are comments. I'll fix that.

> 
> I think you need to also check to see if style()->appearance() ==
> MenulistButtonPart. We should return endOfLinePadding in that case, too. You
> can test this using <select style="border:1px solid black; width:40px">. You
> should add that to your test case.

I'll add another select with these properties.

> We also need to make a similar change to clientPaddingLeft(). For RTL
> <select>s, the button is on the left! I think we'll need to detect what
> direction the <select> is following (using style->direction()) and act
> accordingly. (You should add a test for an RTL <select>, too.)
> 

RTL in popups was turned off in http://trac.webkit.org/changeset/23372 to match Mac, so I don't think this needs to happen.

> I think the comment needs to be reworded a little more from its original form
> in PopupMenuWin. I'd say something like:
> 
> // For these appearance values, the theme applies padding to leave room for the
> drop-down button.
> // But leaving room for the button inside the popup menu itself looks strange,
> so we return a
> // small default padding to avoid having a large empty space appear on the side
> of the popup menu.

I will clean up the comment.

> 
> r- so we can fix the MenulistButtonPart and RTL issues.

I'll upload a new patch with a cleaned up test and MenulistButtonPart fixed. Thanks for the review.

[Brian Weinstein]

Created attachment 46092 [details]
[PATCH] Fix with Adam's Comments

[Adele Peterson]

Comment on attachment 46092 [details]
[PATCH] Fix with Adam's Comments

This looks good.

[Brian Weinstein]

Landed in r52958.