1610597 - The RDM "Device simulation changes require a reload..." info-bar isn't large enough for its text, if the text wraps

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Daniel Holbert [:dholbert]]

Attachment: screenshot

STR:

1. Start in a fresh profile (this is important - otherwise the info-bar may not show up, I think)
2. Visit any page, e.g. this page here
3. Open responsive design mode (Ctrl+Shift+M on Linux)
4. Click the device switcher menu (which starts out labeled "Responsive" at top, to the left of center)
5. Pick any option, e.g. the top one "Galaxy S9"
6. (optional) Resize your browser to make it skinnier. Or perform the STR with an already-skinny browser.

ACTUAL RESULTS:
In step 5, an info-bar appears telling you that you may need to reload. If your browser is skinny enough so that the info-bar text wraps (which it does with entirely reasonably-sized browser windows), then the wrapped text is just clipped and doesn't show up.

EXPECTED RESULTS:
When the text linewraps, the info-bar should become taller so that it can hold its text.

Comment 1

[Daniel Holbert [:dholbert]]

Regression pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5e6b050c9427da9ae37ca4c7c8000ba5b47ceea8&tochange=1a85c571a4645798ba4d9295ba2504acac149ac8

--> regression from bug 1576946. ntim, do you know if this is a known issue and/or if there's a known easy-fix?

Comment 2

[Daniel Holbert [:dholbert]]

I verified that current Firefox beta (73.0b7) is affected by this.

[Tracking Requested - why for this release]: Visual regression (broken UI) introduced in the Firefox 73 nightly cycle.

Comment 3

[Tim Nguyen :ntim]

Attachment: Bug 1610597 - Use legacy-stack for notification boxes. r=bgrins

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 9122203 [details]
Bug 1610597 - Use legacy-stack for notification boxes. r=bgrins

Beta/Release Uplift Approval Request

• User impact if declined: See attachment 9122115 [details]
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See 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): Reverts things the way they were.
• String changes made/needed: none

Comment 5

[Tim Nguyen :ntim]

When I set display: block/flow-root/inline-block on .notificationbox-stack (as opposed to the current display: grid) the height seems to be computed correctly, but we do need the grid functionality in our case. I also tried height: max-content on various elements, but that did not make a difference.
Daniel, do you know if this is the expected behaviour ?

The main reason this is using a stack atm is to be able to overlay multiple notifications, but only display the last one on top and being able to animate them.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

So I'm clear, are we intending to land this patch on m-c too or just Beta? Given that we're getting close to having 74 go to Beta, I'd prefer the former if possible.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)

So I'm clear, are we intending to land this patch on m-c too or just Beta? Given that we're getting close to having 74 go to Beta, I'd prefer the former if possible.

I agree with RyanVM -- given how small the patch is (and how large the regressing patch was, which makes the not-risky-assessment from comment 4 a little debatable), I'd be more comfortable having this patch land in Nightly as well, and then we can revert it on Nightly as part of a better non-legacy-stack fix.

(In reply to Tim Nguyen :ntim from comment #5)

When I set display: block/flow-root/inline-block on .notificationbox-stack (as opposed to the current display: grid) the height seems to be computed correctly, [...]
Daniel, do you know if this is the expected behaviour ?

I'll take a look today or tomorrow and see if I can figure out what's going on.

Comment 8

[Tim Nguyen :ntim]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)

So I'm clear, are we intending to land this patch on m-c too or just Beta? Given that we're getting close to having 74 go to Beta, I'd prefer the former if possible.

<legacy-stack> has not landed on 74 at the moment, so this patch should only land on 73.

It's a non-zero effort to also land the stack frame removal backout on Nightly, but I could potentially reconsider it later on if needed.

I'll take a look today or tomorrow and see if I can figure out what's going on.

Thanks!

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Tim Nguyen :ntim from comment #8)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)

So I'm clear, are we intending to land this patch on m-c too or just Beta? Given that we're getting close to having 74 go to Beta, I'd prefer the former if possible.

<legacy-stack> has not landed on 74 at the moment, so this patch should only land on 73.

Ah, ok. Given that, the beta-only nature makes sense, I think.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9122203 [details]
Bug 1610597 - Use legacy-stack for notification boxes. r=bgrins

Beta-only fix to work around the underlying bug for 73. Approved for 73.0b9.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cfec84ed3977

Comment 12

[Daniel Holbert [:dholbert]]

RE why this isn't working with CSS Grid: I haven't looked in gdb yet, but I poked around this element and its contents in the Browser Toolbox, and it looks to me like we're measuring the grid item's preferred size as if there were no wrapping (rather than measuring the preferred size given the limited width & the wrapping that it imposes).

