184359 - "border: none" on textbox shrinks textbox without removing border

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jesse Ruderman]

Setting "border: none" on a textbox shrinks the textbox but does not remove the
border.  This causes text typed into the textbox to only be partly visible. 
Strangely, setting "border: 0px solid" works as expected.

I'm using Phoenix 0.5 and Mozilla 12/07 on Windows XP.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Worksforme, linux trunk build 2002-12-07-22.  Sounds like a problem with the
"themed" XP controls only...

To hyatt. We have dozens of bugs on these controls already; how about fixing
them, eh?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Shouldn't we drop the theming when the border is styled, btw?

Comment 3

[Boris Zbarsky [:bzbarsky]]

*** Bug 186376 has been marked as a duplicate of this bug. ***

Comment 4

[José Jeria]

This happends in both XP and Classic theme on XP.

Comment 5

[Boris Zbarsky [:bzbarsky]]

*** Bug 187068 has been marked as a duplicate of this bug. ***

Comment 6

[Boris Zbarsky [:bzbarsky]]

To tim in the hopes of actually getting traction on this....  Too bad hyatt 
broke all this shit and has no plans to fix it...

Comment 7

[John Keiser (jkeiser)]

Dropping theming when border is changed is eminently reasonable.

Comment 8

[Teal Bauer [:teal]]

Generally, CSS "border" rules, except for solid/dashed/dotted, just don't work
right on "native look" widgets.
-moz-appearance: none; should be set for all input elements by default.

Comment 9

[Boris Zbarsky [:bzbarsky]]

No, -moz-appearance should be set exactly as it is now.  It should compute 
to "none" if the page sets a border.

Comment 10

[Tim Hill]

Attachment: Patch to nsNativeThemeWin

This patch fixes all the recent theme related form control bugs.  It checks the
computed style of buttons and text fields against the default style, and drops
theming if they're at all different in border/background size, color, or style.
 It also fixes a bug with the appearance of disabled/readonly text fields.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 110537 [details] [diff] [review]
Patch to nsNativeThemeWin

> +// Determine if a widget differs from the default style and should drop theming

"Determine whether"

>+      // Get default CSS style values for widget
>+      // (these need to match the values in forms.css)

Please add a comment in forms.css to that effect (saying that anyone editing
forms.css will have to change this code as well).

Also, please use defines with self-explanatory names (eg
"INPUT_BORDER_IN_PIXELS") instead of raw magic numbers like "2".

It strikes me that all this code is incredibly fragile... what we really want
is a way to query style without taking into account author (and probably user)
stylesheets.  Please file a bug to that effect on the style system; cc me and
hyatt...

>+      nsCOMPtr<nsILookAndFeel> lookAndFeel(do_CreateInstance(kLookAndFeelCID));

I know you just copied this code, but please use do_GetService instead (as most
people who need an nsILookAndFeel seem to do, which makes sense).

>+      lookAndFeel->GetColor(nsILookAndFeel::eColor_threedface, defaultBorderColor);

What if the do_GetService (or do_CreateInstance) fails?  Please null-check
|lookAndFeel| and return something sane (probably "drop the theming") if it's
null.

>+        case NS_THEME_BUTTON: {
>+          defaultBGColor = defaultBorderColor;

Why is this?  The default background-color and border-color are different for
buttons in forms.css

>+          if (contentState & NS_EVENT_STATE_HOVER && contentState & NS_EVENT_STATE_ACTIVE)
>+            defaultBorderStyle = NS_STYLE_BORDER_STYLE_INSET;
>+          else
>+            defaultBorderStyle = NS_STYLE_BORDER_STYLE_OUTSET;
>+          break;

Does this correctly deal with disabled buttons (for which the border does _not_
depend on the content state and the colors are different from non-disabled ones
in forms.css)?

Please add a "default" clause to this switch, which does 
NS_ERROR("widget type not handled");
or something along those lines, just in case someone tries to expand the |if|
without expanding the |switch|.

>+      // Check if background differs from default

"Check whether"

>+          (ourBG->mBackgroundFlags & NS_STYLE_BG_IMAGE_NONE) == 0)

I'd write this as "!(ourBG->mBackgroundFlags & NS_STYLE_BG_IMAGE_NONE)" or at
least drop the parens if you plan to compare to 0.

>+      // Check if border style or color differs from default

"Check whether"

>+      // Check if border size differs from default

Same here.

I assume that setting "-moz-appearance: none" on image inputs will be a
separate patch in the relevant bug?

Comment 12

[Boris Zbarsky [:bzbarsky]]

One other thing.  The border on disabled buttons is 1px, not 2px....

Comment 13

[Tim Hill]

Attachment: Patch to nsNativeThemeWin, forms.css #2

> It strikes me that all this code is incredibly fragile... 

Yeah, I don't like it much either, but I don't know of a quick, simple way to
access just the UA style info...

> What if the do_GetService (or do_CreateInstance) fails?  
> Please null-check lookAndFeel| and return something sane 
> (probably "drop the theming") if it's null.

Ok, but crashing is more fun ;)

