1540176 - Setting `display: inline-block` for `<input type="text"> with font inflation inflates text in width-constrained input text element

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

Attachment: patch_add_display_inline_block.txt

Steps to reproduce:

1. Apply attached patch (sets display: inline-block; for all input elements in "layout/style/res/forms.css").

2. Run corresponding reftest: ./mach reftest layout/reftests/font-inflation/input-text-1-noheight.html

Expected result: the test still passes, because the text inside the input element isn't inflated.
Actual result: it fails because the text inside the input element is inflated too.

Comment 1

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

Affects Nightly 68.0a1 and likely earlier versions.

Comment 2

[Daniel Holbert [:dholbert]]

Note that font inflation is disabled by default (since bug 1127441) because it's not clear it's a net win, as described in that bug.

So, unless that changes, this is probably pretty low-priority. If this is blocking you from proceeding with the patch_add_display_inline_block.txt approach on some other bug, I'd suggest doing the following 2 things:
(1) Annotating input-text-1-noheight.html as failing in reftest.list, with a mention of this bug number in a comment alongside the annotation.
(2) Creating a copy of that test (maybe named e.g. input-text-1-inline-noheight.html) with an additional explicit display:inline on the input element. This version presumably would still pass, and would continue to serve the regression-testing purpose that the testcase currently serves.

Comment 3

[Daniel Holbert [:dholbert]]

To be clear, I think you should do those things in one of the patches on whatever bug is making the display: inline-block change for inputs -- not on this bug here. This bug here should remain open, to track the known test-failure/defect.

Comment 4

[Daniel Holbert [:dholbert]]

(It might also be pretty easy to fix this bug, so feel free to try your hand at that instead, if you'd like. :) But, it's probably not a particularly important thing to do.)

Comment 5

[Jan Henning [:JanH]]

(In reply to Daniel Holbert [:dholbert] from comment #2)

Note that font inflation is disabled by default (since bug 1127441) because it's not clear it's a net win, as described in that bug.

Though we might take the opportunity of the GeckoView-based rewrite to have another go at re-enabling it.

(In reply to Daniel Holbert [:dholbert] from comment #4)

(It might also be pretty easy to fix this bug, so feel free to try your hand at that instead, if you'd like. :) But, it's probably not a particularly important thing to do.)

This is just guessing based on what I can remember from looking at font inflation last year, and I didn't have the opportunity to actually look at what's happening, so the following may be completely off the mark (but hopefully it isn't):

1. Originally, "font inflation containers" [1] were the smallest layout unit the font inflation operates on, i.e. for which font inflation can be turned off. This is to ensure that frames that are closely related are always inflated together, i.e. all or nothing. Apart from a few special cases, basically any non-inline frame (including inline-block) will be a font inflation container.
2. Through bug 708175 however, which introduced that reftest in question, we gained some code (the code today) that can turn off inflation for certain frames inside a font inflation container.
3. Therefore my guess would be that because those inputs elements have become inline-block, they are now font inflation containers in their own right and hence the original logic no longer applies to them.
4. Therefore, if I'm not mistaken (dbaron might be able to correct me if I'm talking nonsense), this function now needs to be adapted to detect and handle this particular case.

[1] Scott Johnson wrote an article about how it works. This archived link has working footnotes, but only small images (the big versions weren't archived at that time), whereas the live version has the full-res images available, but broken footnotes.

Comment 6

[Daniel Holbert [:dholbert]]

(I think you mean to linkify "[this function]" in section (4) of your comment 5? You can use the pencil icon at top right of the comment to edit it & add a link target.)

Comment 7

[Jan Henning [:JanH]]

Whoops, yes. Thank you.

Comment 8

[Jan Henning [:JanH]]

Note that font inflation is disabled by default (since bug 1127441) because it's not clear it's a net win, as described in that bug.

Font inflation is enabled again by default in Firefox Preview/Fenix (and has been for a while now).

Comment 9

[Jan Henning [:JanH]]

(In reply to Jan Henning [:JanH] from comment #5)

4. Therefore, if I'm not mistaken (dbaron might be able to correct me if I'm talking nonsense), this function now needs to be adapted to detect and handle this particular case.

It seems I was slightly mistaken, and the proper fix is more likely making sure an <input> element doesn't become a font inflation container in the first place, similar to bug 786946.

Comment 11

[Jan Henning [:JanH]]

Attachment: Bug 1540176 - Ensure input elements don't become font inflation containers. r?emilio

Previously, text input controls weren't font inflation containers simply by
virtue of being "display:inline" by default, which automatically makes them in-
eligible for becoming an inflation container.
As of bug 1539469 this has changed however - those <input> elements are now
"display:inline-block" by default, which with the current font inflation logic
turns them into font inflation containers.

This leads to a few problems:

1. The logic from bug 708175 (stop inflation if there is a size-constrained non-
   inline frame in the chain from the current frame to their font inflation
   container) is built on the assumption that the (possibly size-constrained)
   form control itself isn't a font inflation container.
2. When form controls end up as font inflation containers themselves, they no
   longer size themselves properly to match the size of their inflated
   contents, because they are now subject to the AutoMaybeDisableFontInflation/
   mInflationDisabledForShrinkWrap logic which ends up disabling font inflation
   during the size calculation of the form control.

1.) means that we now inflate some text inputs that we didn't use to inflate
previously and 2.) means that every time we attempt to inflate a text input, we
end up with the text content being inflated, but the containig box being not and
therefore too small.

Because of this, as well as because

1. The introductory comment in nsFrame::IsFontSizeInflationContainer itself
   mentions that form controls aren't expected to be inflation containers.
2. There is the precedent from bug 786946, where <select> elements were
   specifically excluded from becoming font inflation containers when their
   default display style was changed from "inline" to "inline-block".

all of this points towards having to specifically preclude <input> elements
from becoming font inflation containers as well.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1201db1b8cc
Ensure input elements don't become font inflation containers. r=emilio

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a1201db1b8cc