Closed Bug 1370967 Opened 7 years ago Closed 7 years ago

Clicking WebExtension buttons doesn't close the overflow panel

Categories

(Firefox :: Toolbars and Customization, defect, P1)

53 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.2 - Jul 10
Tracking Status
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- disabled
firefox56 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [photon-structure])

Attachments

(1 file)

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+
Blocks: 1370986
Priority: -- → P3
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
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
Flags: needinfo?(mixedpuppy)
Is there a type=button or something like that...we should be able to easily do that.
Flags: needinfo?(mixedpuppy) → needinfo?(gijskruitbosch+bugs)
(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)
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)
Keywords: regression
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.2 - Jul 10
Priority: P2 → P1
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+
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)
(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)
Flags: needinfo?(gijskruitbosch+bugs)
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
https://hg.mozilla.org/mozilla-central/rev/dadc8c945891
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
I think others answered the needinfo.
Flags: needinfo?(gps)
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
Flags: qe-verify+
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: