1882776 - gPopupBlockerObserver shouldn't live in browser.js

Status: RESOLVED FIXED

(Firefox :: General, task)

Description

[:Gijs (he/him)]

The only consumers are DOM-based (which we may wish to move, but no biggie either way), and the DOMUpdateBlockedPopups event fired from PopupBlockingParent.sys.mjs#178, we could substitute with a simple redirecting event listener that actually loads the module ref if/when a blocked popup is encountered. Or we could add singleton-y functionality directly into PopupBlockingParent and/or a separate module, and make the two talk directly instead of through events...

Comment 1

[:Gijs (he/him)]

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

Or we could add singleton-y functionality directly into PopupBlockingParent and/or a separate module, and make the two talk directly instead of through events...

Though tbf that would require a solution to the fact that PopupBlockingParent lives in toolkit and the UI in browser.

Comment 2

[:Gijs (he/him)]

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

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

Or we could add singleton-y functionality directly into PopupBlockingParent and/or a separate module, and make the two talk directly instead of through events...

Though tbf that would require a solution to the fact that PopupBlockingParent lives in toolkit and the UI in browser.

So the popup blocking code has a lot of moving parts:

• the actors, https://searchfox.org/mozilla-central/source/toolkit/actors/PopupBlockingParent.sys.mjs and its child equivalent
• the gPopupBlockerObserver in browser.js - https://searchfox.org/mozilla-central/rev/b3f40fd7c4671537ed29a232e76c962977650045/browser/base/content/browser.js#1120
• the popupBlocker property on browser elements - https://searchfox.org/mozilla-central/rev/b3f40fd7c4671537ed29a232e76c962977650045/toolkit/content/widgets/browser-custom-element.js#121-123

To fix this bug we want to move the code in browser.js somewhere else - ideally to a singleton-y location (a sys.mjs file) rather than something we load again and again in each window. But, we can't "just" move it to PopupBlockingParent because the code in browser.js is Firefox-desktop specific and in principle the actor code in toolkit and the code in browser-custom-element are used in other circumstances than "just" the main web content area in a Firefox desktop window.

The easiest solution for this is likely to use Integration.sys.mjs. That has a bunch of documentation near the top, and you can look at how Downloads deals with this: https://searchfox.org/mozilla-central/source/toolkit/components/downloads/DownloadIntegration.sys.mjs and https://searchfox.org/mozilla-central/rev/b3f40fd7c4671537ed29a232e76c962977650045/browser/components/downloads/DownloadsViewableInternally.sys.mjs#70 in particular. The browser bits are registered here: https://searchfox.org/mozilla-central/rev/b3f40fd7c4671537ed29a232e76c962977650045/browser/components/BrowserGlue.sys.mjs#1282-1283

So there'd need to be a separate module (probably in browser/modules/, added in the list in https://searchfox.org/mozilla-central/rev/b3f40fd7c4671537ed29a232e76c962977650045/browser/modules/moz.build#126 ) that we register() from browser.js instead of adding an observer.

That singleton module should deal with only adding a single observer (even if register is invoked multiple times).

We should add another (singleton) export to https://searchfox.org/mozilla-central/source/toolkit/actors/PopupBlockingParent.sys.mjs , and both the new browser module and that singleton should use Integration.sys.mjs to have an overridable method - so a no-op implementation in the toolkit file, and a "real" implementation in the browser file.

Then instead of creating a DOMUpdateBlockedPopups event we can call that singleton method, and do the actual work of showing the notification.

Comment 3

[Steve P]

Thanks for the info Gijs! I'll get working on this bug this week, and ping you if I have any questions :)

Comment 4

[Steve P]

Attachment: Bug 1882776 - gPopupBlockerObserver shouldn't live in browser.js. r?Gijs

Comment 5

[Steve P]

Hey Gijs,

I wanted to get your input before going too far in the wrong direction. I've separated out PopupBlockerObserver to a sys.mjs file, added it to the list on moz.build, and imported it into browser.js.

Looking at the Downloads example you gave, it looks like they have a file that uses Integration, and another that calls it. I'm thinking I need the PopupBlockerObserver.sys.mjs to import and use Integration, and then instead of importing it like I have it, in browser.js I call it like you have in this example? https://searchfox.org/mozilla-central/rev/b3f40fd7c4671537ed29a232e76c962977650045/browser/components/downloads/DownloadsViewableInternally.sys.mjs#70

I added a bit of starter code at the top of PopupBlockerObserver.sys.mjs to get an idea of what it will look like. When you have a chance please let me know if I'm on the right path or need to adjust, thanks!

Comment 6

[:Gijs (he/him)]

