1896764 - onViewToolbarsPopupShowing and ToolbarContextMenu should move out of browser.js

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, task, P3)

Description

[:Gijs (out for now; he/him)]

To fix this bug:

• create a new module in browser/components/customizableui called ToolbarContextMenu.sys.mjs
• move the ToolbarContextMenu object into that file, and export the object from the file
• move the onViewToolbarPopupShowing event listener to become a method on the same object, and update the few callsites (all in markup)
• import the module lazily in the lazy module getters list in https://searchfox.org/mozilla-central/rev/dd6e430c1bc2db90d9b3b1dd7e5215b4edc4d51a/browser/base/content/browser.js#19
• fix up all the references to this and window and other variables that only exist in the browser window in the new module. Anything handling an event can get menuitem nodes using event.target etc., and from that (event.target.ownerGlobal, probably) it can also get a reference to window and thus window.document.getElementById if it needs other nodes. It's possible (haven't done a detailed check) that there are additional variables that would want to move into this file (or be refactored), if they're only used from this code.

As a bonus, it would be nice to remove the inline event listener for popupshowing for this menu, as well as the command event handling. The former can be added from inside browser-init.js, the latter can be added the first time popupshowing is invoked for a particular node, from inside ToolbarContextMenu.

Comment 1

[Leeya]

Hi Gijs,

Thanks for filing this bug, I'll get to it.

Comment 2

[Leeya]

Hi Gijs,

I'm almost done, just got to the bonus work.

Regarding testing, aside from doing it manually, should I make sure that tests in both browser/base/content/test/ and browser/components/customizableui/test/ pass?

Are there any other tests that I'm missing? Do you think any tests will need to be updated?

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Leeya from comment #2)

Hi Gijs,

I'm almost done, just got to the bonus work.

Regarding testing, aside from doing it manually, should I make sure that tests in both browser/base/content/test/ and browser/components/customizableui/test/ pass?

Yes, that'd be a good set of tests to run locally. And yes, I'd manually check that the context menu on the toolbar (and the entries in it) still work, as well as the View > Toolbars submenu in the main menubar (always visible on macOS; on Windows/Linux you may need to press the "alt" key to show it, or use the toolbar context menu to make it visible!).

Are there any other tests that I'm missing? Do you think any tests will need to be updated?

There probably are some tests but I don't know where they are - a trypush would be the best way to find out.

Stephanie, can we get Leeya (and Steven) try access?

Comment 4

[Stephanie Cunnane [:scunnane]]

Gijs, sure thing - I've kicked off the process for getting Leeya and Steve access to the try server.

Comment 5

[Leeya]

Hi Gijs,

I have a two questions regarding the bonus work.

1. In browser-init.js, which method should I move the inline event listeners for popupshowing to?
   I don't think it's DOMContentLoaded, I'm unsure about onLoad, and I do see what looks like "user-level" events being listened for in _delayedStartup....so I'm leaning towards that last one I listed, but want to be sure.

2. Regarding moving the inline event handlers for command and adding them the first time popupshowing is invoked for a particular node, from inside ToolbarContextMenu Was this what you were talking about?
   In browser-init.js, when popupshowing is fired for a particular node, a command event listener is registered with a handler that will be executed at most once; i.e., the first time popupshowing is fired. When the command event fires, ToolbarContextMenu.commandHandlers executes, calling the appropriate method based on the menuitem's unique data-lazy-l10n-id.

There is some redundancy in ToolbarContextMenu.commandHandlers, the last two switch statements only apply to one of the two menupopup elements...I wasn't sure if extracting them into their own method would be worth possibly reducing readability.

Comment 6

[Leeya]

Hi Gijs,

My first question still stands but please ignore my 2nd one. After rubber ducking it, I realize that my code is not doing what I thought it was and that I need to go back to the drawing board with the command handlers.

Comment 7

[Leeya]

Hi Gijs,

I figured out how to add those command listeners when popupshowing is fired for the first time.

I also think I answered my first question, I should add those inline listeners to DOMContentLoaded rather than wait for all the other resources on the page to load by placing them in onLoad...

Okay, I think I'm good and will move on to testing locally.

Comment 8

[Stephanie Cunnane [:scunnane]]

Gijs, this is to confirm that both Leeya and Steve now have try server access.

Comment 9

[:Gijs (out for now; he/him)]

Sorry for the slow response - I found an open tab where I had started trying to answer but clearly got distracted... Sounds like you're all set now!

Comment 10

[Leeya]

Evening Gijs,

Apologies for pausing on this bug; I had two interviews to prepare for that were back to back. I used the try server and have 90 failures. I definitely should have been testing more along the way. I think moving both ToolbarContextMenu and onViewToolbarPopupShowing was too much to do at once.

Unless you think otherwise, I'm going to abandon my branch and begin again. I still plan on using my code, just breaking down that part into two so I can better parse through the errors that arise.

Also to clarify, I never ran moz-phab so I'd be abandoning it...locally?

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Leeya from comment #10)

Evening Gijs,

