1708148 - When being asked which account to update, the bottom options are cut off

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P2)

Description

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Attachment: image_2021_04_26T15_05_05_920Z.png

Affected platforms:
Platforms: All

Preconditions
Set the following prefs in about:config

• browser.proton.enabled = true
• prompts.windowPromptSubDialog = true
• prompts.contentPromptSubDialog = true
• browser.proton.modals.enabled = true

Steps to reproduce
Preconditions: Have 2 remembered ff accounts that you've synced in the past

1. Launch the Firefox browser
2. Click hamburguer menu
3. Select sign in option in the sync and save data option from menu.
4. After signing in, click hamburguer menu and select manage account.
5. Attempt to change password

Expected result
There are no viusal effects while interacting with the ff browser.

Actual result
The bottom options when being asked which account to update are cut off

Comment 1

[:Gijs (he/him)]

Minimal simple steps to repro for engineering/product: run

Services.prompt.select(
      window,
      "Hello",
      "Select which to update",
      ["a", "b"],
      {value: null}
    );

from the browser console.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1708148 - switch to DOMContentLoaded in selectdialog.js for adding dialog content, to ensure it is present when we determine its height, r?jaws

Depends on D113723

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b0fe20bf807
switch to DOMContentLoaded in selectdialog.js for adding dialog content, to ensure it is present when we determine its height, r=jaws

Comment 4

[Cristian Brindusan [:cbrindusan]]

Backed out 3 changesets (Bug 1704113, Bug 1705330, Bug 1708148) for causing bc failures in browser_username_select_dialog.js and mochitest failures in test_modal_select.html.
https://hg.mozilla.org/integration/autoland/rev/3d2225b699c12f33140428b4be4bf8362c370df7

Comment 5

[:Gijs (he/him)]

(In reply to Cristian Brindusan [:cbrindusan] from bug 1705330 comment #4)

Backed out 3 changesets (Bug 1704113, Bug 1705330, Bug 1708148) for causing bc failures in browser_username_select_dialog.js and mochitest failures in test_modal_select.html.
https://hg.mozilla.org/integration/autoland/rev/3d2225b699c12f33140428b4be4bf8362c370df7

bc failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=30873124f2694ad0d98541fd68ca97db73b74856&selectedTaskRun=IAEk7U_iRwKWmtzhkjep1Q.0
Failure log:
https://treeherder.mozilla.org/logviewer?job_id=338103236&repo=autoland&lineNumber=3352

mochitest failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=30873124f2694ad0d98541fd68ca97db73b74856&selectedTaskRun=dmrgwgWdRCaH5ICCrOoWnA.0
Failure log:
https://treeherder.mozilla.org/logviewer?job_id=338103247&repo=autoland&lineNumber=2239

Seems the tests don't like this change. Investigating.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

It'd be lovely if bug 1706909 was also this. I'm not familiar with how we do that pairing dialog, but it seems possible - I'll try and remember to look

Comment 7

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] [:mhammond] from comment #6)

It'd be lovely if bug 1706909 was also this. I'm not familiar with how we do that pairing dialog, but it seems possible - I'll try and remember to look

I had been hoping this and the other 2 bugs from my previous comment were all similar, but they all required different fixes. Looking at that bug, I believe that too will be something else. If it reproduces only on smaller screens, it sounds like there's just not enough space to fit things. To fix that the dialog would have to be made scrollable, which would be something to fix in the markup/style for that dialog. The changes in bug 1705330 will help a tiny bit as the total vertical padding framing the dialog contents will be reduced by 8px compared to the status quo, but obviously that would still be a problem with a small browser window.

Comment 8

[:Gijs (he/him)]

OK, try is happy so I'm relanding this.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab6035111c40
switch to DOMContentLoaded in selectdialog.js for adding dialog content, to ensure it is present when we determine its height, r=jaws

Comment 10

[Narcis Beleuzu [:NarcisB]]

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

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Hey gijs - do you have a sense of the risk requesting uplift on this one to 89? Or should this ride in 90?

Comment 12

[:Gijs (he/him)]

I think this is probably upliftable, yeah - the test coverage caught my initial mistake, at least! And if we push buttons off-screen for users that's no good. So let's uplift. :rnons, :mkmelin: sorry for not realizing this was used in TB and giving you a heads-up beforehand, but just a heads-up that uplift will mean you'll want to uplift the patch from bug 1708618, too.

Comment 13

[:Gijs (he/him)]

Comment on attachment 9219070 [details]
Bug 1708148 - switch to DOMContentLoaded in selectdialog.js for adding dialog content, to ensure it is present when we determine its height, r?jaws

Beta/Release Uplift Approval Request

• User impact if declined: Required for MR1 / Proton
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: see comment 0 (comment 1 is useful to check visual style, but won't let you check whether the "pick a user" flow actually works correctly, and so it's probably best to use the steps in comment 0 for verification)
• List of other uplifts needed: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): There are automated tests and this is a fairly straightforward JS change, plus we still have 4 weeks of beta runway
• String changes made/needed: Nope

Comment 14

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

Comment on attachment 9219070 [details]
Bug 1708148 - switch to DOMContentLoaded in selectdialog.js for adding dialog content, to ensure it is present when we determine its height, r?jaws

We still have many betas and the patch doesn't look risky, approved for 89 beta 8, thanks.

Comment 15

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

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

Comment 16

[Magnus Melin [:mkmelin]]

Thanks for the note.

Comment 17

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Attachment: buttons.JPG

Verified in latest Nightly 90.0a1 (2021-05-05) (64-bit) and Beta 89.0b8 (64-bit).
This issue is no longer reproducible, buttons are now correctly displayed. Updating flags accordingly.
Best regards,
Clara.