Closed Bug 1702059 Opened 3 years ago Closed 3 years ago

Modals from Addon Manager preferences pages no longer work

Categories

(Toolkit :: Add-ons Manager, defect, P2)

defect

Tracking

()

VERIFIED FIXED
89 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox87 --- disabled
firefox88 --- disabled
firefox89 --- verified

People

(Reporter: mconley, Assigned: rpl)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [proton-modals][priority:2a])

Attachments

(3 files, 1 obsolete file)

STR:

  1. In Nightly, go to about:debugging, choose "This Nightly", and then "Load Temporary Add-on" the add-on I will soon attach to this bug
  2. Go to about:addons, and find the add-on in the Addon Manager
  3. Click on the "..." for the add-on, and choose "Preferences"
  4. Click on the button in the add-on preferences that says "CLICK ME"

ER:

An alert dialog should appear.

AR:

No alert appears. Presumably no other dialog types work either. This is a regression from setting prompts.contentPromptSubDialog to true (bug 1680637).

(I discovered this while working on bug 1686315).

See Also: → 1686315
Attached file Demo WebExtension
Priority: -- → P2
Whiteboard: [proton-modals] → [proton-modals][priority:2a]

This is likely the low risky option:

  • in this patch: identify the modal prompts triggered by extensions options page embedded in about:addons and use the openContenPrompt
    for them

  • in a separate follow up issue (to be files), once we are ready to rip off the content prompt code
    (and the related about:config pref):

    • stop using openContentPrompt
    • remove the stack XUL element currently used in about:addons to host the content prompts
    • adapt the test case to the new prompts (e.g. using PromptTestUtils to wait for the modal prompt),
Assignee: nobody → lgreco
Status: NEW → ASSIGNED

This is another possible option:

  • in this patch:

    • identify the modal prompts triggered by extensions options page embedded in about:addons
      and use the TabDialogBox hooked on the entire about:addons tab
      (it is slightly different from the current behavior, where we do open the modal only for
      the options page subframe, but given that only one options page is opened at any time
      it may be ok, and the dialog would still explicitly mention the extension name in the
      prompt title)
    • adapt the browser_ext_optionsPage_modals test to be able to run with the old and new
      modal prompts (so that we are sure that the test can still run and pass with both)
  • in a separate follow up issue (to be files), once we are ready to rip off the content prompt code
    (and the related about:config pref):

    • remove the stack XUL element currently used in about:addons to host the content prompts
    • cut out from browser_ext_optionsPage_modals the test task for the content prompts
Attachment #9213058 - Attachment is obsolete: true
Attachment #9213057 - Attachment description: Bug 1702059 - (option 1) Use PromptParent.openContentPrompt for about:addons options page modals. r?mconley!,mixedpuppy! → Bug 1702059 - Use PromptParent.openContentPrompt for about:addons options page modals. r?mconley!,mixedpuppy!
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/dfa41bd4757e
Use PromptParent.openContentPrompt for about:addons options page modals. r=mconley,mixedpuppy

Backed out changeset dfa41bd4757e (Bug 1702059) for causing bc failures in browser_library_warnOnOpen.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/9e717f873bad77ac555ae7497a76b290ca40ad19
Push with failures, failure log.
(Update): Also caused bc failures in browser_UITour_modalDialog.js

Flags: needinfo?(lgreco)

My bad, I should have predicted that. I'm updating the fix to take into account that there may not be an embedderWindowGlobal and pushing to try to confirm that is the only additional case to be handled.

Flags: needinfo?(lgreco)

Patch updated on phabricator (and checked locally that browser_library_warnOnOpen.js pass as expected on the updated version)

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f5a9a97bfee82791f422b3d2b9de40bf34f03eb

Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/75be595a19ae
Use PromptParent.openContentPrompt for about:addons options page modals. r=mconley,mixedpuppy
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #0)

This is a regression from setting prompts.contentPromptSubDialog to true (bug 1680637).

