Closed Bug 1620575 Opened 5 years ago Closed 5 years ago

Fix dialog cut-off when dialog description wraps

Categories

(Toolkit :: XUL Widgets, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla77
Tracking Status
firefox77 --- verified

People

(Reporter: ntim, Assigned: emilio)

References

Details

Attachments

(2 files)

Attached image Screenshot from Alice0775 (deleted) —

This can be tested with attachment 9124657 [details] on Windows.

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

Created attachment 9131450 [details]
Screenshot from Alice0775

This can be tested with attachment 9124657 [details] on Windows.

Given that it's dependent on text wrapping, does it require the japanese locale (and Windows' default japanese font) to reproduce?

The fix from bug 1600919 might be a sufficient workaround here...

On Windows10 English version, it may be reproducible if text zoom to 150% in Windows Settings/Ease of Access.

The priority flag is not set for this bug.
:bgrins, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(bgrinstead)

Seems like we keep seeing similar things (Mark mentioned an addon related issue in Bug 1531445). Gijs, the commit message in Bug 1600919 mentions working around a XUL layout bug. It's not clear to me exactly what the layout bug is - is there a bug on file for that issue? Is it something that could be fixed in the layout engine or have we already decided that's not viable?

Flags: needinfo?(bgrinstead) → needinfo?(gijskruitbosch+bugs)
Priority: -- → P3

