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

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Attachment: 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

Comment 2

[Blake Winton (:bwinton) (:☕️)]

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

Comment 3

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 4

[Marco Zehe (:MarcoZ)]

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 5

[Kris Maglione [:kmag]]

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.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

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

Comment 7

[Blake Winton (:bwinton) (:☕️)]

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

Comment 8

[Blake Winton (:bwinton) (:☕️)]

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

Comment 9

[Blake Winton (:bwinton) (:☕️)]

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

Comment 10

[Blake Winton (:bwinton) (:☕️)]

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

Comment 11

[Blake Winton (:bwinton) (:☕️)]

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

Comment 12

[Blake Winton (:bwinton) (:☕️)]

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

Comment 13

[Kris Maglione [:kmag]]

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.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

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

Comment 15

[Blake Winton (:bwinton) (:☕️)]

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

Comment 16

[Blake Winton (:bwinton) (:☕️)]

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

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

[:Gijs (he/him)]

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"...

Comment 19

[Blake Winton (:bwinton) (:☕️)]

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

Comment 20

[Blake Winton (:bwinton) (:☕️)]

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

[:Gijs (he/him)]

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.

Comment 22

[Blake Winton (:bwinton) (:☕️)]

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

Comment 23

[Blake Winton (:bwinton) (:☕️)]

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?

Comment 24

[Blake Winton (:bwinton) (:☕️)]

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

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6d4a3ec3776

Comment 26

[Vasilica Tamas, QA [:vtamas]]

Attachment: 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/

Comment 27

[Blake Winton (:bwinton) (:☕️)]

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

Comment 28

[Kris Maglione [:kmag]]

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.

Comment 29

[Vasilica Tamas, QA [:vtamas]]

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