passwordManager.xul is lacking padding

RESOLVED FIXED in Firefox 67

Status

()

defect
P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: MattN, Assigned: jaws)

Tracking

(Regression, {regression})

65 Branch
mozilla68
Unspecified
macOS
Points:
---
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox65 wontfix, firefox66+ wontfix, firefox67+ verified, firefox68 verified)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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
  • 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.

Flags: needinfo?(jaws)

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.

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

Flags: needinfo?(surkov.alexander)
Priority: -- → P1

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).

(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.

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.

Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(jaws)

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.

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.

(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.

(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.

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.

Flags: needinfo?(MattN+bmo)
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.
Attachment #9050908 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo)

(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.

Summary: Many dialog windows are lacking padding → passwordManager.xul is lacking padding
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53861bf00f84
Add back the .contentPane rule to passwordmgr.css that was removed accidentally. r=MattN
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

Flags: qe-verify+
Flags: needinfo?(MattN+bmo)

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

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
Attachment #9053649 - Flags: approval-mozilla-beta?
Flags: qe-verify+ → qe-verify?
Flags: needinfo?(MattN+bmo)
Flags: qe-verify? → qe-verify+

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.

Attachment #9053649 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
No longer blocks: 1493844
Regressed by: 1493844

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

Flags: needinfo?(jaws)

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
Flags: needinfo?(jaws)
Attachment #9053649 - Flags: approval-mozilla-release?

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.

Attachment #9053649 - Flags: approval-mozilla-release? → approval-mozilla-release-
QA Whiteboard: [qa-triaged]

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

Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(enndeakin)
Component: XUL → Password Manager
Flags: needinfo?(enndeakin)
Product: Core → Toolkit
You need to log in before you can comment on or make changes to this bug.