(In reply to Brian Grinstead [:bgrins] from comment #4)

Seems like we keep seeing similar things (Mark mentioned an addon related issue in Bug 1531445). Gijs, the commit message in Bug 1600919 mentions working around a XUL layout bug. It's not clear to me exactly what the layout bug is - is there a bug on file for that issue? Is it something that could be fixed in the layout engine or have we already decided that's not viable?

I added a workaround (namely: measuring the height of the wrapping element and explicitly assigning it on the node) for the reset profile dialog in bug 1600919. This is insufficient when the dpi/scaling changes while the dialog is up (which changes the text size which changes the wrapping of the box). There's some discussion about this in bug 1609446, especially the last 2 comments. I think those comments do imply that the "real" bug here is in layout, which doesn't measure the "preferred" size for these boxes correctly when the text wraps. Perhaps that means bug 1609446 is that layout bug? I'm not sure. I'm also not 100% sure on what the root cause of the problem is - clearly, sometimes, we get the size correct, and sometimes we do not. Emilio, do you understand the details here - and do we need a separate bug for that issue?

We could add a similar workaround here, but at some point this doesn't really scale.

What I'm more interested in is if it is possible to switch this dialog over to non-XUL layout entirely, and if that would fix the issue completely. This particular dialog is commonDialog.xhtml, which is already using a bunch of HTML, so in theory it should be easy to do. It's also used for all alert/prompt/confirm/confirmEx calls, which means we'd fix a lot of wrapping issues if we fixed just this dialog, plus we could use such a fix as a template for how to convert other dialogs.

Emilio, would switching away from XUL box layout fix anything here? Or would we still have a similar issue determining the preferred size of this box?

(for the avoidance of doubt, you can reproduce the issue in comment #0 by using Windows, setting your overall DPI scaling to 100%, and text scaling to 150%, and then creating one of the affected dialogs using the link in attachment 9124657 [details])

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

Does sizeToContent() do the right thing on Windows if you call it from the inspector on the bogus dialog? Is there any chance to repro on Linux so that I can dig using rr?

As far as I can tell, what gets the initial size of commonDialog is this line of code. If layout changes afterwards you'd need to call that again to get the right size...

Then there's the reflow code which populates these metrics and which is quite an abomination and a bit hacky. I'm really not confident that does the right thing for non-block frames.

And even for block frames, this code which is computing the actual size we'd use seems to only account for the tallest line. That seems wrong.

Seems like an attempt for a fix was done in bug 413027. The nsFrame.cpp bits of "fix v4" are roughly what I'd expect the right fix for this to be... I can try to see how that holds up on automation.

So ok, from Riot chat with Gijs, sizeToContent after the fact does not fix it, which is weird. sizeToContent always prevents the text from being wrapped in Linux (as I'd expect).

One of the things to try is whether width: min-content on the <description> avoids the text wrapping. That means that the issue is probably somewhere in the XUL / block layout interaction. If that does not fix it, then chances are this is a regular block layout bug that we can reproduce outside of XUL.

Another potential fix is this, but hard to say without being able to repro:

diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
index 8dbd0cab0e9f..736995ee9df6 100644
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -10472,30 +10472,8 @@ nsFrame::RefreshSizeCache(nsBoxLayoutState& aState) {
     BoxReflow(aState, presContext, desiredSize, rendContext, rect.x, rect.y,
               metrics->mBlockPrefSize.width, NS_UNCONSTRAINEDSIZE);
 
-    metrics->mBlockMinSize.height = 0;
-    // ok we need the max ascent of the items on the line. So to do this
-    // ask the block for its line iterator. Get the max ascent.
-    nsAutoLineIterator lines = GetLineIterator();
-    if (lines) {
-      metrics->mBlockMinSize.height = 0;
-      int count = 0;
-      nsIFrame* firstFrame = nullptr;
-      int32_t framesOnLine;
-      nsRect lineBounds;
-
-      do {
-        lines->GetLine(count, &firstFrame, &framesOnLine, lineBounds);
-
-        if (lineBounds.height > metrics->mBlockMinSize.height)
-          metrics->mBlockMinSize.height = lineBounds.height;
-
-        count++;
-      } while (firstFrame);
-    } else {
-      metrics->mBlockMinSize.height = desiredSize.Height();
-    }
-
-    metrics->mBlockPrefSize.height = metrics->mBlockMinSize.height;
+    metrics->mBlockMinSize.height = desiredSize.Height();
+    metrics->mBlockPrefSize.height = desiredSize.Height();
 
     if (desiredSize.BlockStartAscent() == ReflowOutput::ASK_FOR_BASELINE) {
       if (!nsLayoutUtils::GetFirstLineBaseline(wm, this,
Flags: needinfo?(emilio)

Sorry, the above comment should say max-content instead of min-content.

I can repro the issue with Cantarell 13pt in the gnome font settings and this patch:

diff --git a/toolkit/locales/en-US/chrome/search/search.properties b/toolkit/locales/en-US/chrome/search/search.properties
index 7891314fa8de..3167e044802f 100644
--- a/toolkit/locales/en-US/chrome/search/search.properties
+++ b/toolkit/locales/en-US/chrome/search/search.properties
@@ -3,7 +3,7 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 addEngineConfirmTitle=Add Search Engine
-addEngineConfirmation=Add “%S” to the list of engines available in the search bar?\n\nFrom: %S
+addEngineConfirmation=iAdd “%S” to the list of engines available in the search bar?\n\nFrom: %S
 addEngineAsCurrentText=Make this the c&urrent search engine
 addEngineAddButtonLabel=Add
Flags: needinfo?(emilio)

If we get a sub-pixel content size, truncating may actually cause the content to
wrap, which is not desirable and would cause the content to be truncated.

Assignee: nobody → emilio
Status: NEW → ASSIGNED

Can you confirm the patch fixes it on windows too when you have some time?

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/77089baefccf Ensure that sizeToContent ceils when converting to dev pixels. r=dholbert

Yes, this seems to be fixed for me. Alice, when you have a moment, could you confirm this is also fixed for you when using the Japanese locale?

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

I can manage to reproduce the issue on Nightly76.0a1(20200406092400)(en-US) Windows10(Japanese 150%).
And I can no longer reproduce the issue on autoland(77089baefccf).

Flags: needinfo?(alice0775)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

Thanks, Alice!

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: