396984 - Border width on GTK text input fields behaving strangely in default (2px) case.

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase

Steps to Reproduce:
 * Load testcase in Linux trunk build.

Expected results: 

  "2px" column and "auto" column should have top/right/bottom/left border-widths defaulting to 2px, as in the other columns.  

Actual results:
   "2px" column and "auto" columns have border-widths defaulting to 3px.

(You can see the difference visually, too -- those columns' text-input fields look weird in comparison to the other text-input fields.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Expected result (screenshot, Firefox 2)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Actual result (screenshot, trunk)

Comment 3

[Michael Ventnor]

I think there is something somewhere that assumes 2px is the same as the native textbox's border (so IsWidgetStyled fails), but then when it asks for GetBorderWidth from the native theme code it gets back 3px.

I'm not sure if this regression is my doing. Try a build just before native theming was turned on, and one just after.

Comment 4

[Michael Ventnor]

Gotcha.

#widget/src/xpwidgets/nsNativeTheme.cpp:
Line 61
nsMargin nsNativeTheme::sTextfieldBorderSize(2, 2, 2, 2);

Yep, as I suspected, hardcoded assumptions. And just to make my life more difficult, this is in x-platform code. Roc, do you have any suggestions on how I could fix this? I'm at Uni at the moment so I can't look at it in great detail right now.

Comment 5

[Michael Ventnor]

Actually, after another look I think I might be able to fix this if I can copy code from there to nsNativeThemeGTK.cpp.

Comment 6

[Michael Ventnor]

Dammit, this won't work. 2px is the specified border value in forms.css. So Gecko assumes that every platform's native textboxes have a border 2px wide, and if it is different it thinks the widget is styled and will use the generic widgets. I'm guessing the 3px value underneath is the right one which is what is returned from GetWidgetBorder.

I'm at a loss here. I can't think of a fix that won't spoil it for other platforms. Any ideas, Roc?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks like forms.css is preprocessed, can you slip a %if ... %endif block in there for GTK2? But I'm not sure how you can fix it that way.

Comment 8

[Michael Ventnor]

Attachment: Patch

I don't like this but I think its our only option at this point. Every theme I've tested returns 3px on every widget, this includes Clearlooks and friends, Crux and some Murrina themes.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Screenshot of trunk with patch applied

Comment 10

[Daniel Holbert [:dholbert]]

Looks like the patch fixed the border-width issues, judging by the numbers in the testcase.

The "3px" and "auto" cases do still look rounded, whereas all the other testcases look square-ish... But that's probably a separate issue.  I'll probably file another bug for that, unless anyone already knows what's going on there.

Comment 11

[Daniel Holbert [:dholbert]]

Patch checked in.  Resolving as fixed.

Checking in layout/style/forms.css;
/cvsroot/mozilla/layout/style/forms.css,v  <--  forms.css
new revision: 3.139; previous revision: 3.138
done
Checking in widget/src/xpwidgets/nsNativeTheme.cpp;
/cvsroot/mozilla/widget/src/xpwidgets/nsNativeTheme.cpp,v  <--  nsNativeTheme.cpp
new revision: 1.40; previous revision: 1.39
done

Comment 12

[Michael Ventnor]

(In reply to comment #10)
> Looks like the patch fixed the border-width issues, judging by the numbers in
> the testcase.
> 
> The "3px" and "auto" cases do still look rounded, whereas all the other
> testcases look square-ish... But that's probably a separate issue.  I'll
> probably file another bug for that, unless anyone already knows what's going on
> there.
> 

When the specified border matches the border of the GTK native widgets, Gecko will use native GTK widgets. Thats what the rounded ones are. When special styles are used, Gecko will fall-back to the generic widgets.

Comment 13

[Daniel Holbert [:dholbert]]

> When the specified border matches the border of the GTK native widgets, Gecko
> will use native GTK widgets. Thats what the rounded ones are. When special
> styles are used, Gecko will fall-back to the generic widgets.
> 

Ok, thanks.  I won't file a bug on that, then.

Comment 14

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

I'm trying to figure out why this patch was needed.

For a start -- I disagree with the statement of the bug in comment 0 and comment 1 -- the behavior described as a bug is exactly what the IsWidgetStyled code was trying to do -- make the textfields that have the default border (or 2px) get the native theme style.

So what was this fixing?  And why was so much code needed to do it?

Comment 15

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

I'd also note that if all it was fixing is making 3px behave the same as unspecified rather than 2px, then I think that's also an incorrect change -- I think we're better off having consistency between platforms for which specified width is the same as the default than having that default match each platform.

(Frankly, a correct IsWidgetStyled would return true if the author specified anything, even if it happened to be the same as the default.  The current IsWidgetStyled is a hack.)

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the issue in comment #0 is that if the author specifies a border of 2px, they should get a border of 2px. Not 3px.

> I think we're better off having consistency between platforms for which
> specified width is the same as the default than having that default match each
> platform.

That means we require all platforms to have the same default. Which means we can't honour the GTK2 theme border width, which is not 2px. So we have to pick one of:

1) WONTFIX this bug
2) allow the default border width of textboxes to vary across platforms (i.e. keep this fix)
3) alter the theme rendering of GTK2 textboxes so that the border is only 2px

