Open Bug 1609446 Opened 4 years ago Updated 2 years ago

Refresh Firefox text pushed buttons out of window when adjusting scale

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

ASSIGNED
Tracking Status
firefox-esr68 --- wontfix
firefox72 --- wontfix
firefox73 --- fix-optional
firefox74 --- fix-optional

People

(Reporter: cfogel, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached image prScale.gif

Affected versions

  • 74.0a1 (2020-01-14);
  • 68.4.1esr, 73.0b7, 72.0.2;

Affected platforms

  • Windows 10;

Steps to reproduce

  1. Launch Firefox access about:profiles
  2. Click on the Restart with addons disabled... button;
  3. Change the Scale value to 125%;
  4. Click on the Refresh Firefox button;

For Windows 10:

  • right Click to bring up Context menu;
  • Display Settings;
  • scroll to Scale and layout section;
  • change to 125%

Expected result

  • the window for Refresh Firefox is disabled and content is properly contained and displayed;

Actual result

  • text from the description is pushed on the next row causing the buttons to appear truncated;

Regression range

  • Pushlog URL
  • Last good build_date: 2018-11-12
  • First bad build_date: 2018-11-13

Additional notes

  • attached recording with the issue;
  • follow up for 1600919.
See Also: → 1600919

FWIW, I think both this and https://bugzilla.mozilla.org/show_bug.cgi?id=1608910#c4 are likely related to the same underlying issue, where it seems like at some point we do a layout, the text in a wrapping <description> fits on a single line, we determine the height of the single line of text to be the minimum intrinsic height for the element, and that gets used to decide the size of the window/frame/whatever. Then when the width gets constrained and the text wraps, we do not re-assess the minimum required height for the element and/or don't re-process it into the required height for the window.

Apologies for the imprecise terms here - not a layout expert! - but based on the conversation on slack, it seems :dholbert and :mats are the experts - :dholbert is out, :mats, would it be possible for you to take a look? Based on the aforementioned conversation, I'm concerned that the rationale to wontfix bug 1293242 (which was another manifestation of this general problem, I think) won't actually apply, ie switching to HTML elements for all of the things in the dialog likely wouldn't fix this issue.

Flags: needinfo?(mats)
See Also: → 1608910

And just to summarize what we did in bug 1600919: we worked around the issue by "manually" measuring the height of the wrapping <description> when the dialog has loaded, using JS, and then setting that height on the <description> (using style, in pixels). Of course, this minimum height stops being accurate when the font size changes as in the STR for this bug; although we could try to figure out the font size as well and then setting the height in em, it probably still wouldn't work if the wrapping changes - and in any case this just starts getting more and more cludgy...

Priority: -- → P3
Flags: needinfo?(emilio)

Updated the regression field in the comment 0, hope it helps.

Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
Version: 74 Branch → Trunk

Is there any way to get a reduced test-case / repro on Linux? I tried to get the dialog to show up and then change the scaling, but that didn't work here. I can repro bug 1608910 comment 4 though, so I can try to poke there.

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Is there any way to get a reduced test-case / repro on Linux? I tried to get the dialog to show up and then change the scaling, but that didn't work here. I can repro bug 1608910 comment 4 though, so I can try to poke there.

STR:

  1. open nightly with a pre-existing profile to about:support
  2. open the browser console (ctrl-shift-j), ensure you can enter JS (need to flip the devtools chrome/remote debugging pref if this isn't a local build etc.)
  3. click the "Refresh Firefox" button on about:support
  4. in the browser console, run:
Array.from(Services.wm.getEnumerator("")).find(w => w.location.href.endsWith("resetProfile.xhtml")).document.documentElement.style.fontSize = (1.5 / 1.25) + "em"

ER: text size changes and the window changes to still fit the content
AR: text size changes, the window size changes, but the content still doesn't fit.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)

I don't see the window size changing at all... Which is what I'd expect out of that. Dialogs are sized via explicit calls to window.sizeToContent() in the front-end JS, and there's nothing triggering that... Of course if I call sizeToContent() again the dialog resizes as expected. Is there anything I'm missing?

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

I don't see the window size changing at all... Which is what I'd expect out of that.

I don't see a lot of change, but it's there. It's more obvious if you use e.g. 2em as the font-size, and esp. if you then removeProperty("font-size") again, which then produces a gap between the dialog text and the buttons.

Dialogs are sized via explicit calls to window.sizeToContent() in the front-end JS, and there's nothing triggering that...

I'm pretty sure not all dialogs call this? This dialog didn't call it prior to the patch in bug 1600919, and the issue in this bug predates that patch (and regressed in 2018, whereas the regression window noted in bug 1600919 is from 2019...).

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)

I see zero change in the outer dialog size regardless of the font-size I set, fwiw.

Ok, so the only other way that anything can trigger the sizeToContent behavior is from AppWindow::SizeShell(). That happens for windows with mIntrinsicallySized=true, which is true if there are no width / height attributes and no width / height passed to openDialog / etc.

But that happens from BeforeStartLayout (~DOMContentLoaded + l10-ready + stylesheets-ready), and OnChromeLoaded (not 100% clear what it maps to, but not something that gets called dynamically).

So I'm not sure how it could ever work changing font-size dynamically.. Do you have a build / revision where it did? Resolution changes may or may not proportionally change the scale of the window or something depending on the actual widget back-end, I guess but it's unclear to me how random layout changes inside the dialog would be expected to affect the dialog size without sizeToContent() calls from the front-end.

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

I don't see the window size changing at all...

I do see window size changes, both on Linux and on Windows. I checked on Windows, and nsWindow::Resize() is called with a stack like this:

 	xul.dll!nsWindow::Resize(double aWidth, double aHeight, bool aRepaint) Line 1836	C++
 	xul.dll!nsBaseWidget::SetSizeConstraints(const mozilla::widget::SizeConstraints & aConstraints) Line 1631	C++
 	xul.dll!nsWindow::SetSizeConstraints(const mozilla::widget::SizeConstraints & aConstraints) Line 1723	C++
 	xul.dll!nsContainerFrame::SetSizeConstraints(nsPresContext * aPresContext, nsIWidget * aWidget, const nsSize & aMinSize, const nsSize & aMaxSize) Line 670	C++
 	xul.dll!nsContainerFrame::SyncWindowProperties(nsPresContext * aPresContext, nsIFrame * aFrame, nsView * aView, gfxContext * aRC, unsigned int aFlags) Line 630	C++
 	xul.dll!mozilla::PresShell::SyncWindowProperties(nsView * aView) Line 10926	C++
>	xul.dll!nsViewManager::ProcessPendingUpdatesForView(nsView * aView, bool aFlushDirtyRegion) Line 375	C++
 	xul.dll!nsViewManager::ProcessPendingUpdates() Line 1020	C++

So as far as I can tell, https://searchfox.org/mozilla-central/rev/174f1195ec740e8f17223b48018f7e14e6d4e40e/view/nsViewManager.cpp#371,375 will resize the window based on changes in the document for that window. Does that make sense? Any reason you're not seeing the same thing?

While looking for this, back to the original reason this bug got filed, I also noticed that nsWindow is supposed to respond to DPI changes ( https://searchfox.org/mozilla-central/rev/174f1195ec740e8f17223b48018f7e14e6d4e40e/widget/windows/nsWindow.cpp#7326 ). So I'm still surprised that between both of these functions, the original steps in this bug do not result in the window getting the correct size...

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)

Hmm, so I don't recall hitting those resizes on Linux, but maybe they got optimized away at some point.

The relevant bit of code is this:

So basically the min size of the <window> is the XUL min-size. It seems plausible that the XUL min-size is wrong given <description> uses regular CSS (not XUL) layout. This seems similar to some of the other cases like the notification box that Daniel was investigating, where we also only accounted for 1 of the lines of a grid item, IIRC.

We can compute content-based heights for CSS relatively reasonably, but it is pretty expensive... I wonder if we could do something a bit simpler. Let me give it a shot.

I don't have a build, but if the theory of having the wrong min-size is right this should fix it:

diff --git a/layout/generic/nsContainerFrame.cpp b/layout/generic/nsContainerFrame.cpp
index 2fb9872eb4f4..64c37fda71c6 100644
--- a/layout/generic/nsContainerFrame.cpp
+++ b/layout/generic/nsContainerFrame.cpp
@@ -627,6 +627,11 @@ void nsContainerFrame::SyncWindowProperties(nsPresContext* aPresContext,
       maxSize.height = pos->mMaxHeight.ToLength();
     }
   }
