1354109 - Update webextensions code to put things in the permanent area in the overflow panel instead of the hamburger menu

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P1)

Description

[:Gijs (he/him)]

When we replace the customizable hamburger menu with a non-customizable menu, and replace the customizable part of the hamburger with a customizable, fixed area in the overflow panel, we'll need to update the WebExtensions functionality that puts buttons there by default.

Aaron/Bryan: I assume we should put things in the overflow panel instead, if add-ons specify they want to be in the hamburger panel?

Comment 1

[Aaron Benson]

That's right. It might also be worth pointing out that, as an upgrade path, icons that were placed in the hamburger menu by the user should show up in the overflow menu once they migrate to Photon.

Comment 2

[:Gijs (he/him)]

(In reply to abenson from comment #1)
> That's right. It might also be worth pointing out that, as an upgrade path,
> icons that were placed in the hamburger menu by the user should show up in
> the overflow menu once they migrate to Photon.

Yep, this was on my radar. Thanks for the quick response!

Comment 3

[Kris Maglione [:kmag]]

It's been suggested that this might interfere with some of our extension install doorhangers, which are attached to the hamburger menu.

Comment 4

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #3)
> It's been suggested that this might interfere with some of our extension
> install doorhangers, which are attached to the hamburger menu.

Doorhangers in the new-new install flow are currently attached to the addons icon in the location bar (#addons-notification-icon to be specific) so I don't think doorhangers are a concern.  This is pending bug 1315739, but there hasn't been any movement on that bug in a while so I don't expect any changes soon.
But we do put a badge on the hamburger menu button and then add items near the bottom of the menu when sideloaded extensions are detected at startup and when there are background updates that require new permissions.  I know very little about Photon (though I would like to know more, I've looked at the wiki page which looks to be a lot of project management type details, what's a good place to see mocks of the actual visual changes?)  Regardless, if the hamburger menu is going away, it does sound like we'll need to update the two flows I mentioned above.  I think there are separate issues with webextension browser actions, so a new bug under the Toolkit :: Add-ons Manager component would be good if we do need to change something.

Comment 5

[:Gijs (he/him)]

(In reply to Andrew Swan [:aswan] from comment #4)
> But we do put a badge on the hamburger menu button and then add items near
> the bottom of the menu when sideloaded extensions are detected at startup
> and when there are background updates that require new permissions.  I know
> very little about Photon (though I would like to know more, I've looked at
> the wiki page which looks to be a lot of project management type details,
> what's a good place to see mocks of the actual visual changes?)  Regardless,
> if the hamburger menu is going away, it does sound like we'll need to update
> the two flows I mentioned above.

The hamburger panel is being replaced with a not-quite-the-same panel that is no longer customizable. I filed bug 1355020 and bug 1355021 for including these notifications.

Separate doorhangers that anchor on the panel menu shouldn't really interfere with anything here - all we're doing is basically swapping out the <popup> that we normally show with a different one.

>  I think there are separate issues with
> webextension browser actions, so a new bug under the Toolkit :: Add-ons
> Manager component would be good if we do need to change something.

What kind of separate issues? I filed this bug mostly just to update the code from bug 1313459 so items get added to the 'right' panel area. I'm not aware of other issues. Can you clarify? Also, I thought the browser action stuff would be in the WebExtensions components, rather than the add-on manager itself...

Comment 6

[Andrew Swan [:aswan]]

(In reply to :Gijs from comment #5)
> >  I think there are separate issues with
> > webextension browser actions, so a new bug under the Toolkit :: Add-ons
> > Manager component would be good if we do need to change something.
> 
> What kind of separate issues? I filed this bug mostly just to update the
> code from bug 1313459 so items get added to the 'right' panel area. I'm not
> aware of other issues. Can you clarify? Also, I thought the browser action
> stuff would be in the WebExtensions components, rather than the add-on
> manager itself...

Maybe I'm misunderstanding the docs, I thought that setting "default_area" to "menu_panel" (see https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action#Syntax) puts an action into the hamburger menu?
And sorry I was unclear with the new bug comment, I meant this bug could remain for things specific to webextensions and that a new add-ons manager bug would be appropriate for anything specific to the install (and update) process, but it looks like the install stuff will just be covered by the new bugs you just filed?

Comment 7

[:Gijs (he/him)]

(In reply to Andrew Swan [:aswan] from comment #6)
> (In reply to :Gijs from comment #5)
> > >  I think there are separate issues with
> > > webextension browser actions, so a new bug under the Toolkit :: Add-ons
> > > Manager component would be good if we do need to change something.
> > 
> > What kind of separate issues? I filed this bug mostly just to update the
> > code from bug 1313459 so items get added to the 'right' panel area. I'm not
> > aware of other issues. Can you clarify? Also, I thought the browser action
> > stuff would be in the WebExtensions components, rather than the add-on
> > manager itself...
> 
> Maybe I'm misunderstanding the docs, I thought that setting "default_area"
> to "menu_panel" (see
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/
> browser_action#Syntax) puts an action into the hamburger menu?

Yes. After the changes for Photon, the action should be in the overflow panel by default. It should be a pretty straightforward change, we just need to make sure we do it. :-)

> And sorry I was unclear with the new bug comment, I meant this bug could
> remain for things specific to webextensions and that a new add-ons manager
> bug would be appropriate for anything specific to the install (and update)
> process, but it looks like the install stuff will just be covered by the new
> bugs you just filed?

Yes, that should be covered. Thanks!

Comment 8

[:Gijs (he/him)]

Shane, are you happy with comment #7 or do you think this means we should reconsider shipping bug 1313459?

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs from comment #8)
> Shane, are you happy with comment #7 or do you think this means we should
> reconsider shipping bug 1313459?

I am fine with comment 7, it is a good solution.  I am also fine with removing "menu_panel" as an option if that is better for the future, but we'd need to do it now and uplift through beta.  You're in a better position to have an eye towards what might happen later.

Comment 10

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4414fdff7cbac8170706e341a4861c1f30d4153e

Comment 11

[:Gijs (he/him)]

Attachment: Bug 1354109 - update WebExtensions code for Photon,

Review commit: https://reviewboard.mozilla.org/r/146260/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146260/

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1354109 - fix min-width issues with panels,

Review commit: https://reviewboard.mozilla.org/r/146262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146262/

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8874879 [details]
Bug 1354109 - fix min-width issues with panels,

https://reviewboard.mozilla.org/r/146262/#review150250

LGTM!

Comment 14

[:Gijs (he/him)]

So, the trypush has some orange in browser_ext_browserAction_popup_resize.js (and then it has other orange, not relevant to this bug). It looks like this:

Document should not be vertically scrollable - Got 15, expected 0

On my mbp, both before and after flipping the photon pref (and these patches), I get errors that look like this:

Document should not be vertically scrollable - Got 1, expected 0

which look like rounding errors to me. Obviously, not being able to reproduce the issue, I'm really not sure what the deal is with this, though it might get fixed as part of bug 1369339...

In any case, the trypush flips the pref but these patches do not, so I think this is still reviewable as-is.

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8874878 [details]
Bug 1354109 - update WebExtensions code for Photon,

https://reviewboard.mozilla.org/r/146260/#review151540

Are we currently running tests in both photon and non-photon modes somewhere?

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:236
(Diff revision 1)
> -  await testInArea(CustomizableUI.AREA_PANEL);
> +  await testInArea(gPhotonStructure ? CustomizableUI.AREA_FIXED_OVERFLOW_PANEL
> +                                    : CustomizableUI.AREA_PANEL);

Can we add a helper to head.js for this? `getPanelAreaID()` or something?

::: browser/components/extensions/test/browser/head.js:223
(Diff revision 1)
>    } else if (group.areaType == CustomizableUI.TYPE_MENU_PANEL) {
> +    if (gPhotonStructure) {

It seems like we should need to change the areaType comparison rather than check `gPhotonStructure` here...

Comment 16

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #15)
> Comment on attachment 8874878 [details]
> Bug 1354109 - update WebExtensions code for Photon,
> 
> https://reviewboard.mozilla.org/r/146260/#review151540
> 
> Are we currently running tests in both photon and non-photon modes somewhere?

Not yet. We will soon enable photon structure on trunk, and then m-c and inbound trees will run tests with photon enabled. We have the cedar twig which will run tests with photon disabled.

> :::
> browser/components/extensions/test/browser/browser_ext_browserAction_popup.
> js:236
> (Diff revision 1)
> > -  await testInArea(CustomizableUI.AREA_PANEL);
> > +  await testInArea(gPhotonStructure ? CustomizableUI.AREA_FIXED_OVERFLOW_PANEL
> > +                                    : CustomizableUI.AREA_PANEL);
> 
> Can we add a helper to head.js for this? `getPanelAreaID()` or something?

Good idea, will do.

> ::: browser/components/extensions/test/browser/head.js:223
> (Diff revision 1)
> >    } else if (group.areaType == CustomizableUI.TYPE_MENU_PANEL) {
> > +    if (gPhotonStructure) {
> 
> It seems like we should need to change the areaType comparison rather than
> check `gPhotonStructure` here...

The overflow panel is still the same area type as the pre-photon menu panel - both of them are panels. The actual area id will be different. I could alternatively get the placement object with getPlacementOfWidget(group.id) - there is no getter for the area id on the widget group wrapper, unfortunately - but this seemed simpler, especially given all of this will go away for 57 (that is, the branching will disappear again). I'll add a comment.


I still need to figure out the browser_ext_browserAction_popup_resize.js failures mentioned in comment #14...

Comment 17

[:Gijs (he/him)]

Comment on attachment 8874878 [details]
Bug 1354109 - update WebExtensions code for Photon,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146260/diff/1-2/

Comment 18

[:Gijs (he/him)]

Comment on attachment 8874878 [details]
Bug 1354109 - update WebExtensions code for Photon,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146260/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce403be1e983
update WebExtensions code for Photon, r=kmag

Comment 20

[:Gijs (he/him)]

(In reply to Pulsebot from comment #19)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/ce403be1e983
> update WebExtensions code for Photon, r=kmag

This won't flip the pref yet; I'll file a followup for that and deal with any further issues there. The orange that occurs then isn't a new bug, it's a bug in the panelmultiview that is revealed by using a panel with a (vertically) smaller main view as the location for the webextension item (that is, if there are fewer items in the main hamburger panel, it misbehaves in the same way). Specifically, when the panel view contents are wide, we display a horizontal scrollbar. This reduces the available vertical space (height), but because the browser and panelview are made to match, the presence of the horizontal scrollbar then causes a vertical one to appear because the heights then mismatch. We don't hit this right now because the main menupanel has a bunch of items in it by default, making it relatively tall - to the point where, because we don't ever resize the panel to be less tall, the height of the webextension panelview is not sufficient to trigger a vertical scrollbar (though a horizontal one is present).

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[u554753]

Hi Gijs,

Any suggested STR to be able to verify this bug?

Thanks!

Comment 23

[:Gijs (he/him)]

I think the automated test coverage is sufficient for this bug. Thanks, Grover!