Closed Bug 1695578 Opened 4 years ago Closed 4 years ago

Date field truncated (i.e. it has unexpectedly small min-content size)

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
All
Unspecified
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
88 Branch
Tracking Flags:
Tracking Status
relnote-firefox --- 86+
firefox-esr78 --- unaffected
firefox86 + verified
firefox87 + verified
firefox88 --- verified

People

(Reporter: gabriel, Assigned: dholbert)

References

(Blocks 1 open bug, Regression,
URL
)

Details

(Keywords: regression)

Attachments

(3 files)

Bug 1695578 part 1: Add macro to implement HTMLMarqueeElement::FromNodeOrNull, so that some marquee-specific zero-intrinsic-sizing code is actually marquee-specific. r?emilio
48 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
RyanVM
: approval-mozilla-release+
Details | Review
Bug 1695578 part 2: Add testcase variant for input type="time". r?emilio
48 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
RyanVM
: approval-mozilla-release+
Details | Review
testcase 1 (from reporter)
238 bytes, text/html
Details

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0.3 Safari/605.1.15

Steps to reproduce:

date-field inside a table-cell with 1%-widt is truncated (cut off) since Firefox Version 86.
Before it worked fine (sized table-cell to needed size for the date-element).

Sample code to reproduce:

<!DOCTYPE html>
<html>
<body>

<form action="/action_page.php">
<table border="1" cellspacing="0" style="width:100%;">
<tr>
<td style="width:1%;">
<input type="date">
</td>
<td>
other...
</td>
</tr>
</table>
</form>

</body>
</html>

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

Component: Untriaged → Layout: Tables
Product: Firefox → Core

Thanks for the bug report! I can reproduce this. Some extremely-reduced testcases:

data:text/html,DateField:<input type="date" style="width: min-content">
data:text/html,TimeField:<input type="time" style="width: min-content">

mozregression says this is a regression from https://hg.mozilla.org/integration/autoland/rev/6f7e9ff0c23e3844b46ca31481027d5c29040638 ("Bug 1692380 - The intrinsic min isize of the marquee scroller should be zero.").

This is surprising, since that commit was supposed to be <marquee>-specific, and there aren't any marquees involved in the testcase, but I confirmed it with targeted mozregression runs of that changeset (which reproduces the bug) and its parent (which does not).

mozregression --launch 6f7e9ff0c23e3844b46ca31481027d5c29040638 --repo autoland
mozregression --launch f248e0fc35c0eaa5fc2ae7cc5491ee03797cf718 --repo autoland
URL: data:text/html,DateField:<input type=...
Severity: -- → S2
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
status-firefox86: --- → affected
status-firefox87: --- → affected
status-firefox88: --- → affected
status-firefox-esr78: --- → unaffected
Component: Layout: Tables → Layout
Ever confirmed: true
Regressed by: 1692380
Hardware: Unspecified → All
Summary: Date field truncated → Date field truncated (i.e. it has unexpectedly small min-content size)
Version: Firefox 86 → Trunk

Yup, looks like there was a subtle bug in that marquee commit.

We have this code:

static bool IsMarqueeScrollbox(const nsIFrame& aScrollFrame) {
  [...]
  if (MOZ_LIKELY(!aScrollFrame.GetContent()->IsInUAWidget())) {
    return false;
  }
  [...]
  return aScrollFrame.GetParent() &&
         HTMLMarqueeElement::FromNodeOrNull(
             aScrollFrame.GetParent()->GetContent());
}

https://searchfox.org/mozilla-central/rev/63fcc3f1a2cc73488d8986f4cf91fce2cd4b7564/layout/generic/nsGfxScrollFrame.cpp#1035

