1706279 - Tighten up upgrade dialog spacing to be more usable before showing scrollbar

Status: VERIFIED FIXED

(Firefox :: Messaging System, defect, P2)

Description

[Catalin Sasca, Desktop QA [:csasca]]

Attachment: proton new message after upgrade low res.gif

Affected versions

• Firefox 89.0b1
• Firefox 90.0a1

Affected platforms

• Windows 10
• Ubuntu 20.04
• macOS 10.15.7

Steps to reproduce

1. Have an older profile of Firefox (ex. 88.0b) available
2. Launch Firefox 89.0b with the older profile
3. An onboarding message is shown

Expected result

• The message is fully visible or scrolable on lower resolutions

Actual result

• The message is not fully visible or scrolable on lower resolutions

Regression range

• Will see for a regression if there is one.

Additional notes

• The issue can be seen in the attachment
• This issue was reproduced on a 1366x768 laptop display
• The issue can be reproduced on higher res displays, if the browser's window is shrinked

Comment 1

[Ed Lee :Mardak]

Attachment: dialog scrollbar height

Gijs, the dialog content scrollbar seems to appear only after shrinking the browser window a little bit past the bottom edge of the dialog -- roughly an extra amount matching the height of the toolbars.

I took a screenshot of the window at a height that just starts showing the scrollbar that doesn't allow scrolling down to the bottom of the dialog content. I pasted in the missing content and a copy of the toolbar area of similar height.

Maybe the max height of the dialog needs to be reduced by the toolbar height?

You can show the dialog with Cc["@mozilla.org/browser/browserglue;1"].getService().wrappedJSObject._showUpgradeDialog()

Comment 2

[:Gijs (he/him)]

(In reply to Ed Lee :Mardak from comment #1)

Maybe the max height of the dialog needs to be reduced by the toolbar height?

I don't think any max-height is set right now, the assumption was that dialog content would be small. :-)

We set a min-height from https://searchfox.org/mozilla-central/rev/d280cc26237b62096b89317e4ed6dea8b2bdf822/browser/base/content/browser.css#1610-1611,1615 , which is set from https://searchfox.org/mozilla-central/rev/d280cc26237b62096b89317e4ed6dea8b2bdf822/toolkit/modules/SubDialog.jsm#534-537 .

This is probably somewhat related to bug 1699430, where I was thinking we'd want to start moving the dialog up (ie start putting it further on top of browser chrome, if there is no room for it to go below chrome).

Note that the print dialog uses sizeto: available which has different logic, and it copes with this kind of problem. We may want to switch to that for the onboarding dialog? That'd take some more research (and adding extra args to gDialogBox.open, I guess). The print dialog has other requirements as well though (regarding aspect ratio and leaving room for the print preview and such) so I don't know whether that is a good fit for the onboarding dialog. Mark Striemer would be a good person to talk to if the CSS around the earlier link is inscrutable. :-)

Does that help?

Comment 3

[Giorgia Nichita, Release Desktop QA]

Attachment: screenshot.PNG

We tried to reproduce this issue on the latest Nightly 90.0a1, but we couldn't reproduce it on Windows 10x64 and Mac 11.2.3.
We looked for a regression manually but the bug does not reproduce on Nightly builds.
I mention that we can reproduce the steps on Beta and Devedition builds.

NOTE:
The vertical scrollbar length exceeds the height of the dialog content. Please see attached screenshot. Should this be considered a separate issue and logged accordingly?

Comment 4

[:Gijs (he/him)]

