Closed Bug 1049001 Opened 6 years ago Closed 4 years ago

Convert the certManager dialog to in-content

Categories

(Firefox :: Preferences, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: Paenglab, Assigned: jyeh)

References

(Regressed 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1036815 moves for the Advanced pane almost all dialogs to in-content. But the certManager gets clipped on the right for OS X. To not block the other bug this dialog will be made in-content here.
Attached patch certManager.patch (obsolete) — Splinter Review
This patch is to apply on top of bug 752197.

I managed to fix the issue by setting the certmanager width to 51em. This change doesn't influence the width of the normal certmanager dialog. It has always the same width. I tested this change on Linux, OS X and Win7.

If desired I could also apply this width to OS X only.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8470435 - Flags: review?(MattN+bmo)
Comment on attachment 8470435 [details] [diff] [review]
certManager.patch

Review of attachment 8470435 [details] [diff] [review]:
-----------------------------------------------------------------

I guess may be fine but it may break for other locales so may need a better solution in the future if that's the case.

(In reply to Richard Marti (:Paenglab) from comment #1)
> Created attachment 8470435 [details] [diff] [review]
> certManager.patch
> 
> This patch is to apply on top of bug 752197.

I think you meant a different bug number.
Attachment #8470435 - Flags: review?(MattN+bmo) → review+
Yep, bug 1036815 was meant. This bug can be checked-in together with bug 1036815 when this is ready.
Updated to tip.

I also exchanged two tabs with whitespaces.
Attachment #8470435 - Attachment is obsolete: true
Attachment #8473549 - Flags: review+
Assignee: richard.marti → nobody
Status: ASSIGNED → NEW
Can we check this patch in? It's been a while since the patch had been reviewed, but it still works fine now.
Flags: needinfo?(MattN+bmo)
Maybe this didn't land because of my concern:

(Quoting Matthew N. [:MattN] from comment #2)
> Comment on attachment 8470435 [details] [diff] [review]
> certManager.patch
> 
> Review of attachment 8470435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess may be fine but it may break for other locales so may need a better
> solution in the future if that's the case.



> (In reply to Richard Marti (:Paenglab) from comment #1)
> > Created attachment 8470435 [details] [diff] [review]
> > certManager.patch
> > 
> > This patch is to apply on top of bug bug 752197.
> 
> I think you meant a different bug number.
(In reply to Richard Marti (:Paenglab) from comment #3)
> Yep, bug 1036815 was meant. This bug can be checked-in together with bug
> 1036815 when this is ready.

And maybe it has a dependency on bug 1036815 still?
Flags: needinfo?(MattN+bmo) → needinfo?(richard.marti)
(In reply to Matthew N. [:MattN] from comment #6)
> Maybe this didn't land because of my concern:
> 
> (Quoting Matthew N. [:MattN] from comment #2)
> 
> And maybe it has a dependency on bug 1036815 still?

It's long ago. But I think it's also depending on bug 1036815 because of the failing tests. But this needs testing again.
Flags: needinfo?(richard.marti)
Since bug 1036815 has landed, can we check this patch in? I've checked it on my machine, didn't see any tests broke.
Flags: needinfo?(MattN+bmo)
Duplicate of this bug: 1190163
Hi Joseph, it would be great if you could add a screenshot test to https://dxr.mozilla.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Preferences.jsm like we have for panePrivacy-DNTDialog and then do a try push so we can check that it looks decent on the OSs we test in automation.

See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots
Flags: needinfo?(MattN+bmo)
As bug 1298872 described, cert manager also encounter the same problem in Polish. So I think this patch needs some update as well.
See Also: → 1298872
Assignee: nobody → jyeh
Comment on attachment 8790597 [details]
Bug 1049001 - Convert the certManager dialog to in-content;

https://reviewboard.mozilla.org/r/78342/#review77116

Thanks. I cleaned up Preferences.jsm so will land a rebased version of this when the tree re-opens.
Attachment #8790597 - Flags: review?(MattN+bmo) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/fx-team/rev/bf23ff06c7cd
Refactor mozscreenshots Preferences configuration to fix a typo and reduce duplication. r=me
https://hg.mozilla.org/integration/fx-team/rev/46f83778b4bb
Convert the certManager dialog to in-content; r=MattN
https://hg.mozilla.org/mozilla-central/rev/bf23ff06c7cd
https://hg.mozilla.org/mozilla-central/rev/46f83778b4bb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Blocks: 1204320
Regressions: 1596215
You need to log in before you can comment on or make changes to this bug.