Closed Bug 1610597 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla75
Tracking Status
firefox-esr68 --- unaffected
firefox72 --- wontfix
firefox73 + verified
firefox74 + verified
firefox75 --- verified

People

(Reporter: dholbert, Assigned: ntim)

References

(Depends on 2 open bugs, Regression)

Details

(Keywords: regression)

Attachments

(6 files)

Attached image 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.

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?

Flags: needinfo?(ntim.bugs)
Regressed by: 1576946

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.

Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED

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
Flags: needinfo?(ntim.bugs)
Attachment #9122203 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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.

Flags: needinfo?(dholbert)

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.

Flags: needinfo?(ntim.bugs)

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

(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!

Flags: needinfo?(ntim.bugs)

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

Flags: needinfo?(dholbert)
Flags: needinfo?(dholbert)

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.

Attachment #9122203 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Flags: needinfo?(dholbert)
QA Whiteboard: [qa-triaged]

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 12 makes this sound like an issue in Layout. I'm moving the bug over to that component for triage.

Component: Responsive Design Mode → Layout
Product: DevTools → Core
Priority: -- → P3

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

Priority: P3 → P2

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

Flags: needinfo?(dholbert)
Duplicate of this bug: 1611705
See Also: → 1612306

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

Attachment #9125558 - Attachment filename: text.xhtml → test.xhtml
Depends on: 1614447

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.

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

Flags: needinfo?(ntim.bugs)

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

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

Flags: needinfo?(ntim.bugs)

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

Pushed by ntim.bugs@gmail.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75

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:
Attachment #9126434 - Flags: approval-mozilla-beta?

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.

Attachment #9126434 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Duplicate of this bug: 1612306
Depends on: 1596184
You need to log in before you can comment on or make changes to this bug.