+
+  // Ensure that the size never crops the root frame.
+  minSize.height = std::max(rootFrame->GetSize().height, minSize.height);
+  minSize.width = std::max(rootFrame->GetSize().width, minSize.width);
+
   SetSizeConstraints(aPresContext, windowWidget, minSize, maxSize);
 #endif
 }

Of course that's not good enough, as probably it'd prevent dialogs from ever shrinking...

Is this also fixed by bug 1620575? Otherwise we may need a similar fix (ceiling the min size, flooring the max size around here).

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

Is this also fixed by bug 1620575?

Unfortunately not.

Otherwise we may need a similar fix (ceiling the min size, flooring the max size around here).

Unfortunately this also does not appear to be sufficient - tried with https://paste.mozilla.org/Gh7trr52 per IRC, but same results. Backing out the patch from bug 1600919 (which I imagine might not help the issue at hand, given it fixes a height for some things) doesn't help either.

Do you want to investigate further here? I'm just mindful that this is an edgecase...

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)

Ok, so I took a look, and I finally fully understood what the XUL -> Block abomination is trying to do. And that explains perfectly why it doesn't give the desired results here.

The key is here... To compute the min size, we layout the block with the pref size as the containing size, which means that the result will only have one line.

The problem is we really can't do much better. Given how different XUL and block layout are, we can't get the containing size of our parent, because it depends on our min size, which we're computing.

