The opening animation for default_popups for browserActions is different than for the Menu Panel.

VERIFIED FIXED in Firefox 44

Status

()

Toolkit
WebExtensions: Untriaged
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: bwinton, Assigned: bwinton)

Tracking

Trunk
mozilla44
Points:
---
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed, firefox48 verified)

Details

(Whiteboard: [webextension-polish][browserAction])

MozReview Requests

()

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

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
As (hopefully) shown in https://dl.dropboxusercontent.com/u/2301433/Firefox/WebExtensions/PopupAnimation.m4v.  We would like the animation to be the same for the menu panel as for the WebExtension default_popup.

Updated

2 years ago
Whiteboard: [webextension-polish] → [webextension-polish][browserAction]

Updated

2 years ago
Assignee: nobody → bwinton
Blocks: 1214433
(Assignee)

Comment 1

2 years ago
Created attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?martiz, r?kmag, r?MarcoZ
Attachment #8676319 - Flags: review?(mzehe)
Attachment #8676319 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 2

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

(I'll fix up the commit message before I land it.)
Attachment #8676319 - Flags: ui-review?(mjaritz)
(Assignee)

Comment 3

2 years ago
https://dl.dropboxusercontent.com/u/2301433/Firefox/WebExtensions/panel-test.xpi is a reasonable test.  Try putting it on the left side in front of the urlbar, and on the right side in front of the menu button.

Updated

2 years ago
Attachment #8676319 - Flags: review?(mzehe) → review+

Comment 4

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

https://reviewboard.mozilla.org/r/22657/#review20137

r=me for the accessibility parts.
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

https://reviewboard.mozilla.org/r/22657/#review20145

::: browser/components/extensions/ext-utils.js:188
(Diff revision 1)
> +      panel.setAttribute("aria-label", contentViewer.DOMDocument.title);

This should probably be updated when `DOMTitleChanged` fires on the browser. And it's probably best to use `browser.documentTitle`.

feedback+ from me. I'd give r+, but I'm not a peer.
Attachment #8676319 - Flags: review?(kmaglione+bmo)

Updated

2 years ago
Attachment #8676319 - Flags: feedback+
(Assignee)

Comment 6

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm
Attachment #8676319 - Attachment description: MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?martiz, r?kmag, r?MarcoZ → MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm
Attachment #8676319 - Flags: review?(wmccloskey)
Attachment #8676319 - Flags: review?(mjaritz)
Attachment #8676319 - Flags: feedback+
(Assignee)

Comment 7

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm
Attachment #8676319 - Flags: review?(mjaritz)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm
(Assignee)

Comment 9

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm
(Assignee)

Comment 10

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm
(Assignee)

Comment 11

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm
(Assignee)

Comment 12

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm
https://reviewboard.mozilla.org/r/22657/#review20163

::: browser/components/extensions/ext-utils.js:202
(Diff revision 2)
> +      browser.removeEventListener("DOMTitleChanged", titleChangedListener, true);

This listener should be permanent, so that if the panel navigates to a new page (or just loads new content and changes the title), the label gets updated to match. I'm thinking of a panel that loads a login page, and then navigates to an "Add Bookmark" page, for instance.

It might be worth removing when we destroy the panel, but it shouldn't be necessary.
(Assignee)

Comment 14

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm
(Assignee)

Comment 15

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm
Attachment #8676319 - Flags: review?(mjaritz)
(Assignee)

Comment 16

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm
Attachment #8676319 - Flags: review?(mjaritz)
Attachment #8676319 - Flags: review?(wmccloskey) → review+
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

https://reviewboard.mozilla.org/r/22657/#review20229

::: browser/components/extensions/ext-utils.js:131
(Diff revision 3)
> -  panel.setAttribute("flip", "slide");
> +  panel.setAttribute("orient", "vertical");
> +  panel.setAttribute("level", "top");
> +  panel.setAttribute("side", "top");
> +  panel.setAttribute("arrowposition", "after_start");
> +  panel.setAttribute("flip", "both");
> +  panel.setAttribute("role", "group");

I don't know enough about the frontend to know what these attributes mean. Hopefully one of the other reviewers does :-).

::: browser/components/extensions/ext-utils.js:201
(Diff revision 3)
> +    browser.addEventListener("DOMTitleChanged", () => {

I do think we want to remove this listener when the panel goes away. Otherwise we'll leak the panel until the tab goes away. Each time they click the action they'll leak another panel. Removing the listener could be done in the popuphidden listener perhaps.

Comment 18

2 years ago
https://reviewboard.mozilla.org/r/22657/#review20255

::: browser/components/extensions/ext-utils.js:131
(Diff revision 3)
> -  panel.setAttribute("flip", "slide");
> +  panel.setAttribute("orient", "vertical");
> +  panel.setAttribute("level", "top");
> +  panel.setAttribute("side", "top");
> +  panel.setAttribute("arrowposition", "after_start");
> +  panel.setAttribute("flip", "both");
> +  panel.setAttribute("role", "group");

Where did this new set of attributes come from?

If I look at e.g. https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#5 or https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.js#331 only some of them are used, and "flip" should probably stay as "slide"...
(Assignee)

Updated

2 years ago
Attachment #8676319 - Attachment description: MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r?billm → MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r=billm, r?gijs
Attachment #8676319 - Flags: review?(mjaritz)
Attachment #8676319 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 19

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r=billm, r?gijs
(Assignee)

Comment 20

2 years ago
https://reviewboard.mozilla.org/r/22657/#review20229

> I don't know enough about the frontend to know what these attributes mean. Hopefully one of the other reviewers does :-).

(I think Gijs has this covered.  ;)

Comment 21

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

https://reviewboard.mozilla.org/r/22657/#review20301

::: browser/components/extensions/ext-utils.js:131
(Diff revision 4)
> -  panel.setAttribute("flip", "slide");
> +  panel.setAttribute("flip", "both");

You can just remove this line (setting "flip"), and the effect should be the same - the arrow panel binding should take care of this attribute being set to "both" for all arrow panels.
Attachment #8676319 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Attachment #8676319 - Attachment description: MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r?maritz, f=kmag, r=MarcoZ, r=billm, r?gijs → MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs
(Assignee)

Comment 22

2 years ago
Comment on attachment 8676319 [details]
MozReview Request: Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs

Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs
(Assignee)

Comment 23

2 years ago
https://reviewboard.mozilla.org/r/22657/#review20255

> Where did this new set of attributes come from?
> 
> If I look at e.g. https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#5 or https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/content/panelUI.js#331 only some of them are used, and "flip" should probably stay as "slide"...

They came from inspecting the Downloads panel.  I'm happy to remove most of them, but want to keep `role` for a11y, and `flip` really needs to be `both` otherwise the arrow animates in from the wrong side.  Would you be okay with just those two changes?
(Assignee)

Updated

2 years ago
Attachment #8676319 - Flags: review?(mjaritz)
Attachment #8676319 - Flags: ui-review?(mjaritz) → ui-review+
(Assignee)

Comment 24

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/b6d4a3ec377603b0a651d6495c1af56a49e12d8a
Bug 1199056 - Give the WebExtension's panels the same attributes as the other toolbar button's panels. ui-r=maritz, f=kmag, r=MarcoZ, r=billm, r=gijs
https://hg.mozilla.org/mozilla-central/rev/b6d4a3ec3776
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

2 years ago
Flags: blocking-webextensions+
Created attachment 8733352 [details]
Untitled_Screencast.webm

Performed Exploratory testing around this bug on Firefox 48.0a1 (2016-03-21) and the Webextension animation is the same as the Panel menu animation, wherever it is positioned across Windows 10 64-bit and Mac OS X 10.11.

Differences can be seen under Ubuntu 12.04 32-bit where the animation is a bit delayed and some flickers are displayed intermittently in the top left corner of the drop-down. See attached screencast.
Should I file a new bug for this issue?

Tested using: https://addons.allizom.org/en-US/firefox/addon/whimsy-pro-test/
Flags: needinfo?(bwinton)
(Assignee)

Comment 27

a year ago
Wow, that's hard to see…  Can you replicate it with the Menu panel?  (I guess filing a new bug for this would be good.  Also, I'ld love to see the steps to reproduce.  Does it matter how quickly you click?)
Flags: needinfo?(bwinton)
I think this is actually fallout from bug 1217129, since we now open the panel before waiting for the iframe to load. For local content, though, the frame usually finishes loading fast enough that the initial state isn't visible.

I don't really have a good workaround for this that wouldn't require massively refactoring the CustomizableUI widget code, or spinning the event loop while we wait for a load.
(In reply to Blake Winton (:bwinton) (:☕️) from comment #27)
> Wow, that's hard to see…  Can you replicate it with the Menu panel?  (I
> guess filing a new bug for this would be good.  Also, I'ld love to see the
> steps to reproduce.  Does it matter how quickly you click?)

Filed Bug 1259093.
This issue does not reproduces for the panel menu and doesn't matter how quickly you click.

I am marking this bug as Verified since the other issue is tracked separately.
Status: RESOLVED → VERIFIED
status-firefox48: --- → verified
You need to log in before you can comment on or make changes to this bug.