418294 - [10.5] Text pop-up <select> is not vertically centered (1px too low)

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Johan Ronsse]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3

It's one pixel off, not in the center.

Reproducible: Always

Steps to Reproduce:
Render this HTML with Firefox 3 Beta 3:

<select style="font-size: 12px;">
	<option>Lorem ipsum</option>
</select>

<select style="font-size: 11px;">
	<option>Lorem ipsum</option>
</select>
Actual Results:  
Wrong rendering

Expected Results:  
Display the text exactly in the middle

See Safari 3 for correct implementation.

Comment 1

[philippe (part-time)]

Can you attach a screenshot of what you see ? (using the 'Add an attachment' link)

Comment 2

[Johan Ronsse]

Attachment: Example

Comment 3

[Johan Ronsse]

Attachment: This is the correct rendering (This is a screenshot from Webkit, NOT firefox 3)

Comment 4

[Johan Ronsse]

I added 2 attachments, example shows the bug, the other shows how it should be.

Comment 5

[philippe (part-time)]

Thanks,
I don't see that on 10.4, but it is indeed the case on 10.5. The descenders ar touching the bottom edge of the widget. The text part is correctly aligned, but the 'aqua layer' is painted too high / or is no tall enough.

Josh, I thought we had already a bug on this, but I can't find it atm.

Comment 6

[philippe (part-time)]

Attachment: test case

Based on comment 0.

On the second row, I added 1 px bottom padding to the select widget, that sort of 'fixes' the issue.

Comment 7

[Johan Ronsse]

Re: p(In reply to comment #6)
> Created an attachment (id=304120) [details]
> test case
> 
> Based on comment 0.
> 
> On the second row, I added 1 px bottom padding to the select widget, that sort
> of 'fixes' the issue.
> 

It's better but not a 100%... it needs to use the calculated line height of the select box

Comment 8

[philippe (part-time)]

So, I didn't find any dupes, or previous mention of this issue -> confirming

(In reply to comment #7)
That 1px bottom-padding is a hack , not a fix :-)

Comment 9

[Josh Aas]

It looks like we have the same problem we had with push buttons - the native drawing routines will only draw particular sizes and nothing else. We're probably going to have to scale just like we do for push buttons.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: add more sizes to testcase

Add some more dropdown sizes to the testcase

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: test program

Would be helpful if someone could compile this and run on 10.4, and screenshot the result.. I only have 10.5 handy here, just want to make sure it's not affected.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: after all that, a patch

This fixes things for me.. essentially, the result of the test program on 10.5 is that HITheme will happily scale things from 20px height and up, drawing the actual border 1px below the given rectangle (but using up 2 pixels below, to draw a shadow border).  The original problem here was that we were decreasing the height by 2px instead of 1, so the dark border was being drawn too high.

For below 20px, HITheme will only draw 18px or 15px, using whatever the nearest value is to the requested height.  Note that there's no way to natively draw a 20px high "real" border, since specifying 20px will get you 21px, and specifying 19px will get you 18px.  So, for desired heights 20 or below, that are not 15 or 18, we draw to an offscreen context and scale.  This looks pretty good in practice.

Comment 13

[Josh Aas]

Attachment: focus ring problem, pic 1

Comment 14

[Josh Aas]

Comment on attachment 308763 [details] [diff] [review]
after all that, a patch

This patch breaks focus rings.

>@@ -1438,7 +1506,6 @@ nsNativeThemeCocoa::GetMinimumWidgetSize
>     {
>       SInt32 popupHeight = 0;
>       ::GetThemeMetric(kThemeMetricPopupButtonHeight, &popupHeight);
>-      aResult->SizeTo(0, popupHeight);
>       break;
>     }

I don't doubt that the theme metric here is bogus, but just removing the result assignment is incorrect. We need to replace it with a hard-coded value (probably the right thing to do) and get rid of the then-useless ::GetThemeMetric call.

Comment 15

[Josh Aas]

Comment on attachment 308763 [details] [diff] [review]
after all that, a patch

+    CGContextScaleCTM (ctx, 1.0f, -1.0f);

Also space between the function name and opening arg "(" should be removed. Happens a bunch of places.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #14)
> (From update of attachment 308763 [details] [diff] [review])
> This patch breaks focus rings.
> 
> >@@ -1438,7 +1506,6 @@ nsNativeThemeCocoa::GetMinimumWidgetSize
> >     {
> >       SInt32 popupHeight = 0;
> >       ::GetThemeMetric(kThemeMetricPopupButtonHeight, &popupHeight);
> >-      aResult->SizeTo(0, popupHeight);
> >       break;
> >     }
> 
> I don't doubt that the theme metric here is bogus, but just removing the result
> assignment is incorrect. We need to replace it with a hard-coded value
> (probably the right thing to do) and get rid of the then-useless
> ::GetThemeMetric call.

Yeah, this was a typo, I meant to leave everything here as-is.  It's odd that it doesn't seem to actually affect anything, which is weird, but that's to fix another day.  Things seem to work fine even with the height coming from GetThemeMetric.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch

Updated patch, fixes focus ring stuff and whatnot.

Comment 18

[Josh Aas]

Comment on attachment 309561 [details] [diff] [review]
updated patch

The dropdown buttons now render upside-down on Mac OS X 10.5. The horizontal shadow goes on the bottom of the control. See the top of any bugzilla bug report.

Comment 19

[Josh Aas]

In case it only renders upside down depending on the scaling path, I see the upside down problem with a frame height of 20.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated, again

Bah, I was even staring at it to make sure that the flip was right, but the scaling was throwing me off.

Comment 21

[Josh Aas]

Comment on attachment 310032 [details] [diff] [review]
updated, again

+  PRBool needsScaling = PR_FALSE;
+  int drawWidth, drawHeight;

These can get declared further down, above the block where they are first used. Because it isn't obvious that the ints are initialized before use, please init them to 0.

+      else if (drawFrame.size.height == 18 ||
+               drawFrame.size.height == 15)
+      {
+        // these two get drawn correctly, don't munge anything
+        needsScaling = PR_FALSE;

PR_FALSE is the default value for needsScaling. Please rearrange the logic so that this else if statement that sets it to false again isn't necessary. Like this:

// Don't do anything for sizes that draw naturally
if (height != 18 && height != 15) {
  if (height > 20) {
    ...
  }
  else {
    needsScaling = PR_TRUE;
    ...
  }
}

I still have to test on Tiger.

Comment 22

[Josh Aas]

Comment on attachment 310032 [details] [diff] [review]
updated, again

+      // leave things alone on Tiger
+      drawFrame.size.height -= 2;

This patch doesn't fix this bug on Tiger. The above needs to subtract 1, not 2. That fixes things on Tiger.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: one more time

updated

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in.