1825709 - date input positions incorrectly when using min-height

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Adam Cooke]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/111.0

Steps to reproduce:

#div1 input {
min-height: 38px;
}
#div2 input {
height: 38px;
}

<div id='div1'>
<input type='text'/><input type='date'/>
</div>

<div id='div2'>
<input type='text'/><input type='date'/>
</div>

https://jsfiddle.net/2ozf63d4/2/

Actual results:

The position of inputs with a type='date' and a min-height behaves differently to text and number inputs or date inputs with height set instead of min-height.

date inputs with a min-height are offset down

Expected results:

inputs should be positioned the same

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::Layout: Form Controls' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Adam Cooke]

Possibly related to vertical alignment

https://jsfiddle.net/yhg4Lbqw/1/

Comment 3

[David Shin[:dshin]]

Looks like the regression happened around here.
Maybe bug 1698606?

Comment 4

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

The severity field is not set for this bug.
:TYLin, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: testcase 1 (standalone html from the reporter)

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to David Shin[:dshin] from comment #3)

Looks like the regression happened around here.
Maybe bug 1698606?

bug 1698606 changes the baseline of date control. Maybe it's related.

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Oops. I don't mean to take this bug.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1825709 - Misc clean-ups in nsTextControlFrame. r=#layout

Use inline initializers etc.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1825709 - Remove DEBUG-only override of nsTextControlFrame::MarkIntrinsicISizesDirty. r=dshin,#layout

This seems to be here since nsTextControlFrame has a meaningful
baseline, but it doesn't make much sense to me:

• Baseline is a block axis, not inline axis measurement.
• It doesn't call into the base class which seems clearly a bug (though
  the intrinsic isize of the input is ~fixed, doesn't depend on font
  metrics, so it's probably ok).

My guess is that it was intended to be a debug-only check so that we
could detect stale baseline values.

Just remove this. If you think the check is useful we could keep this
but call the base class or something, but not convinced it's worth it.

Depends on D175744

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1825709 - Account for min/max bsize in nsTextControlFrame::ComputeBaseline. r=dshin,#layout

This isn't needed for nsTextControlFrame because its ComputeAutoSize
implementation doesn't return an unconstrained line-height for inputs,
so we never end up in the UNCONSTRAINEDSIZE case, but it's needed for
date/time inputs.

Use GetLineHeight while at it, since it's the inflated line-height which
is what we want, and may be cached so we can avoid computing it.

Maybe in the future we can make date/time inputs just use
nsTextControlFrame, which would prevent this from happening in the
future.

Depends on D175745

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1825709 - Make nsDateTimeControlFrame inherit from nsTextControlFrame. r=#layout

Seems like this should be doable, and would avoid mistakes like this in
the future.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0093f90bfd85
Misc clean-ups in nsTextControlFrame. r=dshin

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d22afc76172
Account for min/max bsize in nsTextControlFrame::ComputeBaseline. r=dshin

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfc64b8c1591
Remove DEBUG-only override of nsTextControlFrame::MarkIntrinsicISizesDirty. r=dshin

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39594 for changes under testing/web-platform/tests

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9ef9922a38b
Make nsDateTimeControlFrame inherit from nsTextControlFrame. r=dshin

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/662b4b7bac25
Tweak assertion count of 1623918.html since it hits both the old and new asserts.

Comment 19

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/0093f90bfd85

Comment 20

[Iulian Moraru]

Backed out for causing wr failures on input-date-content-size.html.

• Push with failures

• Failure log

• Backout link

LATER EDIT: there are also some wpt failures:

• PROCESS-CRASH | /html/semantics/forms/the-input-element/focus-dynamic-type-change.html | application crashed [@ libxul.so + 0x00000000068f447d]
• PROCESS-CRASH | /html/semantics/forms/the-input-element/focus-dynamic-type-change.html | application crashed [@ style::gecko::wrapper::GeckoNode::get_bool_flag]
• SUMMARY: ThreadSanitizer: SEGV /builds/worker/checkouts/gecko/servo/components/style/gecko/wrapper.rs:321:9 in style::gecko::wrapper::GeckoNode::get_bool_flag::h39d63b6be00000d1

Comment 21

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/4d22afc76172
https://hg.mozilla.org/mozilla-central/rev/dfc64b8c1591
https://hg.mozilla.org/mozilla-central/rev/662b4b7bac25

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Ahh, the null crashes could be fixed like so:

diff --git a/layout/forms/nsTextControlFrame.cpp b/layout/forms/nsTextControlFrame.cpp
index b6233ee07c675..c92390d179df8 100644
--- a/layout/forms/nsTextControlFrame.cpp
+++ b/layout/forms/nsTextControlFrame.cpp
@@ -565,7 +565,9 @@ void nsTextControlFrame::AppendAnonymousContentTo(
     aElements.AppendElement(mRevealButton);
   }
 
-  aElements.AppendElement(mRootNode);
+  if (mRootNode) {
+    aElements.AppendElement(mRootNode);
+  }
 }
 
 nscoord nsTextControlFrame::GetPrefISize(gfxContext* aRenderingContext) {

But I don't have the cycles right now to investigate the other failure and I don't think it's important, the bug is fixed with the other changes.

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 24

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2023-04-16 Nightly build on Windows 10.
Verified as fixed on Firefox 114.0b4(build ID: 20230514175823) and Nightly 115.0a1(build ID: 20230515215623) on Windows 10, macOS 12, Ubuntu 22.