This feels like bug 1593060, in that we're probably we're falling prey to impedance mismatches between our XUL layout model vs. our CSS layout model (specifically for the grid item's content-height measurement - we might be getting a value that XUL calculates optimistically as if there were no width constraint, or something like that).

Comment 13

[Alin Ilea, Desktop QA]

Reproduced the issue using a Nightly build 74.0a1 (2020-01-22).
Verified - Fixed in latest Beta 73.0b9 (Build id: 20200123022532) using Windows 10, Ubuntu 18.04 and Mac OS 10.13.

Comment 14

[Brad Werth [:bradwerth]]

Comment 12 makes this sound like an issue in Layout. I'm moving the bug over to that component for triage.

Comment 15

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

Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See What Do You Triage for more information

Comment 16

[Tim Nguyen :ntim]

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

RE why this isn't working with CSS Grid: I haven't looked in gdb yet, but I poked around this element and its contents in the Browser Toolbox, and it looks to me like we're measuring the grid item's preferred size as if there were no wrapping (rather than measuring the preferred size given the limited width & the wrapping that it imposes).

FWIW, Bug 1606617 seems to be like a similar issue, where the container size is measured without taking account the wrapping text. After talking to Neil Deakin in person, he suggested triggering some sort of reflow after the text has been wrapped might work. Is it something that you could possibly try ? I don't know enough about the layout code to do this unfortunately.

Side note: I did also try changing the child elements to HTML as it did fix a similar issue in bug 1602939, but that was without any success.

Comment 18

[Daniel Holbert [:dholbert]]

I'm going to take a look at this today, to hopefully see if there's a clear fix/workaround...

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1 (requires XUL to be enabled)

Here's a reduced testcase that shows the behavior-difference between "true" XUL stacks vs. our current emulated-with-grid XUL stacks. (This testcase uses XUL and hence can only be viewed in certain configurations, e.g. by viewing a locally-saved copy locally and flipping the pref described at https://developer.mozilla.org/en-US/docs/Archive/Mozilla/XUL/Using_Remote_XUL )

EXPECTED RESULTS: The bordered boxes should shrinkwrap the full height of the paragraph.
ACTUAL RESULTS: The bordered boxes are only as tall as a single line of text.

Firefox versions with "true" <stack> support give EXPECTED RESULTS.
Firefox versions with css grid emulation give ACTUAL RESULTS.

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: screenshot of reduced testcase (expected results on left, from old Firefox version)

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: reference case 1 (using 'legacy-stack', which works in Firefox 72 and 73 at least)

Here's a reference case, using legacy-stack which (per the attached uplifted bandaid) gets us the old stack behavior in Firefox 72 and 73 (but not in Nightly currently, I think, and probably also not in the branching-from-Nightly-this-week beta version)

Comment 22

[Daniel Holbert [:dholbert]]

Actually, I spun off bug 1614447 as a single place to cover the underlying cause of this bug, i.e. the piece of <stack> emulation that's currently broken. I'm going to mark comment 19 through 21 as obsolete, since I'll be moving their contents/ information over to bug 1614447.

This bug here can be left covering specifically the RDM issue (which we have a frontend band-aid for, on 73 but not on 74/75 at this point, per comment 4 and comment 8.

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Tim Nguyen :ntim from comment #8)

It's a non-zero effort to also land the stack frame removal backout on Nightly, but I could potentially reconsider it later on if needed.

ntim, would you mind landing this <stack>-frame removal-backout on Nightly, so that we can use <legacy-stack> here & in related bugs for the time being?

The testcases on bug 1614447 show that intrinsic sizing is pretty broken in both axes for stack-emulated-via-grid right now. (Particularly from the perspective of the stack's parent box, which probes the stack for its preferred size as an input to the parent-box's XUL layout.) It's possible we can fix it, but I'm unsure about how straightforward it'll be & how robust the fix will be, given that sizing/reflow happen at different times in XUL layout vs. our modern layout codepaths, and the sizes that we need here ("early" in the XUL algorithm, for the parent-box's XUL layout) are the results from information that we calculate "late" in the modern reflow codepath. Also: even if we come up with a fix, there may be other cases where we need to revert to the <legacy-stack> behavior to stave off some UI bustage that we don't have the resources to investigate/fix immediately.

So I'd like to have the <legacy-stack> codepath available to us for the time being, so we can mitigate this bug and the associated bugs without being gated on XUL reflow rr-debugging & hacks.

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

Tim, is a beta-only fix needed for uplift in 74 as we did for 73 or is a permanent fix in the works that we will uplift instead? Thanks

Comment 25

[Tim Nguyen :ntim]

Attachment: Bug 1610597 - Re-introduce display: -moz-stack; as <legacy-stack>.

Comment 26

[Tim Nguyen :ntim]

(In reply to Pascal Chevrel:pascalc from comment #24)

Tim, is a beta-only fix needed for uplift in 74 as we did for 73 or is a permanent fix in the works that we will uplift instead? Thanks

Probably a beta-only fix, but could be landed on 75 as well depending on bug 1614447 (which is the permanent fix).

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim from comment #26)

(In reply to Pascal Chevrel:pascalc from comment #24)

Tim, is a beta-only fix needed for uplift in 74 as we did for 73 or is a permanent fix in the works that we will uplift instead? Thanks

Probably a beta-only fix, but could be landed on 75 as well depending on bug 1614447 (which is the permanent fix).

We've uplifted this at least a couple betas in a row now. Unless if we expect to fix all consumers relying on it in this release (do we?) we should land this on central to save everyone time after next merge IMO.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d7c5ffe9245
Re-introduce display: -moz-stack; as <legacy-stack>. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cb0238b3e53b
Use legacy-stack for notification boxes. r=bgrins

Comment 29

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/5d7c5ffe9245
https://hg.mozilla.org/mozilla-central/rev/cb0238b3e53b

Comment 30

[Tim Nguyen :ntim]

Comment on attachment 9126434 [details]
Bug 1610597 - Re-introduce display: -moz-stack; as <legacy-stack>.

Beta/Release Uplift Approval Request

• User impact if declined: See comment 0
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See 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): Back out, may be slightly risky because it's only a partial backout (eg. with manual changes).
• String changes made/needed:

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9126434 [details]
Bug 1610597 - Re-introduce display: -moz-stack; as <legacy-stack>.

Same fix we've been taking in previous releases. Approved for 74.0b4.

Comment 32

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/54e0450480a4
https://hg.mozilla.org/releases/mozilla-beta/rev/7a308383a485

Comment 33

[Alin Ilea, Desktop QA]

Verified - Fixed in latest Nightly 75.0a1 (Build id: 20200216210001) and latest Beta 74.0b4 (Build id: 20200216164042) using Windows 10, Mac OS 10.13 and Ubuntu 18.04.