Bug 33235 - Padding in popup menu gets lost with styled <select> in Windows
Summary: Padding in popup menu gets lost with styled <select> in Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2010-01-05 16:18 PST by Brian Weinstein
Modified: 2010-01-07 16:30 PST (History)
5 users (show)

See Also:


Attachments
[PATCH] Fix (2.97 KB, patch)
2010-01-05 16:25 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
[PATCH] Fix with manual test (5.38 KB, patch)
2010-01-06 11:22 PST, Brian Weinstein
bweinstein: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Typo Fix (5.38 KB, patch)
2010-01-06 11:24 PST, Brian Weinstein
bweinstein: commit-queue-
Details | Formatted Diff | Diff
Screenshot of Example (ToT w/ Patch) (22.70 KB, image/png)
2010-01-06 13:35 PST, Brian Weinstein
no flags Details
Screenshot of Example (r52733 - Before Patch) (23.07 KB, image/png)
2010-01-06 13:35 PST, Brian Weinstein
no flags Details
Screenshot of Example w/ WebKit Appearance Off (ToT with Patch) (23.51 KB, image/png)
2010-01-06 13:50 PST, Brian Weinstein
no flags Details
Screenshot of Example w/ WebKit Appearance Off (r52733) (23.19 KB, image/png)
2010-01-06 13:51 PST, Brian Weinstein
no flags Details
[Patch] Different Fix (5.96 KB, patch)
2010-01-07 14:37 PST, Brian Weinstein
aroben: review-
bweinstein: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Fix with Adam's Comments (5.93 KB, patch)
2010-01-07 15:36 PST, Brian Weinstein
adele: review+
bweinstein: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2010-01-05 16:18:06 PST
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>.
Comment 1 Brian Weinstein 2010-01-05 16:25:07 PST
Created attachment 45938 [details]
[PATCH] Fix
Comment 2 Eric Seidel (no email) 2010-01-06 09:17:35 PST
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. :(
Comment 3 Brian Weinstein 2010-01-06 09:43:17 PST
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.
Comment 4 Brian Weinstein 2010-01-06 11:22:52 PST
Created attachment 45971 [details]
[PATCH] Fix with manual test
Comment 5 Brian Weinstein 2010-01-06 11:24:41 PST
Created attachment 45972 [details]
[PATCH] Typo Fix
Comment 6 Eric Seidel (no email) 2010-01-06 12:03:53 PST
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.
Comment 7 Adam Roben (:aroben) 2010-01-06 12:08:43 PST
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?
Comment 8 Brian Weinstein 2010-01-06 12:11:37 PST
(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.
Comment 9 Adam Roben (:aroben) 2010-01-06 12:27:57 PST
(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>
Comment 10 Adam Roben (:aroben) 2010-01-06 12:28:43 PST
(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.)
Comment 11 Brian Weinstein 2010-01-06 13:20:18 PST
(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.
Comment 12 Brian Weinstein 2010-01-06 13:35:31 PST
Created attachment 45986 [details]
Screenshot of Example (ToT w/ Patch)
Comment 13 Brian Weinstein 2010-01-06 13:35:59 PST
Created attachment 45987 [details]
Screenshot of Example (r52733 - Before Patch)
Comment 14 Brian Weinstein 2010-01-06 13:50:50 PST
Created attachment 45989 [details]
Screenshot of Example w/ WebKit Appearance Off (ToT with Patch)
Comment 15 Brian Weinstein 2010-01-06 13:51:12 PST
Created attachment 45990 [details]
Screenshot of Example w/ WebKit Appearance Off (r52733)
Comment 16 Brian Weinstein 2010-01-07 14:37:13 PST
Created attachment 46087 [details]
[Patch] Different Fix

Instead of forcing padding left and right, support padding-right when webkit-appearance is off.
Comment 17 Adam Roben (:aroben) 2010-01-07 14:45:18 PST
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?
Comment 18 Brian Weinstein 2010-01-07 14:48:14 PST
(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.
Comment 19 Brian Weinstein 2010-01-07 14:48:38 PST
(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.
Comment 20 Adam Roben (:aroben) 2010-01-07 15:07:46 PST
(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.
Comment 21 Adam Roben (:aroben) 2010-01-07 15:18:54 PST
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.
Comment 22 Brian Weinstein 2010-01-07 15:23:19 PST
(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.
Comment 23 Brian Weinstein 2010-01-07 15:36:23 PST
Created attachment 46092 [details]
[PATCH] Fix with Adam's Comments
Comment 24 Adele Peterson 2010-01-07 16:21:59 PST
Comment on attachment 46092 [details]
[PATCH] Fix with Adam's Comments

This looks good.
Comment 25 Brian Weinstein 2010-01-07 16:30:42 PST
Landed in r52958.