1535253 - passwordManager.xul is lacking padding

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Matthew N. [:MattN]]

Attachment: Master Password: "Password Change Succeeded"

Since bug 1493844, many dialog windows/prompts are missing padding which makes them look really poor and unpolished. I'm not sure of the common theme between them but maybe Jared knows (I think he reviewed the one of two patches [part 2/3] that causes this) or understand what the general fix would be.

• passwordManager.xul (when opened in its own window from the context menu or autocomplete footer)
• changemp.xul - Master password: "Password change succeeded" - Uses Services.prompt.alert => commonDialog.xul
• • Existing problem before bug 1493844
• ReportFalseDeceptiveSite() - Uses Services.prompt.alert
• Set as homepage confirmation - Upon URL drag and drop on the home button - Services.prompt.confirmEx
• etc.

[Tracking Requested - why for this release]: Secondary UI regression affecting the password manager dialog.

Comment 1

[Matthew N. [:MattN]]

Attachment: Set Home Page

Comment 2

[Matthew N. [:MattN]]

Attachment: "This isn't a deceptive site" prompt

Comment 3

[Matthew N. [:MattN]]

Attachment: passwordManager.xul (missing padding on all sides)

Comment 4

[Pascal Chevrel:pascalc]

Thanks for reporting and the initial investigation. I would like to have a general fix for 67 uplifted if we can get a safe patch so tracking for 67. Windows 10 and Linux are not impacted as the title of dialog boxes is part of the title bar, that's probably why it wasn't reported before.

Comment 5

[Neil Deakin]

Alex, do you know what might have caused this from bug 1493844?

Comment 6

[Brian Grinstead [:bgrins]]

I can reproduce the master password issue, but I don't see any obvious reason in https://hg.mozilla.org/mozilla-central/rev/e6a310eb8e42 (commonDialog and dialog.xml are unchanged there). If I increase the margin bottom on .dialog-button-box it does space it out, but would like to know what's going on before sending a specific fix for the common dialog up. My best guess so far is that there's an accidental side effect to some of the CSS changes in that patch (like groupbox.css being removed).

Comment 7

[:Gijs (he/him)]

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

I can reproduce the master password issue, but I don't see any obvious reason in https://hg.mozilla.org/mozilla-central/rev/e6a310eb8e42 (commonDialog and dialog.xml are unchanged there). If I increase the margin bottom on .dialog-button-box it does space it out, but would like to know what's going on before sending a specific fix for the common dialog up. My best guess so far is that there's an accidental side effect to some of the CSS changes in that patch (like groupbox.css being removed).

Looks like the contentPane class has different behavior pre/post patch and is used in passwordmanager.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

I cannot reproduce the issues with Services.prompt.alert or Services.prompt.confirmEx when testing on Nightly 68.0a1 (2019-03-26) OSX 10.13.6.

However I am able to reproduce the issue with the passwordManager.xul dialog and see the issue.

.contentPane rules were changed and moved to dialog.inc.css, but the passwordManager.xul dialog does not include the dialog.css file.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1535253 - Add back the .contentPane rule to passwordmgr.css that was removed accidentally. r?MattN

The .contentPane rule for /browser was consolidated and removed throughout the codebase but the .contentPane rule in passwordmgr.css is in /toolkit and shouldn't have been removed.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

I have also tried to reproduce the small Services.prompt.confirmEx using mozregression on a build from 2019-03-10 (two days before this bug was filed) on OSX 10.13.6 and don't see the same issue that are shown in the screenshot. I also tested a build from 2018-10-23 and couldn't reproduce the small Services.prompt.confirmEx on that one either.

Comment 11

[Matthew N. [:MattN]]

