Closed Bug 1247929 Opened 4 years ago Closed 4 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)

46 Branch
defect
Not set

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)

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
Duplicate of this bug: 1247897
Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=65e84a
da79e41a3afdcb0d8148a84a7152888df1&tochange=623934e9db8ab60eb605fba84e03e882651e
8f02
Blocks: 823483
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dbaron)
Keywords: regression, testcase
OS: Unspecified → All
Hardware: Unspecified → All
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: nobody → dbaron
Status: NEW → ASSIGNED
Tracking, recent regression. 
Please request uplift once you have a fix, thanks!
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.
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)
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)
Attachment #8722855 - Flags: review?(dholbert) → review+
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 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+
Attachment #8722857 - Flags: review?(dholbert) → review+
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 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+
(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.
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.
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
(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
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?
Attachment #8722856 - Flags: approval-mozilla-aurora?
Attachment #8722857 - Flags: approval-mozilla-aurora?
Attachment #8722858 - Flags: approval-mozilla-aurora?
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+
Attachment #8722856 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8722857 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8722858 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.