1247929 - In Firefox DeveloperEdition 46 the table layout algorithm has sizing-problems with some form fields like <input type="number" />.

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Pirmin]

Attachment: ff46_bug_input_table.PNG

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.

Comment 2

[Loic]

Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=65e84a
da79e41a3afdcb0d8148a84a7152888df1&tochange=623934e9db8ab60eb605fba84e03e882651e
8f02

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: reporter's testcase

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: additional tests

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking, recent regression. 
Please request uplift once you have a fix, thanks!

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: MozReview Request: Bug 1247929 patch 1 - Add GetType() overrides for nsProgressFrame and nsMeterFrame. r?dholbert

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/

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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

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/

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: MozReview Request: Bug 1247929 patch 3 - Reftests. r?dholbert

Review commit: https://reviewboard.mozilla.org/r/36269/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36269/

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: MozReview Request: Bug 1247929 patch 4 - Adjust reftest manifest for existing tests. r?dholbert

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/

Comment 13

[Daniel Holbert [:dholbert]]

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

[Daniel Holbert [:dholbert]]

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.)

Comment 15

[Daniel Holbert [:dholbert]]

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

[Daniel Holbert [:dholbert]]

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

Comment 17

[Daniel Holbert [:dholbert]]

(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

[Daniel Holbert [:dholbert]]

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.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Daniel Holbert [: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 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f03658208cc64bc1c1195804933c0ada4e4aec5
Bug 1247929 patch 5 - Remove spurious canvas rule from reftests.  r=dholbert

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

   https://hg.mozilla.org/releases/mozilla-aurora/rev/7f3e715cc12a
   https://hg.mozilla.org/releases/mozilla-aurora/rev/b1613f6a8d29
   https://hg.mozilla.org/releases/mozilla-aurora/rev/15e79eafeaac
   https://hg.mozilla.org/releases/mozilla-aurora/rev/7557d8e62bd9