Closed
Bug 1247929
Opened 9 years ago
Closed 9 years ago
In Firefox DeveloperEdition 46 the table layout algorithm has sizing-problems with some form fields like <input type="number" />.
Categories
(Core :: Layout: Form Controls, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox45 | --- | unaffected |
firefox46 | + | fixed |
firefox47 | + | fixed |
People
(Reporter: pirmin.rehm, Assigned: dbaron)
References
Details
(Keywords: regression, testcase)
Attachments
(7 files)
5.51 KB,
image/png
|
Details | |
992 bytes,
text/html
|
Details | |
2.23 KB,
text/html
|
Details | |
58 bytes,
text/x-review-board-request
|
dholbert
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
dholbert
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
dholbert
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
dholbert
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160211075903
Steps to reproduce:
I use bootstrap 3 input-groups for my webpage. For development i use Firefox DevEdition (currently 46). The input-groups have a specific width, so that the width of the inpupt-field gets dynamically adapted to the width of the input-group-addon (depending on the text in addon).
For bootstrap input-groups take a look here:
http://getbootstrap.com/css/#forms-inline
I rebuild the problem (without bootstrap) in a jsfiddle:
https://jsfiddle.net/o1tkax4t/
Actual results:
With some of the last Versions of Firefox DevEdition the <input type="number"> breaks out ouf the outer div (class input-group) instead fitting in.
However, i decided to rebuild the bug in an jsfiddle:
https://jsfiddle.net/o1tkax4t/
As you can see (with Firefox 46 DevEdition), the input-field with type="number" breaks out of the red div. With Firefox 44 or IE11 the input-field fits into the red outer div.
Expected results:
The table sizing algorithm should have calculated for the input field the expected width, so that the field fills out the rest of the div.
Component: Untriaged → Layout: Form Controls
Product: Firefox → Core
Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=65e84a
da79e41a3afdcb0d8148a84a7152888df1&tochange=623934e9db8ab60eb605fba84e03e882651e
8f02
Blocks: 823483
Status: UNCONFIRMED → NEW
status-firefox45:
--- → unaffected
status-firefox46:
--- → affected
status-firefox47:
--- → affected
tracking-firefox46:
--- → ?
tracking-firefox47:
--- → ?
Ever confirmed: true
Flags: needinfo?(dbaron)
Keywords: regression,
testcase
OS: Unspecified → All
Hardware: Unspecified → All
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
This looks like a relatively simple bug; the special case for text inputs probably also needs to be added for number controls, although I should check that they do in fact have the same special-case behavior (of responding to percentage widths but not percentage min-widths), and I should check more thoroughly through other form controls.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
Comment 6•9 years ago
|
||
Tracking, recent regression.
Please request uplift once you have a fix, thanks!
Assignee | ||
Comment 7•9 years ago
|
||
It seems like all form controls *except* for buttons and fieldsets have the special behavior for 'width' but do not have it for 'max-width'. This appears to be true in both Chromium and Edge. I'm not sure why I thought that special behavior was text inputs only; I can no longer find the testcase I used to determine that.
Assignee | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c48cbd30153d is a good try run, except for a failure that I've fixed in the patch 4 that I will attach here.
Flags: needinfo?(dbaron)
Assignee | ||
Comment 9•9 years ago
|
||
These frames previously inherited nsFrame::GetType (which returns null).
Review commit: https://reviewboard.mozilla.org/r/36265/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36265/
Attachment #8722855 -
Flags: review?(dholbert)
Assignee | ||
Comment 10•9 years ago
|
||
This adjusts the behavior previously modified by bug 823483 patch 2 and
bug 823483 patch 5.
Review commit: https://reviewboard.mozilla.org/r/36267/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36267/
Attachment #8722856 -
Flags: review?(dholbert)
Assignee | ||
Comment 11•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36269/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36269/
Attachment #8722857 -
Flags: review?(dholbert)
Assignee | ||
Comment 12•9 years ago
|
||
This reverts some of the changes made in bug 823483 patch 3 because it
turns out we don't want all of that behavior change.
Review commit: https://reviewboard.mozilla.org/r/36271/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36271/
Attachment #8722858 -
Flags: review?(dholbert)
Updated•9 years ago
|
Attachment #8722855 -
Flags: review?(dholbert) → review+
Comment 13•9 years ago
|
||
Comment on attachment 8722855 [details]
MozReview Request: Bug 1247929 patch 1 - Add GetType() overrides for nsProgressFrame and nsMeterFrame. r?dholbert
https://reviewboard.mozilla.org/r/36265/#review32925
Comment 14•9 years ago
|
||
Comment on attachment 8722856 [details]
MozReview Request: Bug 1247929 patch 2 - Hard-code the Web-compatible set of form controls whose intrinsic minimum inline-size shrinks to 0 when inline-size (width) is specified as a percentage. r?dholbert
https://reviewboard.mozilla.org/r/36267/#review32927
r=me, just one nit:
::: layout/base/nsLayoutUtils.cpp:4623
(Diff revision 1)
> - aFrame->GetType() == nsGkAtoms::textInputFrame))) {
> + FormControlShrinksForPercentWidth(aFrame)))) {
Consider renaming this method, s/Width/ISize/, to make its meaning a bit less ambiguous.
(With "Width" in the name, a reader ends up slightly wondering whether it's valid to combine this method with an abstract MIN_ISIZE check, as we do here, when the writing-mode is vertical.)
Attachment #8722856 -
Flags: review?(dholbert) → review+
Updated•9 years ago
|
Attachment #8722857 -
Flags: review?(dholbert) → review+
Comment 15•9 years ago
|
||
Comment on attachment 8722857 [details]
MozReview Request: Bug 1247929 patch 3 - Reftests. r?dholbert
https://reviewboard.mozilla.org/r/36269/#review32933
r=me, with concern below addressed:
::: layout/reftests/css-sizing/min-intrinsic-with-width-percents-across-form-controls.html:48
(Diff revision 1)
> + <td>input type="file"</td>
Chrome fails this part of the 'width' reftest (for input type="file"), on my system at least. The reference case has a wide blue element, whereas the testcase has a skinny blue element.
Perhaps this is one more frame that we need to exempt (like buttons) in Patch 2? (and adjust the reference's expectations here)
I'm using Chrome Dev Edition, version 50.0.2652.0 dev (64-bit)
Comment 16•9 years ago
|
||
Comment on attachment 8722858 [details]
MozReview Request: Bug 1247929 patch 4 - Adjust reftest manifest for existing tests. r?dholbert
https://reviewboard.mozilla.org/r/36271/#review32935
Attachment #8722858 -
Flags: review?(dholbert) → review+
Comment 17•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #15)
> Perhaps this is one more frame that we need to exempt (like buttons) in
> Patch 2? (and adjust the reference's expectations here)
Sorry, I had the failure scenario swapped in my head when I wrote this paragraph.
It seems like (for compat) we *do want* <input type="file"> to shrink -- so the reftest is correct here. The Chrome failure seems to just be a bug in chrome, where it renders the reference case's <input type="file" style="width: 0"> with a large width for some reason.
So, no action we need to take here for this I think, aside from perhaps filing a Chrome bug.
Comment 18•9 years ago
|
||
Oh, one nit on the reftests, though -- all of the new reftest files here have:
> canvas {
> background: blue
> }
...but no actual <canvas> elements. So, that rule probably wants to be removed.
Assignee | ||
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7defbcbde4ced37e43e45c646c28fc4eca93d87b
Bug 1247929 patch 1 - Add GetType() overrides for nsProgressFrame and nsMeterFrame. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/89e5af841e9d162217257489c05aa2fbe45e8a17
Bug 1247929 patch 2 - Hard-code the Web-compatible set of form controls whose intrinsic minimum inline-size shrinks to 0 when inline-size (width) is specified as a percentage. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/87b9d21672a8dc7aaf376be217ac62eb4c128610
Bug 1247929 patch 3 - Reftests. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/b55f1a5a8dae044b9a7cdb387d27d5da0caa20b8
Bug 1247929 patch 4 - Adjust reftest manifest for existing tests. r=dholbert
Comment 20•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #17)
> So, no action we need to take here for this I think, aside from perhaps
> filing a Chrome bug.
I filed these bugs on the side issue discovered with input type="file" here:
https://bugs.chromium.org/p/chromium/issues/detail?id=589555
https://bugs.webkit.org/show_bug.cgi?id=154646
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8722855 [details]
MozReview Request: Bug 1247929 patch 1 - Add GetType() overrides for nsProgressFrame and nsMeterFrame. r?dholbert
Approval Request Comment
[Feature/regressing bug #]: bug 823483
[User impact if declined]: incorrect page layouts
[Describe test coverage new/current, TreeHerder]: reftests in tree
[Risks and why]: Relatively low risk; the behavior change the patch is intending to make is to revert to previous behavior for most but not all form controls. There's always a slight risk of unintended behavior change, but if that happens I think we'll still catch it with plenty of time.
[String/UUID change made/needed]: No.
Attachment #8722855 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8722856 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8722857 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8722858 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f03658208cc64bc1c1195804933c0ada4e4aec5
Bug 1247929 patch 5 - Remove spurious canvas rule from reftests. r=dholbert
Comment 23•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7defbcbde4ce
https://hg.mozilla.org/mozilla-central/rev/89e5af841e9d
https://hg.mozilla.org/mozilla-central/rev/87b9d21672a8
https://hg.mozilla.org/mozilla-central/rev/b55f1a5a8dae
https://hg.mozilla.org/mozilla-central/rev/4f03658208cc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 24•9 years ago
|
||
Comment on attachment 8722855 [details]
MozReview Request: Bug 1247929 patch 1 - Add GetType() overrides for nsProgressFrame and nsMeterFrame. r?dholbert
Regression fixes, let's uplift all of this to aurora.
Attachment #8722855 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8722856 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8722857 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8722858 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 25•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•