1605724 - [GTK] Dialog boxes are shrank after Bug 1602939

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect, P2)

Description

[Martin Stránský [:stransky] (ni? me)]

Attachment: dialog.png

Dialog boxes on Wayland are shrank on Wayland after Bug 1602939, see the attached picture.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This happens also on X11.

This is because the icon hasn't loaded by the time we size the popup, and when it loads it makes stuff wrap.

This is what I'm seeing in bug 1606617.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Tim, applying this rule to GTK too fixes it.

It changes the sizes of the icons a bit though. An alternative would be to use an HTML <img> instead of list-style-image, as that'd block onload and we'd have a known point in time where we can call SizeToContent.

Alternatively we could probably override nsImageFrame::GetIntrinsicSize so that it works more like <html:img> and interacts better with HTML layout.

Thoughts?

Comment 3

[Tim Nguyen :ntim]

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

Tim, applying this rule to GTK too fixes it.

It changes the sizes of the icons a bit though.

That's my main concern, it may also be possible that different GTK distros have different icon sizes.

An alternative would be to use an HTML <img> instead of list-style-image, as that'd block onload and we'd have a known point in time where we can call SizeToContent.

I thought about that, but unfortunately, each platform uses a different image, so we'd have to duplicate the .question-icon CSS, but adapted for HTML <img>.

Alternatively we could probably override nsImageFrame::GetIntrinsicSize so that it works more like <html:img> and interacts better with HTML layout.

That sounds fine to me, but I don't know much about how much breakage this would cause. I guess you'll know more about this solution :)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1605724 - Call sizeToContent() again when the icon loads if it hasn't loaded yet. r=dao,ntim

https://hg.mozilla.org/mozilla-central/rev/234701139a2a61d1262e609c9d8ac42384ecafda

Removed the following CSS rule:

#iconContainer {
-moz-box-pack: center;
min-height: 55px; /* maximum icon height + icon margin /
min-width: 58px; / maximum icon width + icon margin */
}

Which enforced the size of the icon row.

The icon loads asynchronously, so by the first time we fire DOMContentLoaded it
may not have loaded yet. This means that sizeToContent() will size the window to
an smaller size and stuff will wrap around when it loads.

<image> doesn't block onload so even delaying this wouldn't work.

Instead, call sizeToContent() again when it loads so that we size the window
right again.

Alternatively / on top of this, we could add a wrapper around the <image>
again and fix its width, may be better too...

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1605724 - Ensure loading the dialog icons doesn't change over-all layout. r=dao,ntim

https://hg.mozilla.org/mozilla-central/rev/234701139a2a61d1262e609c9d8ac42384ecafda

Removed the following CSS rule:

#iconContainer {
-moz-box-pack: center;
min-height: 55px; /* maximum icon height + icon margin /
min-width: 58px; / maximum icon width + icon margin */
}

Which enforced the size of the icon row.

The icon loads asynchronously, so by the first time we fire DOMContentLoaded it
may not have loaded yet. This means that sizeToContent() will size the window to
an smaller size and stuff will wrap around when it loads.

<image> doesn't block onload so even delaying this wouldn't work.

Restore the wrapper to ensure loading the icon or not doesn't affect layout of
the dialog.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Dao, ni? for https://phabricator.services.mozilla.com/D58704#1790630. I think that path is fine to land for now regardless anyway but I wanted your opinion.

Comment 7

[Dão Gottwald [:dao]]

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

Dao, ni? for https://phabricator.services.mozilla.com/D58704#1790630. I think that path is fine to land for now regardless anyway but I wanted your opinion.

Replied there

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e410d72f9b0
Call sizeToContent() again when the icon loads if it hasn't loaded yet. r=dao

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7e410d72f9b0

Comment 10

[Viktor Jägersküpper]

This bug affects Firefox 73 beta, although I only see it with the Wayland backend. Do you think this is a "wontfix" for the 73 release?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Viktor Jägersküpper from comment #10)

This bug affects Firefox 73 beta, although I only see it with the Wayland backend. Do you think this is a "wontfix" for the 73 release?

No, we should probably uplift it, specially now that's early enough. Thanks for the reminder.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9118691 [details]
Bug 1605724 - Call sizeToContent() again when the icon loads if it hasn't loaded yet. r=dao,ntim

Beta/Release Uplift Approval Request

• User impact if declined: dialogs appear cut off on Linux (at least with gtk + wayland, but I could repro on kde too).
• Is this code covered by automated tests?: No
• 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): Sizes the dialog again when the layout changes.
• String changes made/needed: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9118691 [details]
Bug 1605724 - Call sizeToContent() again when the icon loads if it hasn't loaded yet. r=dao,ntim

Fix for mis-sized dialog boxes on Linux. Approved for 73.0b3.

Comment 14

[Andreea Pavel [:apavel]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f81ec1f37a25
Ensure loading the dialog icons doesn't change over-all layout. r=dao

Comment 16

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f81ec1f37a25

Comment 17

[Cristian Fogel [:cfogel]]

In an attempt to reproduce the issue for a proper fix verification, we were unable to reproduce the issue on our available setups. (Ubuntu16/Ubuntu 18/Ubuntu19).
With a clean Fedora31 on a VM, I've noticed the window exhibiting the issue for a fraction of a second before jumping to the proper size before resizing to the proper values.

In regards to this, clearing the qe+ flag since the issue appears to not affect vanilla versions of the OS.

@Martin, if your team could provide an image with wayland set up we could load it on a virtual machine.
For this, probably your confirmation if this is fixed on the current builds would suffice.

Comment 18

[Martin Stránský [:stransky] (ni? me)]

(In reply to Cristian Fogel, QA [:cfogel] from comment #17)

With a clean Fedora31 on a VM, I've noticed the window exhibiting the issue for a fraction of a second before jumping to the proper size before resizing to the proper values.

Fedora 31 / Gnome is a good testcase, Wayland is enabled by default there.

Comment 19

[Martin Stránský [:stransky] (ni? me)]

I tested latest nightly and looks fixed to me.