Apologies for pausing on this bug; I had two interviews to prepare for that were back to back. I used the try server and have 90 failures. I definitely should have been testing more along the way. I think moving both ToolbarContextMenu and onViewToolbarPopupShowing was too much to do at once.

That makes sense. It's probably easier to start with ToolbarContextMenu. That said, the errors are all the same (the module couldn't be loaded) so you could also try to fix that and then try to re-run the test locally. It looks like you didn't hg add the new module file, and you didn't add it to the moz.build file at https://searchfox.org/mozilla-central/rev/f9139f56bbcc0d587966c007178910ea31df7d58/browser/components/customizableui/moz.build#18-25 so it didn't get packaged. I think if you do both of those and push to try again, you'll get... well, hopefully fewer failures, but definitely different ones!

Unless you think otherwise, I'm going to abandon my branch and begin again. I still plan on using my code, just breaking down that part into two so I can better parse through the errors that arise.

That is also fine, though I will say that the changes in the patch so far look right to me - though then again, they are the removal bits, and adding the new bits is probably the trickier half of the patch. :-)

Also to clarify, I never ran moz-phab so I'd be abandoning it...locally?

If you wanted to go ahead with this, yes - you would just hg pull and update to central and start again.

Do let me know if you're stuck on something or need more guidance!

Comment 12

[Leeya]

Hi Gijs,

Oh wow, thank you for this info. I'll add the new module file and make sure it's packaged before running tests again. I'll decide from there whether to debug or start over.

Good, I'm glad the code looks good at first glance. I'll definitely speak up more, thank you again!

Comment 13

[Leeya]

Hi Gijs!

I have two issues that I need help with. Here are my latest Try server results for context.

First Issue
The onViewToolbarPopupShowing sometimes raises a ReferenceError due to localName. I was able to re-create the error by right-clicking in the toolbar-context-menu area, then hovered over Bookmarks Toolbar which fired onpopupshowing. I put a debugger statement at the top of onViewToolbarPopupShowing and saw that aEvent.target.triggerNode was null, which explains why there was a reference error for localName. Meanwhile, aEvent.target.localName returns "menupopup", which seems to be what we need.

I'm thinking I could try to revise this line of onViewToolbarPopupShowing to -> let toolbarItem = popup.triggerNode || popup.target; to account for the event that aEvent.target.triggerNode is null. I'd like to hear your thoughts before I try it out and test, just in case you know something I don't and can save me some time lol.

Second Issue
I'm getting errors related to chrome://browser/content/browser-menubar.js being an "unreferenced file". Do I need to add that file to browser/base/content/test/static/browser_all_files_referenced.js? If so, where should I put it?

Thank you!

Comment 14

[:Gijs (out for now; he/him)]