This is early-beta-and-earlier so far, so setting 87/88 flags accordingly.

Attached image alert.JPG

Issue is no longer reproducible on 89.0a1 (2021-04-07) (64-bit). Updating flag and bug status accordingly.

Has Regression Range: --- → yes

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #3)

  • in a separate follow up issue (to be files), once we are ready to rip off the content prompt code
    (and the related about:config pref):
    • stop using openContentPrompt
    • remove the stack XUL element currently used in about:addons to host the content prompts
    • adapt the test case to the new prompts (e.g. using PromptTestUtils to wait for the modal prompt),

Hey Luca, did this get filed / fixed? I was looking at bug 1705440 and was wondering if we were ready to remove the old tabmodalprompt cruft, or if about:addons was still relying on this somehow? I don't see any addons hits for openContentPrompt, but also I don't see linked bugs here that removed the code, so I'm a bit lost. :-)

Flags: needinfo?(lgreco)

(In reply to :Gijs (he/him) from comment #13)

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #3)

  • in a separate follow up issue (to be files), once we are ready to rip off the content prompt code
    (and the related about:config pref):
    • stop using openContentPrompt
    • remove the stack XUL element currently used in about:addons to host the content prompts
    • adapt the test case to the new prompts (e.g. using PromptTestUtils to wait for the modal prompt),

Hey Luca, did this get filed / fixed? I was looking at bug 1705440 and was wondering if we were ready to remove the old tabmodalprompt cruft, or if about:addons was still relying on this somehow? I don't see any addons hits for openContentPrompt, but also I don't see linked bugs here that removed the code, so I'm a bit lost. :-)

The followup seems likely to have fallen into the cracks, I'd have expected it to be linked to this issue as a seealso or as depending on this issue if I have filed one. I'm going to run a seach on bugzilla but I think that I may have just forgot to file it, and I'm really sorry about that :-(

I need to refresh my memory about what I meant exactly with "stop using openContentPrompt" exactly (me "shaking fists" to past-himself for not being more precise about that part) but the stack XUL element is definitely still there (to be precise here in aboutaddons.js).

I'm going to re-assign the needinfo to myself to come back with a new filed bug for completing that cleanup (possibly after taking a quick look tomorrow into tracking down some more context for theat other point about "stop using openContentPrompt").

Flags: needinfo?(lgreco)

Needinfo-ing myself back for filing the followup and possibly add some more details about the parts of the cleanup mentioned in the patch comment that are currently unclear.

Flags: needinfo?(lgreco)
  • stop using openContentPrompt

After taking a better look into it I now see that "stop using openContentPrompt" is referring to PromptParent.openContentPrompt, and in particular it refers to the fact that the patch we landed is special casing the prompts originated from the inline options pages to always be handled by Prompts.openContentPrompt independently from the value of the prefs enabling/disabling them (see here in searchfox).

Going through PromptParent.openContentPrompt then requires the stack XUL element wrapping the inline browser element used in about:addons to host the options_ui extensions pages.

One way to stop using PromptParent.openContentPrompt is to go for the other option that we didn't pick when we were fixing the regression, see patch attached as comment 4 in this ticket.

I digged a bit back into why that "second option" patch created by "past-me" had to use the tab's tab prompts instead one tied to the embedded browser and the reason is that (unlike e.g. sidebars extension pages or browserAction/pageAction popup pages) the browser element in this case belongs to a chrome document (the about:addons chrome document) that isn't the same as the one that the chrome window, and TabDialogBox hits an exception when trying to append into the XUL stack element that belongs to the about:addons chrome document, and so the "options 2" patch was going through the easier path (turn the modal into a tab modal).

I'm adding these additional details here as a bugzilla comment in the meantime, but I think that it would be worth touching base and discuss between us about this a bit more so that we can determine what would be the best path forward based on our combined perspectives.

See Also: → 1881055
Flags: needinfo?(lgreco)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: