Closed
Bug 1370967
Opened 8 years ago
Closed 7 years ago
Clicking WebExtension buttons doesn't close the overflow panel
Categories
(Firefox :: Toolbars and Customization, defect, P1)
Tracking
()
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+
Updated•8 years ago
|
Priority: -- → P3
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Assignee | ||
Comment 1•7 years 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-firefox56:
--- → affected
tracking-firefox55:
? → ---
Flags: needinfo?(mixedpuppy)
Keywords: regressionwindow-wanted
Comment 2•7 years ago
|
||
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•7 years 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•7 years ago
|
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
Comment 4•7 years ago
|
||
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•7 years ago
|
Keywords: regression
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.2 - Jul 10
Priority: P2 → P1
Comment 6•7 years 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+
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
Comment 8•7 years ago
|
||
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)
Comment 9•7 years ago
|
||
(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)
Comment 10•7 years ago
|
||
(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•7 years 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).
Updated•7 years ago
|
Flags: needinfo?(ryanvm)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 13•7 years 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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 16•7 years 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]
Comment 17•7 years ago
|
||
Verified on Windows, Mac, and Linux
You need to log in
before you can comment on or make changes to this bug.
Description
•