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.

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?

Is there a type=button or something like that...we should be able to easily do that.

(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?

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.

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.

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)

(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>

(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).

(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).

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

I think others answered the needinfo.

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

Verified on Windows, Mac, and Linux