Do you recommend #3?

Comment 17

[Michael Ventnor]

#3 is not possible with GTK, afaik.

Comment 18

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

How does it mean we can't honor the GTK2 theme border width?  The border width from nsITheme will override the one from the style system as long as IsWidgetStyled returns false.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

ok, so you're picking option #1, WONTFIX this bug?

Comment 20

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

I think so.  All this bug did is move the native-theme look from the 2px specified width to the 3px specified width, right?  As far as I can tell, "strange" in the original description means "themed".

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to comment #20)
> All this bug did is move the native-theme look from the 2px
> specified width to the 3px specified width, right? 

I think so, basically.  It does make us honor the author's requested 2px specified width, as roc said in Comment #16.

> As far as I can tell,
> "strange" in the original description means "themed".

Correct, I was mistaken about the "weird"ness -- when I filed the bug, I hadn't realized that there were separate themed and unthemed widgets.


I still think it's a bit counterintuitive for an author to request a 2px border and end up with a 3px one.  But if a 2px border request is likely to actually be just a request for a native widget, then I guess it makes sense to leave it as it was before this bug.

So, dbaron, should we back this patch out?

Comment 22

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

I think we should back it out -- I'd rather not have all these platform-specific ifdefs.  I'm also not convinced that the borders on my theme look like 3px rather than 2px (really, I think they look like 1px, but I recognize there's an extra pixel of shading).  And I'd rather keep the ugliness that needs to be in 2 places (since IsWidgetStyled needs to match forms.css right now) as simple as possible.

Comment 23

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

Actually, I guess I'm ok with this in.  Authors really shouldn't be able to rely on either way working.

Although -- are there cases when the native theming fails and we actually draw using the styles in forms.css?  If so, we probably want 2px for those.  And I'm still not sure the complexity would be worth it if we did want this.

Comment 24

[Michael Ventnor]

If you want this to be backed out I'm fine with that.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the original issue here --- authors asking for a 2px border and getting a 3px border --- is pretty bad, so I'm hesitant to back out without a better suggestion on the table.

How hard would it be to hook IsWidgetStyled up to the style system so we can detect whether there are any relevant non-UA CSS rules applying?

If that's too hard, how about defining a new length value -moz-native-border-width whose computed value is GetNativeBorder().top, and have IsWidgetStyled compare GetNativeBorder to the style value?

Comment 26

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

(In reply to comment #25)
> I think the original issue here --- authors asking for a 2px border and getting
> a 3px border --- is pretty bad, so I'm hesitant to back out without a better
> suggestion on the table.

But that's a general bug that applies to any themable control, when authors specify the default value.  I'm hesitant to add a good bit of code to fix it in just one specific case.

> How hard would it be to hook IsWidgetStyled up to the style system so we can
> detect whether there are any relevant non-UA CSS rules applying?

See bug 240117.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Although -- are there cases when the native theming fails and we actually draw
> using the styles in forms.css? 

Any time the author styles anything.  E.g. if they set the color on the button we'll now do different rendering than we used to (and different on different platforms).

We should remember to at least back this out once we fix bug 240117.

Comment 28

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

OK, I guess I think we should back this out -- I'd rather not have the unthemed style vary between platforms.  Fixing bug 240117 isn't too hard (I just wrote half the patch, in fact).  I could do it for 1.9 pretty easily if you think it's important.

Comment 29

[Daniel Holbert [:dholbert]]

Ok, patch backed out.

Checking in widget/src/xpwidgets/nsNativeTheme.cpp;
/cvsroot/mozilla/widget/src/xpwidgets/nsNativeTheme.cpp,v  <--  nsNativeTheme.cpp
new revision: 1.41; previous revision: 1.40
done
Checking in layout/style/forms.css;
/cvsroot/mozilla/layout/style/forms.css,v  <--  forms.css
new revision: 3.140; previous revision: 3.139
done

Comment 30

[Daniel Holbert [:dholbert]]

Reopening, depending on bug 240117

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #26)
> (In reply to comment #25)
> > I think the original issue here --- authors asking for a 2px border and getting
> > a 3px border --- is pretty bad, so I'm hesitant to back out without a better
> > suggestion on the table.
> 
> But that's a general bug that applies to any themable control, when authors
> specify the default value.  I'm hesitant to add a good bit of code to fix it in
> just one specific case.

It's only a bug when the theme width doesn't match the forms.css width, and my impression is that themes don't change widths all that often, if at all (since that would break apps that use (the equivalent of) absolute-positioned widgets, which is a lot of apps), so I'm not aware of any other actual bugs of this type that we've found.

But OK.

> > How hard would it be to hook IsWidgetStyled up to the style system so we can
> > detect whether there are any relevant non-UA CSS rules applying?
> 
> See bug 240117.

OK. That would be great to have, it would make a lot of code less fragile. But right now I can't argue for blocking status.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(If we find a real site that sets border:2px and breaks because it doesn't get it, then I'd argue for blocking status)

Comment 33

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

Fixed by checkin of bug 240117.