1844935 - Replace restart prompt when changing history settings with an in-content dialogue box

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P5)

Description

[bugcatcher66]

Attachment: "Restart Firefox" pop-up does not match the dark theme of Firefox

[Affected Versions]

• Firefox 115.0.2 (64-bit)

[Affected Platforms]

• Windows 11 x64

[Preconditions]:

• Dark theme is set on Firefox by enabling the 'Dark' ratio under 'Website appearance'.

[Steps to reproduce]:

1. Go to "about:preferences#privacy".
2. Select "Never Remember history" from the "History" section.

[Expected result]:

• "Restart Firefox" pop-up should be dark theme.

[Actual result]:

• "Restart Firefox" pop-up is not dark theme.

[Note]:

• If clicking the "Clear History…" button, "Clear Recent History" pop-up is under dark theme.

Comment 1

[Cieara Meador [:cmkm] (she/her)]

We should update this prompt to use an in-content dialogue modal, rather than an OS dialogue box. The work done for bug 1685346 looks similar to what needs to be done here: passing a browsing context, and updating how we listen for the user's response to the prompt in an async manner. (Thanks Gijs!)

Comment 2

[Steve P]

Hi, I'm a new contributor and will be submitting a patch for this bug asap.

Comment 3

[Steve P]

Hello,

After looking through, the bug seems to be that when clicking to never remember history in the privacy menu, the pop up you get is an OS themed pop up, and doesn't look like all the other pop up themed by the browser. I've found in the code the functions and html where a variable is set based on the option you choose, and then using that variable to apply the settings. I'm struggling with finding where exactly the code is calling the pop up to execute so that I can attempt to change what happens there. Do you have any thoughts on where I might go from here? Thanks!

Comment 4

[Cieara Meador [:cmkm] (she/her)]

Hi there, thanks for working on this! :) To answer your question, this prompt is invoked here in privacy.js on line 2241. To make this change, you'll need to update the confirmRestartPrompt function where it's defined in preferences.js, on line 518.

Similar to the fix for Bug 1685346, we need to use Services.prompt.asyncConfirmEx here instead of the current Services.prompt.confirmEx. Note that the new function is async.

There are also some differences in the parameters accepted by the new function. The documentation for this function is here. You'll need to pass the browsing context (window.browsingContext) instead of just the window reference. You'll then need to insert a new second argument for the type of prompt to show (you want Ci.nsIPrompt.MODAL_TYPE_CONTENT).

Finally, instead of getting the index value directly, you'll now need to get the result of the prompt input using buttonIndex.get("buttonNumClicked"). You'll need to add a new variable to store the result value, and return that variable instead of buttonIndex.

I'm happy to give further help if needed, please don't hesitate to follow up if you get stuck! Best of luck!

Comment 5

[Steve P]

Attachment: Bug 1844935 - Replace restart prompt when changing history settings with an in-content dialogue box. r?cmkm

Comment 6

[Steve P]

Hello,

Here is a WIP patch for your review: https://phabricator.services.mozilla.com/D207202

You mentioned to get the result and save it a new variable to return, however I noticed the code below using the buttonIndex variable, and rather than making changes to all that as well, I went ahead and renamed the variable originally declared as buttonIndex to button, and named my new variable buttonIndex since that's what it is storing what is used below. Please let me know if you see anything I missed, and if there are any tests I should be running. Thanks!

Comment 7

[Steve P]

Sorry, I corrected the patch from a WIP to submitting for review and tagging you. Let me know if there's anything else, or tests, thank you!

Comment 8

[Cieara Meador [:cmkm] (she/her)]

Thank you for your patience! I've approved the patch and I'll land it as soon as I'm able to. Thanks so much for contributing to Firefox! :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f7d19b2b800
Replace restart prompt when changing history settings with an in-content dialogue box. r=cmkm,settings-reviewers,Gijs

Comment 10

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/3f7d19b2b800