[webkit-reviews] review denied: [Bug 33235] Padding in popup menu gets lost with styled <select> in Windows : [Attachment 46087] [Patch] Different Fix

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 33235: Padding in popup menu gets lost with styled <select> in Windows
https://bugs.webkit.org/show_bug.cgi?id=33235

Attachment 46087: [Patch] Different Fix
https://bugs.webkit.org/attachment.cgi?id=46087&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +    <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.