Closed
Bug 1437302
Opened 6 years ago
Closed 6 years ago
The textbox[type="number"] does not respect the size attribute
Categories
(Toolkit :: UI Widgets, defect)
Toolkit
UI Widgets
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.
Assignee | ||
Comment 1•6 years ago
|
||
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.
Assignee | ||
Comment 2•6 years ago
|
||
Another possibility is simply to drop support for size on textbox[type=number] and set the width in the CSS where appropriate.
Reporter | ||
Comment 3•6 years ago
|
||
And how would this CSS look? I played a bit but didn't succeed.
Assignee | ||
Comment 4•6 years ago
|
||
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.
Assignee | ||
Comment 5•6 years ago
|
||
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; }
Reporter | ||
Comment 6•6 years ago
|
||
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)
Comment 7•6 years ago
|
||
(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)
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Comment 9•6 years ago
|
||
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.
Comment 10•6 years ago
|
||
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.
Comment 11•6 years ago
|
||
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)
Updated•6 years ago
|
Keywords: regression
Comment 12•6 years ago
|
||
Flags: needinfo?(acelists)
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
mozreview-review |
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+
Updated•6 years ago
|
Attachment #8962321 -
Flags: review?(enndeakin)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(ntim.bugs)
Comment 15•6 years ago
|
||
Gentle review ping: Neil, any chance to get this reviewed soon?
Flags: needinfo?(enndeakin)
Comment 16•6 years ago
|
||
mozreview-review |
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+
Updated•6 years ago
|
Flags: needinfo?(enndeakin)
Comment 17•6 years ago
|
||
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
Comment 18•6 years ago
|
||
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)
Comment 19•6 years ago
|
||
The correct changeset is this: Backed out changeset 031bed4cc2cd (bug 1437302) for windows bustages on numberbox.css.
Comment 20•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(ntim.bugs)
Comment 21•6 years ago
|
||
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
Comment 22•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ca384cb18f71 https://hg.mozilla.org/mozilla-central/rev/3a04ec0fc8f1
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ntim.bugs
Updated•6 years ago
|
status-firefox59:
--- → unaffected
status-firefox60:
--- → fix-optional
status-firefox-esr52:
--- → unaffected
Comment 23•6 years ago
|
||
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?
Reporter | ||
Comment 24•6 years ago
|
||
FX uses it in the proxy settings, which don't look good without this patch.
Flags: needinfo?(richard.marti)
Assignee | ||
Comment 25•6 years ago
|
||
> 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•6 years ago
|
||
I updated the mozreview request to also include the followup from comment 21 so it can be uplifted easily.
Comment 29•6 years ago
|
||
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)
Comment 30•6 years ago
|
||
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
Assignee | ||
Comment 31•6 years ago
|
||
(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)
Comment 33•6 years ago
|
||
(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.
Comment 34•6 years ago
|
||
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?
Comment 35•6 years ago
|
||
Mixing the field width change with the spinbutton removal makes this a bit of a pain. Not convinced this is uplift material as-is.
Updated•6 years ago
|
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 36•6 years ago
|
||
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)
Comment 37•6 years ago
|
||
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)
Comment 38•6 years ago
|
||
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)
Comment 39•6 years ago
|
||
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.
Assignee | ||
Comment 40•6 years ago
|
||
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 41•6 years ago
|
||
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+
Comment 42•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/2eb1065270bb https://hg.mozilla.org/releases/mozilla-beta/rev/363aebb824a0
You need to log in
before you can comment on or make changes to this bug.
Description
•