Closed Bug 1437302 Opened 2 years ago Closed 2 years ago

The textbox[type="number"] does not respect the size attribute

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: Paenglab, Assigned: ntim)

References

Details

(Keywords: regression)

Attachments

(4 files)

Before bug 1429573 the textbox[type="number"] respected the size attribute. Now it has always the same with. For example the port textboxes in the Proxy settings on the prefs are too wide.
According to the HTML spec, input[type=number] shouldn't respect the size attribute.

I guess for XUL textbox[type=number], a possible path is to implement the most common size attribute values using CSS.
Another possibility is simply to drop support for size on textbox[type=number] and set the width in the CSS where appropriate.
And how would this CSS look? I played a bit but didn't succeed.
Richard, it looks like forcing all the input[type=number] anonymous children to use XUL flexbox instead of HTML flexbox as shown in the screenshot, makes changing the width via CSS work.
Here's a text version of the CSS:

html|input[type="number"],
html|input[type="number"]::-moz-number-wrapper,
html|input[type="number"]::-moz-number-spin-box {
  display: -moz-box;
  -moz-box-align: center;
}

html|input[type="number"]::-moz-number-spin-box {
  -moz-box-orient: vertical;
}

html|input[type=number]::-moz-number-text {
  -moz-box-flex: 1;
}
This makes the field width changeable through CSS but at least on Windows the spinbuttons are too low (but the Linux/Mac padding change from bug 1437284 would also apply on Windows).