(In reply to Giorgia Nichita, Release Desktop QA from comment #3)

The vertical scrollbar length exceeds the height of the dialog content. Please see attached screenshot. Should this be considered a separate issue and logged accordingly?

Ed is a better person to answer this than I am, I suspect.

Comment 5

[Ed Lee :Mardak]

(In reply to Giorgia Nichita, Release Desktop QA from comment #3)

The vertical scrollbar length exceeds the height of the dialog content.

Looks like this is caused by the <dialog> overflowing due to the height set on the <browser>. I suppose if the fix here sizes things appropriately, the <browser> will have the scrollbar instead of the <dialog>.

Comment 6

[Ed Lee :Mardak]

(In reply to Catalin Sasca, QA [:csasca] from comment #0)

Created attachment 9217001 [details]
proton new message after upgrade low res.gif

Looks like this particular situation has shorter space because there's the bookmark toolbar and an infobar showing pushing down where the modal starts. This can also happen for the bookmark dialogs from bug 1693139 although it's not as tall as the upgrade dialog bug 1697222.

Comment 7

[Ed Lee :Mardak]

Attachment: bookmark dialog with strange scrollbar

Comment 8

[Ana Medinac]

Marking this as a P2a

Comment 9

[Ed Lee :Mardak]

Attachment: 88/pre-MR1 edit bookmark with short window

There are now figma specs that indicate short windows have modals positioned higher but never overlapping tabs or urlbar toolbars (so bookmark toolbar and infobars can be covered). This is also while allowing the bottom to extend out below the window.

This behavior seems to be pretty close to the pre-MR1 behavior where the panel(?) is not contained by the window although top anchor point is different. In 88, it's somewhat centrally aligned and goes no higher than the bookmark toolbar as the window shrinks.

Is it difficult to switch to the old behavior? If not "like a panel" for MR1, emanuela did suggest "do the “max overlaps” and then scrollbar for right now." Although "max overlaps" doesn't specify a minimum bottom spacing as the desired behavior is to have it extend, so maybe we just need to pick something reasonable, e.g., match the top edge or maybe just 1em or a fixed px.

Comment 10

[Ed Lee :Mardak]

Looks like bug 1699430 will focus on making the most of the available window height to show the dialog. If the content still doesn't fit that available space, we'll end up showing the correct (<browser>) scrollbar. This bug will then switch upgrade dialog to a shorter design for height < 540px that take up less vertical space to begin with for short screens to avoid needing to show the scrollbars.

Comment 11

[Ed Lee :Mardak]

Attachment: example short dialog still anchored to bottom of chrome

Comment 12

[Ed Lee :Mardak]

Attachment: example functional scrollbar going to the bottom

Comment 13

[Ed Lee :Mardak]

Attachment: Bug 1706279 - Tighten up upgrade dialog spacing on short windows to be more usable before showing scrollbar r?pdahiya

Detect if there's not enough vertical space to switch to compact mode. Use selectors and variables to adjust font-sizes, margins, etc.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23cbc71e3df1
Tighten up upgrade dialog spacing on short windows to be more usable before showing scrollbar r=pdahiya

Comment 15

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/23cbc71e3df1

Comment 16

[Robert Martin [:romartin], Ecosystem QA]

I have verified that a vertical and a horizontal scrollbar are displayed and are working correctly if a small OS resolution or a small browser window is set before the upgrading users modal is displayed.

The verification was done using Firefox Nightly 90.0a1 (Build ID: 20210509213623) on Windows 10 x64, macOS 11.3.1, and Ubuntu Linux 20.04 x64.

However, I've observed that if the upgrading users modal is displayed and the resize is done afterward, only a vertical scrollbar is displayed which does not adjust itself dynamically. I've logged the following bug for this specific behavior: bug 1710434.

Comment 17

[Ed Lee :Mardak]

Shilpa, do we want to uplift this fix for MR1? It's closely related to the fix from bug 1699430 comment 14 but neither are directly dependent on each other. The other bug makes it so dialogs can be positioned higher and correctly sized to show scrollbars while this bug makes the upgrade dialog shorter when a short window is detected, e.g., attachment 9220438 [details], instead of always showing the full-height content, e.g., attachment 9217001 [details].

Comment 18

[Shilpa Mohanty]

Sure. Let's uplift this one.

Comment 19

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

Ed, will you request an uplift to beta? Thanks

Comment 20

[Ed Lee :Mardak]

Comment on attachment 9220488 [details]
Bug 1706279 - Tighten up upgrade dialog spacing on short windows to be more usable before showing scrollbar r?pdahiya

Beta/Release Uplift Approval Request

• User impact if declined: Those with low screen resolution or short windows cannot see bottom part of MR1 upgrade dialog as it's too tall
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Size the window to be slightly shorter around 650px tall

2. Trigger upgrade dialog, e.g., set browser.startup.homepage_override.mstone to 88 clear browser.startup.upgradeDialog.version or run Cc['@mozilla.org/browser/browserglue;1'].getService().wrappedJSObject._showUpgradeDialog()

• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): A little bit of JS to detect window height and CSS to style the dialog with less whitetspace as approved by UX.
• String changes made/needed: none

Comment 21

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

Comment on attachment 9220488 [details]
Bug 1706279 - Tighten up upgrade dialog spacing on short windows to be more usable before showing scrollbar r?pdahiya

Has trsts and looks low risk for a visible bug, approved for 89 beta 12, thanks.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Robert Martin [:romartin], Ecosystem QA]

I have verified that the issue is no longer reproducible by following the steps provided in the description and using Firefox Beta 89.0b12 (Build ID: 20210513185752) on Windows 10 x64, macOS 11.3.1, and Ubuntu Linux 20.04 x64.