1712750 - Clear Recent History modal has the Old UI

Status: VERIFIED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Rares Doghi, Desktop QA]

Attachment: 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.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1712750 - Open sanitize dialog using the window modal dialog box when possible. r=mtigley!

Comment 3

[Mike Conley (: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?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc5c5ba21183
Open sanitize dialog using the window modal dialog box when possible. r=mtigley

Comment 6

[Alexandru Michis [:malexandru]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ca5387a57ce
Open sanitize dialog using the window modal dialog box when possible. r=mtigley

Comment 8

[Marian-Vasile Laza]

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

Comment 9

[Rares Doghi, Desktop QA]

Attachment: 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 ?

Comment 10

[Mehmet]

Attachment: Bildschirmfoto 2021-06-24 um 13.56.17.png

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.

Comment 11

[:Gijs (he/him)]

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

Comment 12

[Mehmet]

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

Comment 13

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 14

[Dorel Luca [:dluca]]

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

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

Comment 15

[Iulian Moraru]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/50a4bb499585827be4ff37b67b0d6d7c07104b2b

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1712750 - Get rid of some old-modal condition handlers in sanitize.xhtml. 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

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1712750 - Make sure sanitize.xhtml gets to set its mozSubdialogReady. r?mtigley!

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

Comment 18

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/93dae0d7f460
https://hg.mozilla.org/mozilla-central/rev/924d3431bdc7
https://hg.mozilla.org/mozilla-central/rev/73870972fca7

Comment 22

[Rares Doghi, Desktop QA]

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

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

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

Comment 25

[Mehmet]

(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 :)

Comment 26

[Rares Doghi, Desktop QA]

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.