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

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Frontend
P1
normal
RESOLVED FIXED
3 months ago
a day ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

53 Branch
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-structure], triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

59 bytes, text/x-review-board-request
kmag
: review+
Details | Review
(Assignee)

Description

3 months ago
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?
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
(Assignee)

Updated

3 months ago
Component: Toolbars and Customization → WebExtensions: Frontend
Product: Firefox → Toolkit

Comment 1

3 months ago
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.

Updated

3 months ago
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
(Assignee)

Comment 2

3 months ago
(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!
Blocks: 1352693
No longer blocks: 1349210
Summary: Update webextensions code to put things in the (permanent?) overflow panel or toolbar instead of the hamburger menu → Update webextensions code to put things in the permanent area in the overflow panel instead of the hamburger menu

Comment 3

3 months ago
It's been suggested that this might interfere with some of our extension install doorhangers, which are attached to the hamburger menu.
Flags: needinfo?(aswan)

Comment 4

3 months ago
(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.
Flags: needinfo?(aswan) → needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 5

3 months ago
(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...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(aswan)

Comment 6

3 months ago
(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?
Flags: needinfo?(aswan)
(Assignee)

Comment 7

3 months ago
(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!

Updated

2 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]

Updated

2 months ago
Whiteboard: [photon-structure] → [photon-structure], triaged

Updated

2 months ago
Blocks: 1358881
(Assignee)

Comment 8

2 months ago
Shane, are you happy with comment #7 or do you think this means we should reconsider shipping bug 1313459?
Flags: needinfo?(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.
Flags: needinfo?(mixedpuppy)
(Assignee)

Updated

18 days ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1
(Assignee)

Comment 10

18 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4414fdff7cbac8170706e341a4861c1f30d4153e
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

18 days ago
mozreview-review
Comment on attachment 8874879 [details]
Bug 1354109 - fix min-width issues with panels,

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

LGTM!
Attachment #8874879 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 14

17 days ago
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

16 days ago
mozreview-review
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...
Attachment #8874878 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 16

16 days ago
(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 hidden (mozreview-request)
(Assignee)

Updated

15 days ago
Attachment #8874879 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 19

12 days ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ce403be1e983
update WebExtensions code for Photon, r=kmag
(Assignee)

Comment 20

12 days ago
(In reply to Pulsebot from comment #19)
> Pushed by gijskruitbosch@gmail.com:
> 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).

Updated

11 days ago
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26

Comment 21

11 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce403be1e983
Status: ASSIGNED → RESOLVED
Last Resolved: 11 days ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Hi Gijs,

Any suggested STR to be able to verify this bug?

Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 23

2 days ago
I think the automated test coverage is sufficient for this bug. Thanks, Grover!
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(gijskruitbosch+bugs)
QA Contact: gwimberly
You need to log in before you can comment on or make changes to this bug.