What's the state for the Proxy port fields? Is it planned to make them less wide?
Flags: needinfo?(paolo.mozmail)
(In reply to Tim Nguyen :ntim from comment #1)
> Another possibility is simply to drop support for size on
> textbox[type=number] and set the width in the CSS where appropriate.

Setting a fixed width on the element using CSS sounds like the best solution me, but have you tried to fix this without having to change the box model of the inner input? I think it should be doable.

By the way, having spin buttons for the Port settings doesn't really make sense, as the number doesn't indicate a quantity, so I would be fine with hiding them while we're here.
Flags: needinfo?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #7)
> (In reply to Tim Nguyen :ntim from comment #1)
> > Another possibility is simply to drop support for size on
> > textbox[type=number] and set the width in the CSS where appropriate.
> 
> Setting a fixed width on the element using CSS sounds like the best solution
> me, but have you tried to fix this without having to change the box model of
> the inner input? I think it should be doable.

I couldn't manage to change the width without getting rid of HTML flexbox usage inside input[type=number]. I tried tweaking width/max-width/min-width/-moz-box-flex and even position: absolute; but couldn't get it to work in the right way without making input[type=number] use XUL flex.
Can you post a patch to fix the size of the textboxes, even if it only works with the changes in comment 5?

Maybe we can use it as a starting point to see if there are alternatives that work with flexbox.
We should fix this in some way as otherwise our release of 60 has the account manager hosed, the elements do not fit into the window, etc.
Thanks for the reminder aceman! Can you post scheenshots of the broken interfaces you found?

Tim, can you take a look at this?
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(acelists)
Keywords: regression
Flags: needinfo?(acelists)
Comment on attachment 8962321 [details]
Bug 1437302 - Fix width of textbox[type=number] consumers.

https://reviewboard.mozilla.org/r/231194/#review236784

Looks good to me, but I'd like Neil to review the parts related to the box model.

We're also investigating converting the XUL box model to flexbox, so this may be fine for now and may not be needed anymore later.
Attachment #8962321 - Flags: review?(paolo.mozmail) → review+
Attachment #8962321 - Flags: review?(enndeakin)
Flags: needinfo?(ntim.bugs)
Gentle review ping: Neil, any chance to get this reviewed soon?
Flags: needinfo?(enndeakin)
Comment on attachment 8962321 [details]
Bug 1437302 - Fix width of textbox[type=number] consumers.

https://reviewboard.mozilla.org/r/231194/#review240646
Attachment #8962321 - Flags: review?(enndeakin) → review+
Flags: needinfo?(enndeakin)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/031bed4cc2cd
Fix width of textbox[type=number] consumers. r=enndeakin+6102,Paolo
Backed out 6 changesets (bug 1416879) for windows bustages on numberbox.css

Backout: https://hg.mozilla.org/integration/autoland/rev/b5065c61bbd78a43d3a8dfa4d6531d2071e2fbc8 

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=031bed4cc2cd9776ae817d2cffb51986fc1dd1bb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=172701939&repo=autoland&lineNumber=31847

7:50:08     INFO -    File "c:\mozilla-build\python\Lib\runpy.py", line 174, in _run_module_as_main
17:50:08     INFO -      "__main__", fname, loader, pkg_name)
17:50:08     INFO -    File "c:\mozilla-build\python\Lib\runpy.py", line 72, in _run_code
17:50:08     INFO -      exec code in run_globals
17:50:08     INFO -    File "z:\build\build\src\python\mozbuild\mozbuild\action\jar_maker.py", line 17, in <module>
17:50:08     INFO -      sys.exit(main(sys.argv[1:]))
17:50:08     INFO -    File "z:\build\build\src\python\mozbuild\mozbuild\action\jar_maker.py", line 13, in main
17:50:08     INFO -      return mozbuild.jar.main(args)
17:50:08     INFO -    File "z:\build\build\src\python\mozbuild\mozbuild\jar.py", line 608, in main
17:50:08     INFO -      jm.makeJar(infile, options.d)
17:50:08     INFO -    File "z:\build\build\src\python\mozbuild\mozbuild\jar.py", line 333, in makeJar
17:50:08     INFO -      self.processJarSection(info, jardir)
17:50:08     INFO -    File "z:\build\build\src\python\mozbuild\mozbuild\jar.py", line 388, in processJarSection
17:50:08     INFO -      self._processEntryLine(e, outHelper, jf)
17:50:08     INFO -    File "z:\build\build\src\python\mozbuild\mozbuild\jar.py", line 474, in _processEntryLine
17:50:08     INFO -      pp.failUnused(realsrc)
17:50:08     INFO -    File "z:\build\build\src\python\mozbuild\mozbuild\preprocessor.py", line 332, in failUnused
17:50:08     INFO -      raise Preprocessor.Error(fake, msg, None)
17:50:08     INFO -  mozbuild.preprocessor.Error: ('z:\\build\\build\\src\\toolkit\\themes\\windows\\global\\numberbox.css', None, 'no preprocessor directives found', None)
17:50:08     INFO -  z:/build/build/src/config/rules.mk:1235: recipe for target 'libs' failed
17:50:08     INFO -  mozmake.EXE[5]: *** [libs] Error 1
17:50:08     INFO -  mozmake.EXE[5]: Leaving directory 'z:/build/build/src/obj-firefox/toolkit/themes/windows/global'
17:50:08     INFO -  z:/build/build/src/config/recurse.mk:100: recipe for target 'toolkit/themes/windows/global/libs' failed
17:50:08     INFO -  mozmake.EXE[4]: *** [toolkit/themes/windows/global/libs] Error 2
17:50:08     INFO -  z:/build/build/src/config/recurse.mk:32: recipe for target 'libs' failed
17:50:08     INFO -  mozmake.EXE[3]: *** [libs] Error 2
17:50:08     INFO -  z:/build/build/src/config/rules.mk:418: recipe for target 'default' failed
17:50:08     INFO -  mozmake.EXE[2]: *** [default] Error 2
17:50:08     INFO -  Makefile:237: recipe for target 'profiledbuild' failed
17:50:08     INFO -  mozmake.EXE[1]: *** [profiledbuild] Error 2
17:50:08     INFO -  client.mk:168: recipe for target 'build' failed
17:50:08     INFO -  mozmake.EXE: *** [build] Error 2
17:50:08     INFO -  148 compiler warnings present.
17:50:09    ERROR - Return code: 2
17:50:09  WARNING - setting return code to 2
17:50:09    FATAL - 'mach build' did not run successfully. Please check log for errors.
17:50:09    FATAL - Running post_fatal callback...
17:50:09    FATAL - Exiting -1
Flags: needinfo?(ntim.bugs)
The correct changeset is this:

Backed out changeset 031bed4cc2cd (bug 1437302) for windows bustages on numberbox.css.
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca384cb18f71
Fix width of textbox[type=number] consumers. r=Paolo, Neil Deakin
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a04ec0fc8f1
Followup: fix reftest failures on a CLOSED TREE. r=me
https://hg.mozilla.org/mozilla-central/rev/ca384cb18f71
https://hg.mozilla.org/mozilla-central/rev/3a04ec0fc8f1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee: nobody → ntim.bugs
Comment on attachment 8962321 [details]
Bug 1437302 - Fix width of textbox[type=number] consumers.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1429573.
[User impact if declined]: It's used in Thunderbird in various places, without the patch all the number fields look too big.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: I'll let the reporter do that.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: Low risk, it restores the formatting of a number field which is not(?) used in FF, or maybe it is in proxy port numbers or similar.
[Why is the change risky/not risky?]: See above.
[String changes made/needed]: None.

Richard, can you please verify this bug. Do you know whether FF uses this textbox style?
Flags: needinfo?(richard.marti)
Attachment #8962321 - Flags: approval-mozilla-beta?
FX uses it in the proxy settings, which don't look good without this patch.
Flags: needinfo?(richard.marti)
> It's used in Thunderbird in various places, without the patch all the number fields look too big.

Affects Firefox as well, notably the print preview page number and the proxy port settings.
I updated the mozreview request to also include the followup from comment 21 so it can be uplifted easily.
What's the story with the spin buttons? You've added hidespinbuttons="true" in various places and Richard tells me that they don't work any more and/or are oddly placed. So we're following the removal in bug 1453233 which is perhaps not optimal.
Flags: needinfo?(ntim.bugs)
Well, the spinbutton binding was removed in in bug 1429573.
You may also want to close some bugs on this list now: https://mzl.la/2HpIkJu
(In reply to Jorg K (GMT+1) from comment #29)
> What's the story with the spin buttons? You've added hidespinbuttons="true"
> in various places and Richard tells me that they don't work any more and/or
> are oddly placed. So we're following the removal in bug 1453233 which is
> perhaps not optimal.

See comment 7 for why I removed the spinbuttons from the proxy port inputs.

> You may also want to close some bugs on this list now: https://mzl.la/2HpIkJu

Thanks!
Flags: needinfo?(ntim.bugs)
Duplicate of this bug: 448245
(In reply to Tim Nguyen :ntim from comment #31)
> See comment 7 for why I removed the spinbuttons from the proxy port inputs.
I agree, they don't make sense for port numbers. The cache size preference also had them, but it looks like that was removed. TB still exposes that preference together with some other number boxes where increasing/decreasing makes sense, for example "interval to poll server", "days to leave message on server", etc.

Let's continue this discussion in bug 1453322.
I'm confused now. Before this bug here landed, the spinbuttons *were* working for the proxy ports, I've tried in a Nightly from earlier this month: 61.0a1 (2018-04-01) (64-bit). So where the bindings not used?
Mixing the field width change with the spinbutton removal makes this a bit of a pain.  Not convinced this is uplift material as-is.
Flags: needinfo?(ntim.bugs)
I could make a patch without the spinbuttons removal, but seems like bug 1453371 is preventing this. I don't think I can work on bug 1453371 anytime soon, especially since no textbox[type=number] has spin buttons anymore.

JorgK, any chance TB can fix the bug in comm-beta rather than having to uplift this to mozilla-beta ?
Flags: needinfo?(ntim.bugs) → needinfo?(jorgk)
We fixed TB in trunk (61) and beta (60) in bug 1453322, so our number fields have the correct size and they all have spinbuttons, including the port fields. If you use proxies, it can make sense to just go up a port on the proxy.

TB is all set and we don't need anything.

FF 61 seems to be good as well since you landed this bug here. FF 60 ESR(!) has a problem since without this bug here the number fields are too wide.

So you have the choice of two for FF:
- Don't uplift: Ship FF 60 ESR with number fields which are to big or 
- Uplift: Fix the width and remove the spinbuttons at the same time.

I think number two is the lesser evil :-) - Personally, I'd just uplift this bug and be done with it in FF.
Flags: needinfo?(jorgk)
Can you give a firefox test case or example of the text boxes looking wrong? 

I don't understand what the spin buttons removal might break in Firefox, can you make that more clear? 

If this is a super ugly issue or breaks some part of our UI or major websites we should probably fix it for 0. If not, we are pretty late in beta to uplift this.
Flags: needinfo?(ntim.bugs)
Attached image 59-60-61.png
Here you see a comparison of 59, 60 (beta) and 61 (Nightly).

In 60 the number fields are too wide. So I suggest uplift. Removing the spinbuttons doesn't break anything, in fact their removal was requested in bug 448245.

If the spinbuttons weren't removed, they wouldn't show properly since they are now sadly broken, see bug 1453371. (Thunderbird worked around this.)

So you have the choice between what you have in 60 (ugly) or 61.
Jorg summarized the issue, thanks!

The spinbuttons were removed as an intentional UX change (see comment 7), so now none of the Firefox UI number inputs have spinbuttons. That broke however Thunderbird. Firefox still is fine though.
Flags: needinfo?(ntim.bugs)
Comment on attachment 8962321 [details]
Bug 1437302 - Fix width of textbox[type=number] consumers.

OK, let's go ahead and try this for beta 15.
Attachment #8962321 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1459563
You need to log in before you can comment on or make changes to this bug.