Closed Bug 1712750 Opened 1 year ago Closed 1 year ago

Clear Recent History modal has the Old UI

Categories

(Toolkit :: Notifications and Alerts, defect)

Desktop
Unspecified
defect
Points:
2

Tracking

()

VERIFIED FIXED
92 Branch
Tracking Status
firefox88 --- disabled
firefox89 --- wontfix
firefox90 --- wontfix
firefox91 --- wontfix
firefox92 --- verified

People

(Reporter: rares.doghi, Assigned: mconley)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [proton-cleanups])

Attachments

(6 files)

Attached image dropdown.png

[Affected platforms]:
Platforms: Windows, Mac, Ubuntu

[Steps to reproduce]

  1. Launch the Firefox browser.
  2. Hit Ctrl+Shift+Delete on the Keyboard

[Expected result]
The Clear Recent History Modal should display the new Proton UI (similar to the Clear Recent History modal from about:preferences)

[Actual result]
The Clear Recent History Modal has the old UI.

Has Regression Range: --- → no
Has STR: --- → yes
Duplicate of this bug: 1714078
Points: --- → 2
Assignee: nobody → mconley

So the WIP patch I just posted does the trick of loading the dialog as a window modal dialog, and styles it appropriately. The menulist still looks a little wonky, but I can probably fix that before I request review.

I have found browser_sanitizeDialog.js, and I wanted to get your input johannh - do you think it's worth testing the behaviour of both the gDialogBox version of the modal as well as the separate top-level window version of the dialog? Or do you think it's probably sufficient to test the window modal case? I can do both - it'll just require me to do a little bit of re-jigging the test to run each test in either mode, and updating WindowHelper to handle both cases.

Do you have a preference?

Flags: needinfo?(jhofmann)
Status: NEW → ASSIGNED

I did some digging around, and it looks like there aren't any cases where the dialog would be opened in the old separate top-level window mode after this patch. So I'm going to update the test just to test the window modal case.

Flags: needinfo?(jhofmann)
Attachment #9226607 - Attachment description: WIP: Bug 1712750 - Open sanitize dialog using the window modal dialog box when possible. r?mtigley! → Bug 1712750 - Open sanitize dialog using the window modal dialog box when possible. r?mtigley!
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dc5c5ba21183
Open sanitize dialog using the window modal dialog box when possible. r=mtigley

Backed out changeset dc5c5ba21183 (Bug 1712750) for causing bc failures in browser_interventions.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/d54ed20cd35a25486a98224652c1ec2ab9046d0d
Push with failures, failure log.

Flags: needinfo?(mconley)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ca5387a57ce
Open sanitize dialog using the window modal dialog box when possible. r=mtigley
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
Attached image extraSpaces.png

Hi Gijs, can you please take a look at this issue? It seems that its fixed but there seem to be a little bit too much space on the Modal window, also it seems that the dropdown items might be missing some space ? Im not sure its suppose to look like if you could give us a mock up or an image of how its suppose to look like, or does this modal look ok ?

Flags: needinfo?(gijskruitbosch+bugs)

Hi I am on MacOS. The spacing of the buttons at the buttom are cut off when enable/disable one of the options. A screenshot is attached.

Mike, do you have time to follow-up here for the issues QA and Mehmet noted?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mconley)

Hi, in addition to my comment 10, I reported bug 1718180, bug 1718435 and bug 1718432. Thanks for checking in advance.

Yeah, looks like there are some edge-cases here - especially in the long-string case. I'm going to request a backout of the patch so I can look into this.

Flags: needinfo?(mconley)

Backed out changeset 5ca5387a57ce (bug 1712750) on Dev's request. CLOSED TREE

Backout:
https://hg.mozilla.org/integration/autoland/rev/50a4bb499585827be4ff37b67b0d6d7c07104b2b

Flags: needinfo?(mconley)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 91 Branch → ---
Attachment #9226607 - Attachment description: Bug 1712750 - Open sanitize dialog using the window modal dialog box when possible. r?mtigley! → Bug 1712750 - Open sanitize dialog using the window modal dialog box when possible. r=mtigley!

These handlers and markup were only ever relevant when opening the
dialog in an old-style modal. Now that we're opening the dialog as
a SubDialog, these conditions can be cleaned up.

Depends on D117567

The mozSubdialogReady was being set inside of sanitize.xhtml in
its load event handler, which would be scheduled to run AFTER the
SubDialog _onLoad handler (which is what awaits mozSubdialogReady).

The only reason this wasn't more obvious is because the first time
the dialog is opened, the SubDialog _onLoad handler awaits
translation of the document, which gives sanitize.xhtml a chance
to run its load event handler and set the mozSubdialogReady.
Subsequent opens of the dialog wouldn't need to re-run translation
due to document caching, and so the mozSubdialogReady wouldn't
be waited for, resulting in incorrect dialog layout.

Depends on D119329

Regarding the style of the menulist dropdown, I believe we're sharing the same style of menulist dropdown as in about:preferences and other pages that use common.css, just at a smaller font size. To make them match would be a matter of increasing the font size of our modals which is probably a separate matter entirely.

Flags: needinfo?(mconley)

Thanks for the reviews, mtigley! I'll land these after soft freeze ends.

Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93dae0d7f460
Open sanitize dialog using the window modal dialog box when possible. r=mtigley
https://hg.mozilla.org/integration/autoland/rev/924d3431bdc7
Get rid of some old-modal condition handlers in sanitize.xhtml. r=mtigley
https://hg.mozilla.org/integration/autoland/rev/73870972fca7
Make sure sanitize.xhtml gets to set its mozSubdialogReady. r=mtigley
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch

This issue is Verified as fixed in our latest Nightly build 92.0a1 (2021-07-13) on Windows, Mac and Ubuntu.

The patch landed in nightly and beta is affected.
:mconley, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(mconley)

I suggest letting this bake on Nightly to make sure there's no more weirdness with it.

Flags: needinfo?(mconley)

(In reply to Mike Conley (:mconley) (:⚙️) from comment #24)

I suggest letting this bake on Nightly to make sure there's no more weirdness with it.

Maybe you can take a look at bug 1720262? That would be a polishing issue in this new modal. Many thanks :)

Updating the Status flag for this issue to Verified since it will not be uplifted in Beta, I will take a look at Bug 1720262 when that one gets fixed as well.

Status: RESOLVED → VERIFIED
Blocks: 1721466
Blocks: 1721474
Blocks: 1718435
Blocks: 1718432
Blocks: 1720262
Blocks: 1723679
See Also: → 1725890
Depends on: 1725890
See Also: 1725890
No longer depends on: 1725890
You need to log in before you can comment on or make changes to this bug.