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

Updated

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?

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.

https://hg.mozilla.org/mozilla-central/rev/5ca5387a57ce

Reporter

Comment 9

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 ?

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?

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.

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.

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

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.

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

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