(In reply to :Gijs (he/him) from comment #7)

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

I can reproduce the master password issue, but I don't see any obvious reason in https://hg.mozilla.org/mozilla-central/rev/e6a310eb8e42 (commonDialog and dialog.xml are unchanged there). If I increase the margin bottom on .dialog-button-box it does space it out, but would like to know what's going on before sending a specific fix for the common dialog up. My best guess so far is that there's an accidental side effect to some of the CSS changes in that patch (like groupbox.css being removed).

Looks like the contentPane class has different behavior pre/post patch and is used in passwordmanager.

It seems like Brian was talking about the master password issue (changemp.xul) but you are talking about passwordManager.xul.

(In reply to (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #10)

I have also tried to reproduce the small Services.prompt.confirmEx using mozregression on a build from 2019-03-10 (two days before this bug was filed) on OSX 10.13.6 and don't see the same issue that are shown in the screenshot. I also tested a build from 2018-10-23 and couldn't reproduce the small Services.prompt.confirmEx on that one either.

What STR are you using? Did you try drag from the identity block to the homepage button? I can reproduce that issue in my dev and regular profiles now.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #11)

(In reply to :Gijs (he/him) from comment #7)

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

I can reproduce the master password issue, but I don't see any obvious reason in https://hg.mozilla.org/mozilla-central/rev/e6a310eb8e42 (commonDialog and dialog.xml are unchanged there). If I increase the margin bottom on .dialog-button-box it does space it out, but would like to know what's going on before sending a specific fix for the common dialog up. My best guess so far is that there's an accidental side effect to some of the CSS changes in that patch (like groupbox.css being removed).

Looks like the contentPane class has different behavior pre/post patch and is used in passwordmanager.

It seems like Brian was talking about the master password issue (changemp.xul) but you are talking about passwordManager.xul.

I couldn't reproduce the issue with changemp.xul, only with passwordManager.xul.

(In reply to (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #10)

I have also tried to reproduce the small Services.prompt.confirmEx using mozregression on a build from 2019-03-10 (two days before this bug was filed) on OSX 10.13.6 and don't see the same issue that are shown in the screenshot. I also tested a build from 2018-10-23 and couldn't reproduce the small Services.prompt.confirmEx on that one either.

What STR are you using? Did you try drag from the identity block to the homepage button? I can reproduce that issue in my dev and regular profiles now.

Yes, that is the method I was using to attempt to reproduce this.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Matt, if you can reproduce this could you look in to it? I tried on multiple occasions to reproduce this for changemp.xul and the Change Homepage dialog.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 9050908 [details]
Master Password: "Password Change Succeeded"

I can reproduce this issue with commonDialog.xul before bug 1535253 landed so I will file it separately.

Comment 15

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #0)

• ReportFalseDeceptiveSite() - Uses Services.prompt.alert
• Set as homepage confirmation - Upon URL drag and drop on the home button - Services.prompt.confirmEx

I can no longer reproduce these even though I could still reproduce the homepage issue on Thursday or Friday… I guess we can ignore this for now until it starts happening again… I did upgrade to macOS 10.13.6 on the weekend from 10.13.5(?) but maybe it has to do with multiple-monitor support or something else more obscure.

Let's just do the passwordManager.xul fix for now and I'll file the "Password Change Succeeded" one as a separate bug.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53861bf00f84
Add back the .contentPane rule to passwordmgr.css that was removed accidentally. r=MattN

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/53861bf00f84

Comment 18

[Pascal Chevrel:pascalc]

Matthew, could you request an uplift to beta please? Thanks

Comment 19

[Daniel Bodea [:danibodea]]

I can confirm that the padding was added to the windows in question in Nightly v68.0a1.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 9053649 [details]
Bug 1535253 - Add back the .contentPane rule to passwordmgr.css that was removed accidentally. r?MattN

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1493844
• User impact if declined: Password Manager when opened as a dialog will not have enough padding
• 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: Right-click on a login field and choose "Fill Login" -> "View Saved Logins"
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Adding back CSS that was accidentally removed
• String changes made/needed: none

Comment 21

[Pascal Chevrel:pascalc]

Comment on attachment 9053649 [details]
Bug 1535253 - Add back the .contentPane rule to passwordmgr.css that was removed accidentally. r?MattN

Low risk CSS patch fixing a visual issue on the Password Manager dialog, uplift approved for 67 beta 8, thanks.

Comment 22

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/65b5cf758229

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

If you think this warrants uplift for 66.0.3 please let me know.

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 9053649 [details]
Bug 1535253 - Add back the .contentPane rule to passwordmgr.css that was removed accidentally. r?MattN

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #23)

If you think this warrants uplift for 66.0.3 please let me know.

If you're willing to take it then it would be nice to have since it's user-facing and a trivial fix.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1493844
• User impact if declined: Password Manager when opened as a dialog will not have enough padding. Not a huge deal if you don't take it.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Adding back CSS that was accidentally removed
• String changes made/needed: None

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9053649 [details]
Bug 1535253 - Add back the .contentPane rule to passwordmgr.css that was removed accidentally. r?MattN

I'd prefer to keep the release uplifts to what we think is crucial.

Comment 26

[Daniel Bodea [:danibodea]]

I have verified this fix in Firefox Beta v67.0b8, also.
Is this bug going to be uplifted to 66.0.3?
Thanks.