(In reply to Steve P from comment #5)

Hey Gijs,

I wanted to get your input before going too far in the wrong direction. I've separated out PopupBlockerObserver to a sys.mjs file, added it to the list on moz.build, and imported it into browser.js.

Looking at the Downloads example you gave, it looks like they have a file that uses Integration, and another that calls it. I'm thinking I need the PopupBlockerObserver.sys.mjs to import and use Integration, and then instead of importing it like I have it, in browser.js I call it like you have in this example? https://searchfox.org/mozilla-central/rev/b3f40fd7c4671537ed29a232e76c962977650045/browser/components/downloads/DownloadsViewableInternally.sys.mjs#70

Unfortunately I don't think this is quite right.

However, having thought about this a bit more, I think I've over-complicated the suggested design. Sorry!

Instead, I think the patch you have, minus the Integration parts, can work, and we can simply use the browser/ module you've created and update this callsite:

https://searchfox.org/mozilla-central/rev/c84d3db8d7d6503b1208a0378db640095e106355/browser/base/content/browser-init.js#184

to use:

    gBrowser.addEventListener("DOMUpdateBlockedPopups", e => PopupBlockerObserver.handleEvent(e));

This would mean that we don't load the module until the event is first fired.

We'd want to include the observer in the ChromeUtils.defineESModuleGetters definition here rather than importing it directly at the top of browser.js as your PoC patch has done.

Then, we'll want to make sure that the exported variable in the module is PopupBlockerObserver (minus the g) and update the various references...

Hopefully this helps unblock you for now? Don't hesitate to ask more questions if this wasn't clear.

Comment 7

[Steve P]

Thanks Gijs, that was helpful.

I have a couple follow up questions for you now. browser-init.js now has an error for PopupBlockerObserver being undefined, I tried importing it but I then get an error that the import and export may only appear with sourceType: module, and that I'd have to make some changes elsewhere to allow that in this file. Is that something I should be doing? The weird part is, I've removed all instances of gPopupBlockerObserver, but if I chance that call back to it, the error goes away.

The other question is from your comment on phabricator that I use a "normal" event listener for the menupopup element. When going through and replacing instances, I already have those other event handlers you linked changed from the g version to the updated version, am I reading it right to remove all the <menuitem ...> tags there, and replace it with just one tag, and one event listener here?

Thanks again for all your help!

Comment 8

[:Gijs (he/him)]

(In reply to Steve P from comment #7)

Thanks Gijs, that was helpful.

I have a couple follow up questions for you now. browser-init.js now has an error for PopupBlockerObserver being undefined, I tried importing it but I then get an error that the import and export may only appear with sourceType: module, and that I'd have to make some changes elsewhere to allow that in this file. Is that something I should be doing? The weird part is, I've removed all instances of gPopupBlockerObserver, but if I chance that call back to it, the error goes away.

It looks like you solved this? That is, the patch on phabricator looks good to me in this respect and the linter didn't flag up any issues in browser-init.js on phabricator. If you're still seeing this error, can you clarify when+where it appears?

The other question is from your comment on phabricator that I use a "normal" event listener for the menupopup element. When going through and replacing instances, I already have those other event handlers you linked changed from the g version to the updated version, am I reading it right to remove all the <menuitem ...> tags there, and replace it with just one tag, and one event listener here?

The menuitem tags should stay, but all the oncommand and onpopupshowing etc. attributes should disappear, being replaced with event listeners added from JS.

So I think there should be 1 command event listener (and, from the looks of it, a popuphiding and popupshowing event listener), and you can attach them to the menupopup from JS (ie from the new JS module you've created) before showing the menupopup.

That'd probably mean that at the start of this block you could use document.getElementById to get a reference to the menupopup, and then call addEventListener. You'll then want to guard that in some way so we only add the listeners once for each browser window / menupopup (not every time that code runs).

You'll want to change the existing handleEvent code to check the aEvent.type (in a switch statement a bit like here ) and call different methods for each event (popupshowing, popuphiding, command and the existing DOMUpdateBlockedPopups being the different event types). Some of these exist: onPopupHiding and fillPopupList each get invoked for one of the events. But the existing handleEvent only handles DOMUpdateBlockedPopups and so most of the contents will want breaking out into a new method, so that handleEvent just has a switch statement delegating to different other methods based on the event type.

In the oncommand case you'll want a new method, that then checks aEvent.target to see which item got clicked on, and invokes the right callback. Then this code that is setting an oncommand attribute dynamically can also be removed.

Thanks again for all your help!

Nah - thank you for working on this! :-)

Comment 9

[Steve P]

Hi Gijs, sorry for the delay in response. Been a busy few weeks. I added a new commit to moz-phab over the weekend, would you mind taking a look when you have a chance please? Thanks!

Comment 10

[:Gijs (he/him)]

Looks like you're on the right track - I've responded on phab, let me know if that clears things up or if there's more bits that you're unsure about? Thank you for your continued work on this!

Comment 11

[Steve P]

I just commented on phab again about the newest issue I'm facing - I think after getting this one fixed we should be done with this patch (fingers crossed)

Comment 12

[:Gijs (he/him)]

(In reply to Steve P from comment #11)

I just commented on phab again about the newest issue I'm facing - I think after getting this one fixed we should be done with this patch (fingers crossed)

It's a 1-character typo (see phab). Happens to the best of us!

Once you've got something working (or have another question) please submit for review and I can take a look directly on phab, then we can stop the ping-pong in the bug. ;-)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4062a188aba5
gPopupBlockerObserver shouldn't live in browser.js. r=Gijs

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/4062a188aba5