427122 - REGRESSION: Padding on select elements is ignored

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Ojan Vafai]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/525.13 (KHTML, like Gecko) Version/3.1 Safari/525.13
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

<select style="padding:4px;"><option>foo</option></select>

Should have padding. This worked correctly in beta 4 and regressed in beta 5.

Firefox 2 respects the padding, but clips the button incorrectly.

Reproducible: Always

Steps to Reproduce:
Load a page with this html: <select style="padding:4px;"><option>foo</option></select>
Actual Results:  
The padding is ignored.

Expected Results:  
The padding should be respected.

Comment 1

[Peter Kasting]

Late-breaking regression, requesting blocking so it's on drivers' radar

Comment 2

[Brian Polidoro]

That's strange.  If I add a background color to the select then the padding is added.  

Comment 3

[Brian Polidoro]

0304 broken
0303 ok

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-03-03+04%3A00%3A00&maxdate=2008-03-04+06%3A00%3A00&cvsroot=%2Fcvsroot

Sure seems like bug 420580.

Comment 4

[Frank Wein [:mcsmurf]]

Simon: Can you take a look at this bug here?

Comment 5

[Simon Bünzli]

That's rather vlad's call: Properly fixing bug 422488 (which is blocking1.9+) should fix this issue as well.

Comment 6

[Ojan Vafai]

I can see that the two bugs are related and perhaps have the same fix. I just want to reiterate that unlike bug 422488, which regressed from Firefox 2, this bug worked correctly in Firefox 3 beta 4 and stopped working in beta 5.

Comment 7

[Simon Bünzli]

