Closed Bug 970257 Opened 6 years ago Closed 6 years ago

Input type "number" has width of 0px when set to auto

Categories

(Core :: Layout: Form Controls, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: pot, Assigned: jwatt)

References

()

Details

(Keywords: testcase)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140205162153

Steps to reproduce:

I added an input[type="number"] element on my page and gave it "width: auto" in the CSS, then took a look at it in Firefox v28 (beta).


Actual results:

The input element got its width set at 0 and the contents wasn't visible.


Expected results:

The input element gets its width set in a similar matter as the input[type="text"] element, which gets its width set at 144px.
Component: Untriaged → Layout: Form Controls
Product: Firefox → Core
Flags: needinfo?(jwatt)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 8.1 → All
Hardware: x86_64 → All
Version: 28 Branch → Trunk
Keywords: testcase
Priority: -- → P3
Attached patch patchSplinter Review
Just need to implement Get[Min|Pref]Width properly.
Assignee: nobody → jwatt
Attachment #8374594 - Flags: review?(dholbert)
Flags: needinfo?(jwatt)
The test would fail without the patch because the input would be so narrow that the right border and the spin buttons would be visible in the test (not hidden under the black div).
Comment on attachment 8374594 [details] [diff] [review]
patch

>+nscoord
>+nsNumberControlFrame::GetMinWidth(nsRenderingContext* aRenderingContext)
>+{
>+  nscoord result;
>+  DISPLAY_MIN_WIDTH(this, result);
>+
>+  nsIFrame* kid = mFrames.FirstChild();
>+  result = nsLayoutUtils::IntrinsicForContainer(aRenderingContext,
>+                                                kid,
>+                                                nsLayoutUtils::MIN_WIDTH);

We need to handle the possibility of 'kid' being null here (for 'display:none'), since IntrinsicForContainer assumes non-null input. I think it's probably reasonable to just return 0 in that case.

(Same goes for GetPrefWidth.)

It'd be nice to test that, too (at least, that we don't crash/assert), but we can't do that without styling the pseudo-elements, so that'll be another thing to test on bug 926412. I'll add a comment over there.

>+++ b/layout/forms/nsNumberControlFrame.h
>@@ -45,16 +45,20 @@ public:
>   virtual void DestroyFrom(nsIFrame* aDestructRoot) MOZ_OVERRIDE;
[...]
>+  virtual nscoord GetMinWidth(nsRenderingContext *aRenderingContext) MOZ_OVERRIDE;
>+
>+  virtual nscoord GetPrefWidth(nsRenderingContext *aRenderingContext) MOZ_OVERRIDE;
>+
>   NS_IMETHOD Reflow(nsPresContext*           aPresContext,

Nit: Make those asterisks left-aligned, to match surrounding code and to match your impls of these methods.

>+++ b/layout/reftests/forms/input/number/number-auto-width-1.html
>@@ -0,0 +1,8 @@
>+<!DOCTYPE html>
>+<html>
>+  <body>
>+    <input type="number" style="-moz-appearance:none; width:auto;">
>+    <!-- div to cover spin box area -->
>+    <div style="display:block; position:absolute; background-color:black; width:2000px; height:100px; top:0px; left:100px;">

I prefer the "-moz-appearance:textfield" technique for hiding the spinner, that you used in bug 951310 -- that lets us actually verify that the width is exactly the same, instead of just that it's large enough to stick under the black div (i.e. > 100px)

So I'd lean towards switching to that, and dropping the black div.

r=me with the above.
Attachment #8374594 - Flags: review?(dholbert) → review+
> for 'display:none'

Can the kid actually be styled with display:none in this case?
Only in chrome-privileged stylesheets for now, though IIUC we may expose the pseudo-element to the web eventually.

Also, we go to the trouble of checking whether the child is null in ::Reflow():
 http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsNumberControlFrame.cpp#88
and it doesn't really make sense to have a null-check there but not here.
(In reply to Daniel Holbert [:dholbert] from comment #3)
> I prefer the "-moz-appearance:textfield" technique for hiding the spinner,
> that you used in bug 951310 -- that lets us actually verify that the width
> is exactly the same, instead of just that it's large enough to stick under
> the black div (i.e. > 100px)

That's not possible since I'm comparing to a text control which doesn't necessarily have the same pref width as a number control. Other than that I addressed your comments.
Target Milestone: mozilla19 → mozilla29
https://hg.mozilla.org/mozilla-central/rev/7ac30a8de3d6
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla29 → mozilla30
Comment on attachment 8374594 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug in new feature (bug 344616)
User impact if declined: number controls broken if styled using auto width
Testing completed (on m-c, etc.): merged to m-c
Risk to taking this patch (and alternatives if risky): low risk, early in aurora
String or IDL/UUID changes made by this patch: none
Attachment #8374594 - Flags: approval-mozilla-aurora?
Attachment #8374594 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 344616
You need to log in before you can comment on or make changes to this bug.