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

RESOLVED FIXED

Status

()

Core
Layout: Form Controls
--
major
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Jesse Ruderman, Assigned: Tim Hill)

Tracking

({regression})

Trunk
x86
Windows XP
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

15 years ago
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.
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?
Assignee: form → hyatt
Shouldn't we drop the theming when the border is styled, btw?
*** Bug 186376 has been marked as a duplicate of this bug. ***
Blocks: 184218

Comment 4

15 years ago
This happends in both XP and Classic theme on XP.
*** Bug 187068 has been marked as a duplicate of this bug. ***
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...
Assignee: hyatt → tim

Comment 7

15 years ago
Dropping theming when border is changed is eminently reasonable.

Comment 8

15 years ago
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.
No, -moz-appearance should be set exactly as it is now.  It should compute 
to "none" if the page sets a border.
(Assignee)

Comment 10

15 years ago
Created attachment 110537 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #110537 - Flags: superreview?(bzbarsky)
Attachment #110537 - Flags: review?(hyatt)
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?
Attachment #110537 - Flags: superreview?(bzbarsky) → superreview-
One other thing.  The border on disabled buttons is 1px, not 2px....
(Assignee)

Comment 13

15 years ago
Created attachment 110569 [details] [diff] [review]
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.
Attachment #110537 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #110569 - Flags: superreview?(bzbarsky)
Attachment #110569 - Flags: review?(hyatt)
> 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 on attachment 110569 [details] [diff] [review]
Patch to nsNativeThemeWin, forms.css #2

Looks good!
Attachment #110569 - Flags: superreview?(bzbarsky) → superreview+
Attachment #110537 - Flags: review?(hyatt)
Blocks: 171598, 181490, 185708
Blocks: 188785
Blocks: 189907
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...
Attachment #110569 - Flags: review?(hyatt) → review?(bryner)

Comment 17

15 years ago
*** Bug 190444 has been marked as a duplicate of this bug. ***
Blocks: 190610
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?
Attachment #110569 - Flags: review?(bryner) → review-
(Assignee)

Comment 19

15 years ago
*** Bug 190906 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 20

15 years ago
Created attachment 113063 [details] [diff] [review]
Patch to nsNativeThemeWin, forms.css #3

gets cached LookAndFeel from PresContext, gets style from frame instead of
stylecontext
Attachment #110569 - Attachment is obsolete: true
(Assignee)

Comment 21

15 years ago
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.
Attachment #113063 - Flags: superreview+
Attachment #113063 - Flags: review?(bryner)
Attachment #113063 - Flags: approval1.3b?

Comment 22

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

This should wait until we open for 1.4alpha.
Attachment #113063 - Flags: approval1.3b? → approval1.3b-

Comment 23

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

a=asa (on behalf of drivers) for checkin to 1.3beta.
Attachment #113063 - Flags: approval1.3b- → approval1.3b+
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

15 years ago
Seems to be fixed, thank you! Mozilla build 2003013104, Windows 2000.
Blocks: 191453
bz said he checked it in at 2:23pm, and your binary was 10:11 am, so how could 
it be fixed?
Nevermind. 2:23 AM, lol. Forgot bz never sleeps ;-)
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?
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
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

15 years ago
That's probably the type ahead feature.
Attachment #113063 - Flags: review?(bryner)
You need to log in before you can comment on or make changes to this bug.