(In reply to comment #6)
Actually both bug 420580 and bug 422488 are a regression from bug 418552 which happened between Firefox 3.0 Beta 3 and Beta 4. This bug is a side effect from bug 420580 which apparently didn't even completely fix the original regression. The solution is thus to revert the changes from both bug 420580 and bug 418552 - which is exactly what should happen in bug 422488.

Comment 8

[Damon Sicore (:damons)]

If this isn't fixed by bug 422488 (which is still blocking and blocks this issue), please renominate.  At this point, however, we shouldn't hold the release for this.  

wanted1.9.0.x+

Comment 9

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

Attachment: don't force padding if user has specified padding

Don't set padding if user has specified padding; have I mentioned I hate our native theme system?

This patch also has a fix for bug 422488, though I'm pretty sure the theme is at fault there.. but given that the OS itself draws things the way we used to, we may as well do that on XP.

Comment 10

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

(Swapping blocking with this and bug 422488, though this patch fixes both)

Comment 11

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

Comment on attachment 314469 [details] [diff] [review]
don't force padding if user has specified padding

Moving r? to stuart, sr? to roc (just because i'm wondering if the style thing i'm doing is valid)

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 314469 [details] [diff] [review]
don't force padding if user has specified padding

>+  const nsStylePadding *userStylePadding = aFrame ? aFrame->GetStylePadding() : NULL;
>+  nsMargin userPadding;
>+  PRBool hasUserPadding = userStylePadding ? userStylePadding->GetPadding(userPadding) : PR_FALSE;
>+
>+  if (mIsVistaOrLater && !hasUserPadding) {

[2008-04-09 16:47:16] <vlad> dbaron: ping
[2008-04-09 16:48:04] <vlad> dbaron: is https://bugzilla.mozilla.org/attachment.cgi?id=314469&action=diff#a/widget/src/windows/nsNativeThemeWin.cpp_sec3 a good way to check if there is user-specified padding on a frame?
[2008-04-09 16:51:00] <dbaron> vlad, well, you could add it if you need it  -- see nsPresContext::HasAuthorSpecifiedBorderOrBackground
[2008-04-09 16:51:13] <dbaron> vlad, also, nsIFrame::GetStylePadding will never return null
[2008-04-09 16:51:27] <dbaron> vlad, we store backstop default values in reserve for allocation failure
[2008-04-09 16:51:36] <vlad> dbaron: yeah, I saw HasAuthor*, wasn't sure if I should add a new one
[2008-04-09 16:51:41] <vlad> ah ok
[2008-04-09 16:51:58] <vlad> seems like GetPadding's return value would be good enough
[2008-04-09 16:52:05] <dbaron> eh?
[2008-04-09 16:52:06] <vlad> since at some point in the future I may need to take the user padding into account
[2008-04-09 16:52:21] <vlad> I mean using GetPadding instead of creating HasAuthorSpecifiedPadding
[2008-04-09 16:52:37] <dbaron> not sure what you mean
[2008-04-09 16:52:51] <dbaron> you're assuming particular UA and user style sheets that way, though
[2008-04-09 16:53:03] <dbaron> that proved quite painful in the past when those sheets were modified
[2008-04-09 16:53:09] <vlad> hmm, what do you mean?
[2008-04-09 16:53:59] <dbaron> whenever people changed forms.css they'd have to change a bunch of C++ code in native theme implementations that tested for modifications by checking "is the value 2px, since we know that's the default X"?
[2008-04-09 16:54:16] <vlad> oh, no, I don't actually check the value
[2008-04-09 16:54:29] <dbaron> vlad, then what do you check?
[2008-04-09 16:54:32] <vlad> I was just saying in the future I might want to do things like "user requested 5px, so we need to say 7px here to get our 2px that we need"
[2008-04-09 16:54:42] <vlad> I check the return value of nsStylePadding::GetPadding
[2008-04-09 16:54:48] <dbaron> vlad, oh, whether GetPadding returns true or false just tells you whether there was % padding or not
[2008-04-09 16:54:52] <vlad> oh.
[2008-04-09 16:55:01] <vlad> then that's not really what I want :)
[2008-04-09 16:55:27] <dbaron> vlad, and maybe some about when you're calling it
[2008-04-09 16:55:43] <dbaron> see nsStylePadding::RecalcData
[2008-04-09 16:55:47] <vlad> so I guess I need create HasAuthorSpecifiedPadding

Comment 13

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

Attachment: updated

Updated based on irc discussion.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 314750 [details] [diff] [review]
updated

>+  nsCSSValue* borderValues[] = {
>     &marginData.mBorderColor.mTop,
>     &marginData.mBorderStyle.mTop,
>     &marginData.mBorderWidth.mTop,
>@@ -5186,6 +5201,32 @@ nsRuleNode::HasAuthorSpecifiedBorderOrBa
>     &marginData.mBorderStyle.mLeft,
>     &marginData.mBorderWidth.mLeft
>   };

Could you add &marginData.mBorder{Start,End}{Width,Color,Style} to this array as well, or add an XXX comment that it should be done?  (We should really have tests for that case too...)

>+  nsCSSValue* paddingValues[] = {
>+    &marginData.mPadding.mTop,
>+    &marginData.mPadding.mRight,
>+    &marginData.mPadding.mBottom,
>+    &marginData.mPadding.mLeft

Also add &marginData.mPaddingStart and &marginData.mPaddingEnd .

Seems like your reftest should fail if MOZ_WIDGET_TOOLKIT!="windows"

You could also put it in layout/reftest/native-theme/ if you want.

r=dbaron with that.  Sorry for the delay.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 314750 [details] [diff] [review]
updated

er, forgot to mark review+ when I wrote the long comment

Comment 16

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

(In reply to comment #14)
> (From update of attachment 314750 [details] [diff] [review])
> >+  nsCSSValue* borderValues[] = {
> >     &marginData.mBorderColor.mTop,
> >     &marginData.mBorderStyle.mTop,
> >     &marginData.mBorderWidth.mTop,
> >@@ -5186,6 +5201,32 @@ nsRuleNode::HasAuthorSpecifiedBorderOrBa
> >     &marginData.mBorderStyle.mLeft,
> >     &marginData.mBorderWidth.mLeft
> >   };
> 
> Could you add &marginData.mBorder{Start,End}{Width,Color,Style} to this array
> as well, or add an XXX comment that it should be done?  (We should really have
> tests for that case too...)
> 
> >+  nsCSSValue* paddingValues[] = {
> >+    &marginData.mPadding.mTop,
> >+    &marginData.mPadding.mRight,
> >+    &marginData.mPadding.mBottom,
> >+    &marginData.mPadding.mLeft
> 
> Also add &marginData.mPaddingStart and &marginData.mPaddingEnd .

Hmm, can do.. do we really want to change the potential behaviour though, even if it's correct?  I'm more tempted to XXX it and file a new bug (including fixing it on Mac, where it just doesn't work right at all).

> Seems like your reftest should fail if MOZ_WIDGET_TOOLKIT!="windows"

Linux seemed to handle padding correctly (because it wasn't overriding it in the gtk native theme code); Mac overrides it, though.

> You could also put it in layout/reftest/native-theme/ if you want.

Will do.

Comment 17

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

Attachment: updated

Carrying review over and requesting approval.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 316360 [details] [diff] [review]
updated

a1.9=beltzner

Comment 19

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

Checked in; filed bug 429900 on getting the additional values in for borders post-1.9.

Comment 20

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

Sigh, reopened; reftest failures, though the 'failure' was "nothing rendered", which I don't quite understand.

Comment 21

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

Checked in, I copied in the wrong test files first time around :p

Comment 22

[Recall]

Vlad, I believe your patch cause a Vista only issue in bug 431081 for missing selection boxes.