So the min-size of a block can only ever get one line, effectively... If we want an effective size-to-content behavior automatically, we need to use the pref size as the min width of the dialog. But of course that limits the way you can resize these dialogs in a way that may be not great.

I'm going to send a patch here that does that, but it's somewhat scary... It might be what this code was trying to do in the first place? I don't know.

An alternative to this is setting some sort of em-based min-height in the dialog CSS, or something of that sort, maybe...

Gijs, can you take a look at the patch and confirm that it also fixes the issue in the original test-case, which is on windows, and if you spot something horribly wrong with other dialogs?

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)
Assignee: nobody → emilio
Attachment #9139199 - Attachment description: Bug 1609446 - Shrink-to-fit better in SyncWindowProperties. → Bug 1609446 - Make default window-constraints always show the content. r=mats,mstange
Status: NEW → ASSIGNED
Flags: needinfo?(mats)

OK, I think dialogs in general are fine with this.

In terms of resize-ability, I guess we should have a frontend follow-up. I audited the following query:

https://searchfox.org/mozilla-central/search?q=resizable(%3Dyes%7C%22%7C%27%7C%2C)&case=false&regexp=true&path=

Which gives me (bold are the ones that may want a new min-width CSS thing):

  • places library. This might want a min-width setting (to something reasonable like 200px or whatever?) so you can make it smaller but not ridiculously small.
  • layout debugger. I don't have a debug build to hand so unsure if anything needs to happen there; there's a default height/width set; do dom/layout folks expect to be able to resize it (much) beyond that?
  • browser window is fine
  • bookmark properties - already has a min size which is just fine
  • bunch of stuff in privacy.js, behaviour seems unchanged.
  • page info dialog seems to have correct minimum sizing, where it was slightly off before (which I guess is a consequence of the fix)
  • subdialogs mean lots more prefs stuff (like the privacy.js things); I checked most, they seem OK.
  • browser console seems to have a much higher minimum width than before, may want to address that.
  • no change for browser debugger
  • no change for picture-in-picture
  • no change for dialog.xhtml (the unknown protocol dialog)

I... think that's it?

Flags: needinfo?(gijskruitbosch+bugs)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/887f1769a2c6
Make default window-constraints always show the content. r=mats,mstange
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96ff6ce7acbf
Backed out changeset 887f1769a2c6 for causing bc permafails in browser/base/content/test/webextensions/browser_permissions_installTrigger.js CLOSED TREE
See Also: → 1632899

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:emilio, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(emilio)

I should try to re-land this, but there were some failures that I couldn't repro locally and are on windows. I did some bisecting and it was due to the ceiling bits, which I find it astonishing.

Flags: needinfo?(emilio)
Severity: minor → S4
You need to log in before you can comment on or make changes to this bug.