Clicking WebExtension buttons doesn't close the overflow panel

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
2 months ago
14 days ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

53 Branch
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 unaffected, firefox54 unaffected, firefox55 disabled, firefox56 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
STR:

1. install a webextension that has a button that doesn't have a subview, e.g. https://addons.mozilla.org/en-US/firefox/addon/instapaper-official/

2. resize the window so the button ends up in the overflow panel

3. open the overflow panel

4. click the add-on button

ER:
panel closes

AR:
panel stays open


This is a regression - it worked on 54/53.
Flags: qe-verify+
(Assignee)

Updated

2 months ago
Blocks: 1370986

Updated

2 months ago
Priority: -- → P3
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
(Assignee)

Comment 1

2 months ago
The issue here is that these buttons have type=view (which implies they will show their own sub-panel thing), which means the CustomizableUI code doesn't close them itself (instead, it tries to show a view for them). It's not clear to me why this works at all in the main menupanel on non-photon today. I think when I reviewed bug 1217129 I assumed browser_action buttons always had a view, not just some of the time... Shane, do you know why this isn't already broken / what would be a reasonably-scoped fix for this?
Blocks: 1217129
status-firefox55: affected → disabled
status-firefox56: --- → affected
tracking-firefox55: ? → ---
Flags: needinfo?(mixedpuppy)
Keywords: regressionwindow-wanted
Is there a type=button or something like that...we should be able to easily do that.
Flags: needinfo?(mixedpuppy) → needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 3

2 months ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Is there a type=button or something like that...we should be able to easily
> do that.

Yes, but you can't update the widget specification after the fact, and from what I understand WebExtensions can attach/detach their views at will...

My question remains: how does this currently work, when buttons without views are in the main (hamburger) menupanel, if indeed it does?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mixedpuppy)

Updated

2 months ago
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
I think this isn't really a regression, same behavior in the hamburger menu pre-photon.  We probably need to close the parent panel if the button is there.
Flags: needinfo?(mixedpuppy)
(Assignee)

Updated

2 months ago
Keywords: regression
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.2 - Jul 10
Priority: P2 → P1

Comment 6

a month ago
mozreview-review
Comment on attachment 8883043 [details]
Bug 1370967 - close containing popup when clicking a non-popup browser action,

https://reviewboard.mozilla.org/r/154004/#review159670

I'm left wondering if pageAction has a similar issue.
Attachment #8883043 - Flags: review?(mixedpuppy) → review+

Comment 7

a month ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4d0faee8c200
close containing popup when clicking a non-popup browser action, r=mixedpuppy
Backed out for eslint failure: missing comma at browser_ext_browserAction_popup.js:132:

https://hg.mozilla.org/integration/autoland/rev/35cf8f70832cfe22e37e9f2bf193ade0cb33bde2

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4d0faee8c20033d1e4d0aacf694b6845ea295d21&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=112058175&repo=autoland
> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:132:68 | Missing trailing comma. (comma-dangle)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #8)
> Backed out for eslint failure: missing comma at
> browser_ext_browserAction_popup.js:132:

This is one of the most counter-productive and negative, off-putting parts of our infrastructure at the moment, IMHO. When will this become a pre-push hook that runs eslint if there are JS changes? I heard hopeful things during the All-Hands last week, but this backout just reminded me of how painful the current situation is.

<rant, please ignore for a happy day>
Also, tangentially, the comma-dangle rule sucks and the Extensions component sucks for having it switched on whilst the rest of the _frontend_ is not using it.
</rant>
Flags: needinfo?(standard8)
Flags: needinfo?(ryanvm)
Flags: needinfo?(gps)
(In reply to Mike de Boer [:mikedeboer] from comment #9)
> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on
> intermittent or backout) from comment #8)
> > Backed out for eslint failure: missing comma at
> > browser_ext_browserAction_popup.js:132:
> 
> This is one of the most counter-productive and negative, off-putting parts
> of our infrastructure at the moment, IMHO. When will this become a pre-push
> hook that runs eslint if there are JS changes? I heard hopeful things during
> the All-Hands last week, but this backout just reminded me of how painful
> the current situation is.

There's existing commit hooks available today that have been advertised in various places:

https://blog.mozilla.org/standard8/2017/05/04/running-eslint-on-commit-for-mozilla-central-git-mercurial/

Additionally, I'm going to be reviewing bug 1361972 later today which will hopefully get us pre-push hooks as well as commit hooks. I think they'll still need to be installed manually, but that's heading in the right direction.

> <rant, please ignore for a happy day>
> Also, tangentially, the comma-dangle rule sucks and the Extensions component
> sucks for having it switched on whilst the rest of the _frontend_ is not
> using it.
> </rant>

Agreed that isn't ideal. That was a decision of the sub-module owner when they set up ESLint for their directory (they were one of the early ones). As I've mentioned in the FF weekly meetings, we're working on harmonising the rule set as much as possible across the tree, we have been reducing the differences over the last month or so, and there's more already in the pipeline. At the moment I am reluctant to turn off rules that we've already got turned on for various sub-directories until we've decided on their futures across the whole tree.

comma-dangle is one of those stylistic rules, which I think we do want a consistent style, and at some stage we'll look at it, although there may be better ways of doing things by then (e.g. js auto-formatters).
Flags: needinfo?(standard8)
(Assignee)

Comment 11

a month ago
(In reply to Mark Banner (:standard8) from comment #10)
> There's existing commit hooks available today that have been advertised in
> various places:
> 
> https://blog.mozilla.org/standard8/2017/05/04/running-eslint-on-commit-for-
> mozilla-central-git-mercurial/

In my defense, I originally had this enabled, but then it had bugs so I had to disable it to get stuff done - bug 1371597. Which of course got closed as inco because nobody (including me!) could repro, and then I forgot to re-enable it on that particular machine... nobody's fault but mine, even if better automation integration here would be ace.

I'll try to reland this later (and I've re-enabled the local commit hook on this machine).
Flags: needinfo?(ryanvm)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 13

a month ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dadc8c945891
close containing popup when clicking a non-popup browser action, r=mixedpuppy

Comment 14

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dadc8c945891
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
I think others answered the needinfo.
Flags: needinfo?(gps)

Comment 16

a month ago
I have reproduced this Bug with Nightly 55.0a1 (2017-06-07) on Windows 10, 64 Bit!

The bug's fix is now verified on latest Nightly 56.0a1

Build ID 	20170707063454
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170705]
Verified on Windows, Mac, and Linux
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.