> Why is this?	The default background-color and border-color 
> are different for buttons in forms.css

They are both buttonface for regular buttons...

> I assume that setting "-moz-appearance: none" on image inputs will be a
> separate patch in the relevant bug?

I might as well include that here if I'm modifying forms.css anyway.

> Does this correctly deal with disabled buttons 

Oops. Fixed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I don't know of a quick, simple way to access just the UA style info...

There isn't one.  That's why I suggested you file a bug requesting one.  ;)

> They are both buttonface for regular buttons...

So they are; my apologies.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 110569 [details] [diff] [review]
Patch to nsNativeThemeWin, forms.css #2

Looks good!

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 110569 [details] [diff] [review]
Patch to nsNativeThemeWin, forms.css #2

Since hyatt seems to be AWOL, would you mind doing the review, bryner?	This
patch fixes a slew of regressions; it'd be good to have it fixed for 1.3b...

Comment 17

[José Jeria]

*** Bug 190444 has been marked as a duplicate of this bug. ***

Comment 18

[Brian Ryner (not reading)]

Comment on attachment 110569 [details] [diff] [review]
Patch to nsNativeThemeWin, forms.css #2

>Index: gfx/src/windows/nsNativeThemeWin.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/windows/nsNativeThemeWin.cpp,v
>retrieving revision 3.18
>diff -u -r3.18 nsNativeThemeWin.cpp
>--- gfx/src/windows/nsNativeThemeWin.cpp	4 Dec 2002 04:41:33 -0000	3.18
>+++ gfx/src/windows/nsNativeThemeWin.cpp	3 Jan 2003 07:06:20 -0000
>+      float p2t;
>+      aPresContext->GetPixelsToTwips(&p2t);
>+
>+      nsCOMPtr<nsILookAndFeel> lookAndFeel(do_GetService(kLookAndFeelCID));
>+      if (!lookAndFeel) 
>+        return PR_TRUE;

If this is called a lot, you might want to cache the LookAndFeel service to
improve performance.

> 
>       nsCOMPtr<nsIStyleContext> styleContext;
>       aFrame->GetStyleContext(getter_AddRefs(styleContext));
>+      if (!styleContext)
>+        return PR_TRUE;

You're going to have to merge this part to the trunk.  There shouldn't be any
more usage of nsIStyleContext from outside of content and layout. 
nsIFrame::GetStyleData() should be fine for this.

>Index: layout/html/document/src/forms.css
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/document/src/forms.css,v
>retrieving revision 3.58
>diff -u -r3.58 forms.css
>--- layout/html/document/src/forms.css	12 Nov 2002 19:17:46 -0000	3.58
>+++ layout/html/document/src/forms.css	3 Jan 2003 07:06:21 -0000
>@@ -267,6 +272,7 @@
> 
> /* image buttons */
> input[type="image"] {
>+  -moz-appearance: none;
>   padding: 0;
>   border: none;
>   background-color: transparent;

Good, good, I had this fix as part of another patch.

Could you merge this to the trunk and address my comments above, and I'll
review the new patch?

Comment 19

[Tim Hill]

*** Bug 190906 has been marked as a duplicate of this bug. ***

Comment 20

[Tim Hill]

Attachment: Patch to nsNativeThemeWin, forms.css #3

gets cached LookAndFeel from PresContext, gets style from frame instead of
stylecontext

Comment 21

[Tim Hill]

Comment on attachment 113063 [details] [diff] [review]
Patch to nsNativeThemeWin, forms.css #3

requesting review and carrying over bz's sr.  It'd be nice to get this approved
for 1.3 since it fixes quite a few form control issues. It's a fairly
straightforward and low risk change.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 113063 [details] [diff] [review]
Patch to nsNativeThemeWin, forms.css #3

This should wait until we open for 1.4alpha.

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 113063 [details] [diff] [review]
Patch to nsNativeThemeWin, forms.css #3

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 24

[Boris Zbarsky [:bzbarsky]]

I just checked the patch in.  Someone who can actually test on Windows should go
through and mark this bug and its dependencies fixed as appropriate.

Comment 25

[viipale+bugzilla]

Seems to be fixed, thank you! Mozilla build 2003013104, Windows 2000.

Comment 26

[Brian 'netdragon' Bober]

bz said he checked it in at 2:23pm, and your binary was 10:11 am, so how could 
it be fixed?

Comment 27

[Brian 'netdragon' Bober]

Nevermind. 2:23 AM, lol. Forgot bz never sleeps ;-)

Comment 28

[Brian 'netdragon' Bober]

It works for me now. Something strange I noticed is that if you place the 
cursor somewhere else in the window besides where the textbox is, and press a 
button, it gives some weird tones and shows something in the status bar. What 
is all that about?

Comment 29

[Brian 'netdragon' Bober]

BTW: The tones stop after a few buttons. Anyone else able to cause this to 
happen? Still, the issue in this bug is fixed. Marked fixed on Windows XP 
2003013108. I will go through the dependencies and do what bz said on ones 
that appear fixed.

Comment 30

[viipale+bugzilla]

That's probably the type ahead feature.