(In reply to Leeya from comment #13)

Hi Gijs!

I have two issues that I need help with. Here are my latest Try server results for context.

First Issue
The onViewToolbarPopupShowing sometimes raises a ReferenceError due to localName.

Hi Leeya, can you point to which test shows this in the trypush? I don't see this error.

I was able to re-create the error by right-clicking in the toolbar-context-menu area, then hovered over Bookmarks Toolbar which fired onpopupshowing.
I put a debugger statement at the top of onViewToolbarPopupShowing and saw that aEvent.target.triggerNode was null, which explains why there was a reference error for localName. Meanwhile, aEvent.target.localName returns "menupopup", which seems to be what we need.

I'm thinking I could try to revise this line of onViewToolbarPopupShowing to -> let toolbarItem = popup.triggerNode || popup.target; to account for the event that aEvent.target.triggerNode is null. I'd like to hear your thoughts before I try it out and test, just in case you know something I don't and can save me some time lol.

I think the change from bug 1904029 (see below) is fixing this. You can look at that change. Depending on when you get back to this, if the change has merged to mozilla-central you could rebase, or you could temporarily apply it as part of your patch to keep things moving - it'll come out in the wash once the change does make it to mozilla-central and we rebase. :-)

Second Issue
I'm getting errors related to chrome://browser/content/browser-menubar.js being an "unreferenced file". Do I need to add that file to browser/base/content/test/static/browser_all_files_referenced.js? If so, where should I put it?

No, if you take a look at the diff (either locally or on try) you can see that you've removed some references to browser-menubar.js and [main-popupset.js](https://hg.mozilla.org/try/rev/2dc40ffd8c0b650b91eb633cd38165510013a8e8#l5.9). It looks like you've rebased your patch to a more recent copy of mozilla-central, but have overwritten some changes inbrowser/base/content/browser-menubar.incandbrowser/base/content/main-popupset.inc.xhtml`. Reverting those changes, and then only changing the code you're intending to modify, will fix this.

The caller for onViewToolbarsPopupShowing from the menubar code has moved to here so you'll need to update that instead of changing it in browser-menubar.inc. This is because someone else made changes to how these mechanics work - see bug 1893068. In fact, there's a pending patch that is landing today that specifically changes this code a bit more - bug 1904029. This can happen in a big codebase like this with a lot of people doing work - we end up touching the same code! That's OK, we just have to figure out how to make the same thing happen after the other person's changes.

Hopefully that helps? But let me know if not...

Comment 15

[Leeya]

Hi Gijs

Oh, the ReferenceError was a local one the popped up when I ran my local Firefox build. It may be cleared up once I fix the code I've been overwriting this entire time. Thank you Gijs, this will help loads!

Comment 16

[Leeya]

Attachment: Bug 1896764 - onViewToolbarsPopupShowing and ToolbarContextMenu should move out of browser.js r?Gijs

Comment 17

[Leeya]

Hi Gijs,

Apologies but due to my upcoming job transition, I need to step back from this assignment to focus on ramping up for my new role and to take a need break before starting.

Can we please look into reassigning this bug to another dev? Let me know if you need anything from me, I'm happy to provide any necessary context that'll help with a smooth handover.

Thank you for this invaluable experience!

Comment 18

[:Gijs (out for now; he/him)]

(In reply to Leeya from comment #17)

Hi Gijs,

Apologies but due to my upcoming job transition, I need to step back from this assignment to focus on ramping up for my new role and to take a need break before starting.

Hey Leeya, no worries - good luck on the new job!

Can we please look into reassigning this bug to another dev? Let me know if you need anything from me, I'm happy to provide any necessary context that'll help with a smooth handover.

Yep - Stephanie has found Tim who can take a look at this next. :-)

Comment 19

[Tim Williams]

Hi Gijs, looking forward to working on this. I am reading through the previous comments and patches now to catch up.

Hi Leeya, any advice or context you think would help would be appreciated, but I'll get started now and you focus on your break :) Congratulations and good luck!

Comment 20

[:Gijs (out for now; he/him)]

Hey Tim, is there anything I can help with to figure out how to move forward here? :-)

Comment 21

[Stephanie Cunnane [:scunnane]]

Gijs, from what I understand, Tim is working on some job interview assignments at the moment, but should pick this bug back up once he submits those.

Comment 22

[Tim Williams]

Hi Gijs,

Thank you, Stephanie. Yes, I just finished the interview assignment yesterday so I will be back working on this next week and will have some questions for you then. Thanks for checking in :)

Comment 23

[Tim Williams]

Hi Gijs,

I appreciate your patience. I have just reread through the comments and patches to regain some context. This is my first time taking over a bug that already has a revision in Phabriactor. To get started, should I load Leeya's patch into my local working directory with moz-phab patch D214762 and go from there?

-Tim

Comment 24

[:Gijs (out for now; he/him)]

(In reply to Tim Williams from comment #23)

Hi Gijs,

I appreciate your patience. I have just reread through the comments and patches to regain some context. This is my first time taking over a bug that already has a revision in Phabriactor. To get started, should I load Leeya's patch into my local working directory with moz-phab patch D214762 and go from there?

-Tim

Hi Tim,

Yes, that would be a good start. You can also "commandeer" the revision in phabricator, using the dropdown above the comment box, at the bottom of the phabricator page. That will allow you to use moz-phab submit to send newer/updated versions of the patch to phabricator.

Comment 25

[Tim Williams]

Attachment: Bug 1896764 - onViewToolbarsPopupShowing and ToolbarContextMenu should move out of browser.js.r?scunnane!

Comment 26

[Tim Williams]

Hi Gijs,

Unfortunately, it doesn't seem I have permission to "commandeer" the revision in phabricator-- the only options in my "Add Action" dropdown are "Accept Revision" and "Request Changes". I tried submitting with moz-phab and unfortunately it created a new phab revision for the same bug.

Below is the accompanying message for this latest revision:

I have implemented the inline comments you left for Leeya’s last revision. I manually checked that the context menu on the toolbar and the entries in it still work, as well as the View > Toolbars submenu. However, it looks like quite a few of the tests in browser/components/customizableui/test/ are failing due to my import of the CustomizeMode module and the changes detailed below:

Testing my local build, I got console errors of JavaScript error: resource:///modules/ToolbarContextMenu.sys.mjs, line 83: TypeError: can't access property "isWrappedToolbarItem", gCustomizeMode is undefined.

I added CustomizeMode to the lazy object, which got rid of this initial error, but threw a new one:
JavaScript error: resource:///modules/CustomizeMode.sys.mjs, line 1679: TypeError: can't access property "replaceWith", template is null. This was thrown by the _ensureCustomizationPanels method from the CustomizeMode module. I also got an “Error: not well-formed XML” stemming from CustomizeMode here.

Apart from that, I’m a bit uncertain about what to do to remove the inline event listener for popupshowing and the command event handling. It sounds like Leeya created a method ToolbarContextMenu.commandHandlers and I can see she added oncommand event listeners to browser/base/content/main-popupset.inc.xhtml here but unfortunately her pastebin link from Comment #2 has expired.
I will spend more time tomorrow looking into your guidance of “The former can be added from inside browser-init.js, the latter can be added the first time popupshowing is invoked for a particular node, from inside ToolbarContextMenu.”

Comment 27

[:Gijs (out for now; he/him)]

I've responded on phabricator. :-)

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5701231dcfa
onViewToolbarsPopupShowing and ToolbarContextMenu should move out of browser.js.r=scunnane,rpl

Comment 29

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f5701231dcfa