In a reduced testcase with just an input element, that HTMLMarqueeElement::FromNodeOrNull( call is returning something truthy which is not a marquee -- it's returning (mozilla::dom::HTMLDivElement *) 0x7f67d3210040 in my case just now.

(Also: this happens to affect date/time widgets since they have internal components that get past the "IsInUAWidget" check. I suspect this might affect other sorts of widgets, too.)

emilio, mind taking a look?

Flags: needinfo?(emilio)

(It looks like the problem is that HTMLMarqueeElement doesn't have its own impl of FromNodeOrNull. If I single-step into that function, I end up here in nsGenericHTMLElement::FromNodeOrNull<nsIContent>:

class nsGenericHTMLElement : public nsGenericHTMLElementBase {
[...]
  NS_IMPL_FROMNODE(nsGenericHTMLElement, kNameSpaceID_XHTML)

https://searchfox.org/mozilla-central/rev/63fcc3f1a2cc73488d8986f4cf91fce2cd4b7564/dom/html/nsGenericHTMLElement.h#65

I suspect we just need to add a NS_IMPL_FROMNODE line to https://searchfox.org/mozilla-central/source/dom/html/HTMLMarqueeElement.h , and then this IsMarqueeScrollbox function will become more specific & do what it's intended to do.

Yeah, that seems to do it (cribbing the FROMNODE macro from another element's header file). I verified that the testcase on bug 1692380 still renders correctly, too.

Canceling needinfo -- I'll just take this.

Assignee: nobody → dholbert
Flags: needinfo?(emilio)

[Tracking Requested - why for this release]:
We should track this for 87 and possibly consider it for a dot-release ridealong in 86.

Reasoning: the regressing patch landed very late-in-the-game on 86, with very little Nightly testing, and went pretty much straight-to-release, because we thought it was extremely targeted. (per bug 1692380 comment 7.) This bug report revealed that in fact it was not targeted & had some collateral damage, because the targeting API that we were relying on was actually not implemented in the class in question.

This patch fixes that by actually implementing the targeting API that we're relying on (so that the code in question is in fact targeted at <marquee>, and not at other arbitary elements), to undo the collateral damage from bug 1692380.

tracking-firefox86: --- → ?
tracking-firefox87: --- → ?

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a310542166ff459e1cb45d9b5444aad8640d5664

emilio: if you don't have any review feedback, feel free to trigger lando (assuming there's nothing concerning in that Try run)

For completeness, here's the reporter's original testcase (text modified slightly for clarity).

That's embarrassing on my side, thanks for fixing this :(

Comment on attachment 9206559 [details]
Bug 1695578 part 1: Add macro to implement HTMLMarqueeElement::FromNodeOrNull, so that some marquee-specific zero-intrinsic-sizing code is actually marquee-specific. r?emilio

Beta/Release Uplift Approval Request

  • User impact if declined: Comment 0
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: comment 0
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Fixes a cast so that the fix for bug 1692380 actually just affects marquee.
  • String changes made/needed: none
Attachment #9206559 - Flags: approval-mozilla-beta?
Attachment #9206560 - Flags: approval-mozilla-beta?
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b4cc8e3294f9 part 1: Add macro to implement HTMLMarqueeElement::FromNodeOrNull, so that some marquee-specific zero-intrinsic-sizing code is actually marquee-specific. r=emilio https://hg.mozilla.org/integration/autoland/rev/64bc9459015c part 2: Add testcase variant for input type="time". r=emilio

https://hg.mozilla.org/mozilla-central/rev/b4cc8e3294f9
https://hg.mozilla.org/mozilla-central/rev/64bc9459015c

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox88: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch

Comment on attachment 9206559 [details]
Bug 1695578 part 1: Add macro to implement HTMLMarqueeElement::FromNodeOrNull, so that some marquee-specific zero-intrinsic-sizing code is actually marquee-specific. r?emilio

approved for 87.0b6

Attachment #9206559 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9206560 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Reproduced the issue on Firefox 88.0a1 (2021-03-01) under macOS 10.15.7 by using the reporter's original testcase.

The issue is fixed on Firefox 87.0b6 and Firefox 88.0a1 (2021-03-05). Tests were performed on macOS 10.15.7, Windows 10 and Ubuntu 20.04

Status: RESOLVED → VERIFIED
status-firefox87: fixed → verified
status-firefox88: fixed → verified
Flags: qe-verify+

Emilio, I'd like to take this patch in an 86 dot release, could you request uplift to release? Thanks

Flags: needinfo?(emilio)

Comment on attachment 9206559 [details]
Bug 1695578 part 1: Add macro to implement HTMLMarqueeElement::FromNodeOrNull, so that some marquee-specific zero-intrinsic-sizing code is actually marquee-specific. r?emilio

Beta/Release Uplift Approval Request

  • User impact if declined: comment 0
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: comment 0
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): see comment 23
  • String changes made/needed: none
Flags: needinfo?(emilio)
Attachment #9206559 - Flags: approval-mozilla-release?
Attachment #9206560 - Flags: approval-mozilla-release?
Flags: qe-verify+

Comment on attachment 9206559 [details]
Bug 1695578 part 1: Add macro to implement HTMLMarqueeElement::FromNodeOrNull, so that some marquee-specific zero-intrinsic-sizing code is actually marquee-specific. r?emilio

Approved for 86.0.1.

Attachment #9206559 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #9206560 - Flags: approval-mozilla-release? → approval-mozilla-release+
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27994 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot

Added to the 86.0.1 relnotes:

Fixed truncation of date and time widgets due to incorrect width calculation

relnote-firefox: --- → 86+

Verified that this issue is fixed on Firefox 86.0.1 by using the reporter's original testcase. Tests were performed on macOS 11.2.3, Ubuntu 20.04 and Windows 10.

It seems that by wanting to open the panel every second time it will be dismissed very fast. Here's an attachment. Is this a known issue?

status-firefox86: fixed → verified
Flags: qe-verify+ → needinfo?(dholbert)

(In reply to Catalin Sasca, QA [:csasca] from comment #25)

Verified that this issue is fixed

Great, thanks!

It seems that by wanting to open the panel every second time it will be dismissed very fast. Here's an attachment. Is this a known issue?

I'm not sure if that's known, but it seems probably-unrelated to this sizing bug here. (And for what it's worth, I can't reproduce it locally, on Linux.)

Would you mind filing that as a new bug, and CC me? (If you have time to check for a regression range, all the better, but no worries if you can't.)

Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: