1217129 - Use CustomizableUI view widget for BrowserAction panels

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Markus Jaritz [:designakt] (UX)]

Attachment: browserActionPressedWhilePanelOpen.gif

While a panel is open, the button should stay pressed and clicking it again should result in closing the panel. Currently sometimes instead of closing, another panel overlays the previous. (Especially when quickly trying to close the panel again)

Comment 1

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

I'll take this one…

Comment 2

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

Attachment: MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. ui-r=maritz, r=billm

Comment 3

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

https://reviewboard.mozilla.org/r/22945/#review20517

::: browser/components/extensions/ext-utils.js:114
(Diff revision 1)
>  // Open a panel anchored to the given node, containing a browser opened

Please update the comment.

::: browser/components/extensions/ext-utils.js:131
(Diff revision 1)
> +    return panel.hidePopup && panel.hidePopup();

Let's not do this hidePopup check. We only call this code when we get a click event, so I don't see how we could be quitting at the same time. Presumably something else must be doing on in Jetpack.

Comment 4

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. ui-r=maritz, r=billm

Comment 5

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. ui-r=maritz, r=billm

Comment 6

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

https://reviewboard.mozilla.org/r/22945/#review20601

Comment 7

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/22945/#review20617

::: browser/components/extensions/ext-utils.js:124
(Diff revision 2)
> +    return panel.hidePopup();

There's a race here. We insert the panel into the document immediately, but don't open it until the page loads. Calling `hidePopup` before it opens will essentially have no effect. This probably still work better than the previous behavior, but it might have some undesirable effects if the panel load stalls.


Anyway, I'll file a separate bug for that.

Comment 8

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

Oh!  I totally forgot the first part of the bug, where the button doesn't stay pressed!  So, next patch fixes that, and while I was there, tries to address the race Kris pointed out…

Comment 9

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. ui-r=maritz, r=billm

Comment 10

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

(Since it's a bit of a code change, I'm re-requesting review from Bill.)

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ed0888a0a4a

Comment 12

[Markus Jaritz [:designakt] (UX)]

Attachment: pressedButtonTest.gif

Over-all behavior is fine, but somehow the button, instead of staying pressed, flickers while opening. Noticeable on the thick border appearing on the pressed state, disappearing, and coming back again to stay pressed.
This behavior does not exist on the menu- and download-buttons, but exist even more obviously on the hello button (bug 1218797).

Comment 13

[Markus Jaritz [:designakt] (UX)]

And on opening another panel, the webextension button stays active slightly longer than menu button does, leading to a noticeable overlap where both buttons appear to be pressed.

Comment 14

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

Okay, I think I fixed the flickering.  I also did what I could for the overlap, and now I believe it's mostly the animation making it look like both buttons are pressed.  Is the new version worse than the downloads button?  Is it worse enough to stop this improvement from landing?

Comment 15

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. ui-r=maritz, r=billm

Comment 16

[Markus Jaritz [:designakt] (UX)]

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Looks perfect! Thx.

Comment 17

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

https://reviewboard.mozilla.org/r/22945/#review21093

r- because of the openPopup issue.

::: browser/components/extensions/ext-utils.js:119
(Diff revision 4)
> +  node.setAttribute("open", "true");

What's this "open" attribute for? I don't see it used.

::: browser/components/extensions/ext-utils.js:128
(Diff revision 4)
> +      return panel.openPopup(anchor, "bottomcenter topright", 0, 0, false, false);

This is not correct. We're supposed to actually reload the popup HTML every time they click the button. Adblock Plus for Chrome depends on this, and probably lots of other add-ons. I'm sorry we don't have a test for this, but we should.

::: browser/components/extensions/ext-utils.js:169
(Diff revision 4)
> +    node.removeAttribute("open");

Same thing about "open".

Comment 18

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/22945/#review21093

> This is not correct. We're supposed to actually reload the popup HTML every time they click the button. Adblock Plus for Chrome depends on this, and probably lots of other add-ons. I'm sorry we don't have a test for this, but we should.

We should only hit this case if the button was clicked while we're waiting for the popup to open. If the previous popup was shown and then closed, we'd still get a fresh load. Fortunately, we *do* have a test for this. :)

Comment 19

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/22945/#review20983

::: browser/components/extensions/ext-utils.js:124
(Diff revision 4)
> +    if (panel.getAttribute("panelopen") === "true") {

Ah. Interesting solution. I like it. I'd suggest using `.popupState` rather than the `panelopen` attribute, though.

Comment 20

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. ui-r=maritz, r=billm

Comment 21

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

(Kris: I ended up using `panel.state`, which uses `.popupState` internally, because it seems like it'll break less if things change.  :)

Comment 22

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

https://reviewboard.mozilla.org/r/22945/#review21409

::: browser/components/extensions/ext-utils.js:130
(Diff revision 5)
> +      return panel.openPopup(anchor, "bottomcenter topright", 0, 0, false, false);

This doesn't really address Kris's original comment about the race. The user clicked on the button and it's taking a long time to load. So they click again and now we'll show a panel that's presumably empty. It might not have any content, so maybe it will just have size 0? I'd kinda rather we do nothing and just return here.

The right thing to do might be to cancel opening the popup. That's not that easy, though, and I doubt this will happen much.

Maybe file a follow-up for this issue? In any case, I think this patch is fine.

Comment 23

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

https://hg.mozilla.org/integration/fx-team/rev/7733bfad72ab644da6415b1acd5bf8bb9fae84d7
Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. ui-r=maritz, r=billm

Comment 24

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

https://reviewboard.mozilla.org/r/22945/#review21409

> This doesn't really address Kris's original comment about the race. The user clicked on the button and it's taking a long time to load. So they click again and now we'll show a panel that's presumably empty. It might not have any content, so maybe it will just have size 0? I'd kinda rather we do nothing and just return here.
> 
> The right thing to do might be to cancel opening the popup. That's not that easy, though, and I doubt this will happen much.
> 
> Maybe file a follow-up for this issue? In any case, I think this patch is fine.

In that case, shouldn't the panel be in the `opening` state, not the `open` state, and so we would ignore the second click, and end up with an open panel when its done loading?

Comment 25

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

(In reply to Blake Winton (:bwinton) (:☕️) from comment #24)
> In that case, shouldn't the panel be in the `opening` state, not the `open`
> state, and so we would ignore the second click, and end up with an open
> panel when its done loading?

I don't think so. We don't call openPopup until everything is done loading, so I think the state will just be "closed" until loading the popup content is complete.

Comment 26

[Kris Maglione [:kmag]]

I think, with the patch, the second time the button is clicked, if the popup hasn't opened yet, it should open with a partially loaded iframe. If it's clicked again, the popup should close and be removed from the document, whether it's finished loading or not.

There are still a few ways that's not great, but I think any solutions that are better than this one are outside the scope of this patch. I'll file a follow-up bug today.

Comment 27

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

(In reply to Kris Maglione [:kmag] from comment #26)
> I think, with the patch, the second time the button is clicked, if the popup
> hasn't opened yet, it should open with a partially loaded iframe. If it's
> clicked again, the popup should close and be removed from the document,
> whether it's finished loading or not.
> 
> There are still a few ways that's not great, but I think any solutions that
> are better than this one are outside the scope of this patch. I'll file a
> follow-up bug today.

Well, hold up a second there.  The patch got backed out for test failures, so I'll need to go mucking about in the code again, and maybe I can fix the behaviour while I'm there…

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out by https://hg.mozilla.org/integration/fx-team/rev/9891a6101cfd because of bc failures like

09:23:49     INFO -  7 INFO TEST-START | browser/components/extensions/test/browser/browser_ext_browserAction_simple.js
09:23:49     INFO -  JavaScript error: chrome://browser/content/ext-utils.js, line 130: ReferenceError: can't access lexical declaration `anchor' before initialization

and more browser_ext_* failures.

Comment 29

[Kris Maglione [:kmag]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #27)
> Well, hold up a second there.  The patch got backed out for test failures,
> so I'll need to go mucking about in the code again, and maybe I can fix the
> behaviour while I'm there…

I think it would be better to save it for a separate bug. I'm pretty sure it's going to take a fair amount of restructuring to handle attempts to close the popup before it's finished loading fully (e.g., for bug 1190663).

I think your solution is good for now, I just think we need a follow-up to iron out some more corner cases when we wind up opening/closing the popup early.

Comment 30

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

(In reply to Kris Maglione [:kmag] from comment #29)
> I think your solution is good for now, I just think we need a follow-up to
> iron out some more corner cases when we wind up opening/closing the popup
> early.

So, it turns out that the solution I posted _doesn't_ reload the popup's html when the panel is re-opened, which is what (eventually) caused the test failures.  So I'm going with something slightly different, but it's exposing a bug in the underlying toolbarbutton code, but maybe I'm just going to live with that…  Patch coming to reviewboard in a couple of seconds.

Comment 31

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22945/diff/5-6/

Comment 32

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12835ee386fb

Comment 33

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

And now the oddities…

This behaves the same on the downloads button, so I'm not sure if I'm going to fix it in this bug, but:
1) If you have an open panel, and you click on the button at the bottom corners (not on the icon), then the panel doesn't go away.
2) If you have an open panel, and you hold down the button until the popup is gone, then let go, the popup re-appears.

Comment 34

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

Can you explain why this is the right thing to do? The state will only be "hiding" if we're within a popuphiding event handler. It might help if you could post a stack of how we get here from popuphiding.

Also, it sounds like we're no longer going to be toggling with this new patch. Is that true?

Comment 35

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

https://reviewboard.mozilla.org/r/22945/#review21765

From talking to Blake, here's what happens on Windows:
- When the user clicks on the button again, it's outside the panel, which causes the panel to start hiding.
- The state remains "hiding" until the panel closing animation is complete.
- Meanwhile, we get a "command" event because the button was clicked.
- And the panel is (hopefully) still in the "hiding" state at that point.

This is kinda sketchy. For example, on Linux we don't do the animation and so I think we won't be in the hiding state. However, the downloads button doesn't toggle for me on Linux, probably for the same reason. So I guess we'll have the same behavior as the downloads button.

Comment 36

[Markus Jaritz [:designakt] (UX)]

Attachment: multiplePanelOpenOnRepeatedClick-2.gif

I currently see a second panel opening if I try closing the panel too quick.
And I also have 2 panels to close after that.
If I click near the edge of the button, I can open as many duplicates as I want, which seams to me should not be possible. It is even hard closing them again as they stay when I open other panels.
(experienced on Windows 10)

Comment 37

[Kris Maglione [:kmag]]

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

I think Bill has this pretty well covered, so I'm unflagging myself for review.

Your latest change and comment 35 make me think that we should revisit how we're interacting with the popup lifecycle, though, so I'm holding off filing a follow-up bug until I understand a few things better. But it seems like we should begin showing the popup, and enter the showing state, as soon as the button is clicked, and finish showing it/enter the shown state, once the browser has loaded. Which should simplify all the state checks we need for this bug, and allow us to just set the "open" attribute based on popupshowing/popuphidden events.

Comment 38

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

Yeah, see also https://bugzilla.mozilla.org/show_bug.cgi?id=1218797#c6  It might be nice if we didn't need to do any of this stuff, and it could all be fixed in platform.  (Although I wonder how much of it is our fault for using `type: "custom"` instead of `type: "view",` in the call to CustomizableUI.createWidget…)

Comment 39

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22945/diff/6-7/

Comment 40

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

WIP, don't review!  But any feedback on the approach would be appreciated…  ;)

One of the nice things with this is that we can get rid of a bunch of the browserAction code, _and_ we get stuff like https://www.dropbox.com/s/rjjqmrqcor9s9up/Screenshot%202015-11-10%2016.53.51.png?dl=0 for free.

So, what do you all think?

Comment 41

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/22945/#review22387

::: browser/components/extensions/ext-browserAction.js:74
(Diff revisions 6 - 7)
> -          if (popup) {
> +        loadPanel(event.target, popup, this.extension);

What happened to the click event?

::: browser/components/extensions/ext-utils.js:125
(Diff revisions 6 - 7)
> +      (panel.state === "hiding" || panel.state === "open")) {

Why just return if the panel is already open?

::: browser/components/extensions/ext-utils.js:137
(Diff revisions 6 - 7)
> +    panel.parentNode.removeChild(panel);

`if (panel) panel.remove()` would do... but why are you removing it?

::: browser/components/extensions/ext-utils.js:223
(Diff revisions 6 - 7)
> +global.loadPanel = (panel, popupURL, extension) => {

So we're sharing the panel and browser between all WebExtensions now? There might be some side-effects that I don't know enough about (especially once we have OOP extensions), so I'll leave this part for Bill to review.


I don't really like how much code this duplicates from the pageAction version, though.

::: browser/components/extensions/ext-utils.js:259
(Diff revisions 6 - 7)
> +    debugger;

Nope. :)

::: browser/components/extensions/ext-utils.js:264
(Diff revisions 6 - 7)
> +    height /= window.devicePixelRatio;

I know this isn't new, but this should really only happen for the values returned by getContentSize.

I definitely like that this uses more of the CustomizableUI code for panel management, but I'm not so sure about sharing browsers between extensions, or duplicating so much of the logic across page action and browser panels.

Comment 42

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

(In reply to Kris Maglione [:kmag] from comment #41)
> > -          if (popup) {
> > +        loadPanel(event.target, popup, this.extension);
> What happened to the click event?

I'll add it back if this approach seems reasonable…

> ::: browser/components/extensions/ext-utils.js:125
> (Diff revisions 6 - 7)
> > +      (panel.state === "hiding" || panel.state === "open")) {
> Why just return if the panel is already open?

Because the panel will be closed by the panel's click handler.  (For Linux, I think, because they don't have an animation?)  I'm hoping this code goes away, too, and I can use the same panelview for the pageActions, too.

> ::: browser/components/extensions/ext-utils.js:137
> (Diff revisions 6 - 7)
> > +    panel.parentNode.removeChild(panel);
> `if (panel) panel.remove()` would do... but why are you removing it?

I'm hoping this code goes away…

> ::: browser/components/extensions/ext-utils.js:223
> (Diff revisions 6 - 7)
> > +global.loadPanel = (panel, popupURL, extension) => {
> So we're sharing the panel and browser between all WebExtensions now? There
> might be some side-effects that I don't know enough about (especially once
> we have OOP extensions), so I'll leave this part for Bill to review.

Yeah, that was one of the bigger changes.  I checked with #fx-team, and we _shouldn't_ have more than one panel open at once, so in theory it's safe, but getting someone who knows the code better will be necessary.  :)

> I don't really like how much code this duplicates from the pageAction
> version, though.

I agree, and I'll do a bunch of cleanup if this approach seems reasonable to everyone.

> ::: browser/components/extensions/ext-utils.js:259
> (Diff revisions 6 - 7)
> > +    debugger;
> Nope. :)

Comment 43

[Markus Jaritz [:designakt] (UX)]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #40)
> One of the nice things with this is that we can get rid of a bunch of the
> browserAction code, _and_ we get stuff like
> https://www.dropbox.com/s/rjjqmrqcor9s9up/Screenshot%202015-11-10%2016.53.51.
> png?dl=0 for free.
> 
> So, what do you all think?

Free second level menu panels! Wohoww... This bug is gold! :-)

Comment 44

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22945/diff/7-8/

Comment 45

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

Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Just a WIP.  There are 13 tests failing, but if anyone wanted to give it a try, I'ld love to hear feedback.  (Also, if anyone knows why the tests are failing, I'ld love to hear that, too.  ;)

Comment 46

[Kris Maglione [:kmag]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #45)
> (Also, if anyone knows why the tests are failing, I'ld love to hear that, too.  ;)

Do you have a try run?

Comment 47

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e283c71597b6

Comment 48

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/22945/#review22757

::: browser/components/extensions/ext-browserAction.js:69
(Diff revision 8)
> +      onClick: event => {

Should ignore if button is not 0.

::: browser/components/extensions/ext-browserAction.js:81
(Diff revision 8)
> -
> +          return;

Would rather have an else.

::: browser/components/extensions/ext-pageAction.js:144
(Diff revision 8)
> +      let document = node.ownerDocument;

`window.document.getElementById(...)` would do.

::: browser/components/extensions/ext-utils.js:118
(Diff revision 8)
> -global.openPanel = (node, popupURL, extension) => {
> +global.loadPanel = (panel, popupURL, extension, cb) => {

Please use a full name for `cb` and add a comment about when it's called.

::: browser/components/extensions/ext-utils.js:392
(Diff revision 8)
> -  addOpenListener(listener) {
> +  addOpenListener(listener, fireOnExisting = true) {

Hm. I'm not sure whether this is a ReviewBoard bug or you're intentionally backing out these changes...

::: browser/components/extensions/test/browser/browser_ext_browserAction_simple.js:37
(Diff revision 8)
> -    let evt = new CustomEvent("command", {
> +    let evt = new CustomEvent("click", {});

Needs to be `MouseEvent`. `EventUtils.synthesizeMouseAtCenter` would be better, though. Same for below.

::: browser/components/extensions/test/browser/browser_ext_currentWindow.js:127
(Diff revision 8)
> -    let panel = node.querySelector("panel");
> +    node.dispatchEvent(evt);

Dispatching the same event twice is definitely not kosher...

This mostly looks good, but I've only really had a chance to skim. It does look like you might have sucked in some unrelated changes or accidentally backed out someone else's changeset, though.

Comment 49

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

(In reply to Kris Maglione [:kmag] from comment #48)
> ::: browser/components/extensions/ext-browserAction.js:69
> (Diff revision 8)
> > +      onClick: event => {
> Should ignore if button is not 0.

Fixed.

> ::: browser/components/extensions/ext-browserAction.js:81
> (Diff revision 8)
> > +          return;
> Would rather have an else.

fixed.  (But I used an early-return for the "if (button != 0)" at the top.)

> ::: browser/components/extensions/ext-pageAction.js:144
> (Diff revision 8)
> > +      let document = node.ownerDocument;
> `window.document.getElementById(...)` would do.

Fixed.

> ::: browser/components/extensions/ext-utils.js:118
> (Diff revision 8)
> > -global.openPanel = (node, popupURL, extension) => {
> > +global.loadPanel = (panel, popupURL, extension, cb) => {
> Please use a full name for `cb` and add a comment about when it's called.

Will fix by the next version.  ;)

> ::: browser/components/extensions/ext-utils.js:392
> (Diff revision 8)
> > -  addOpenListener(listener) {
> > +  addOpenListener(listener, fireOnExisting = true) {
> Hm. I'm not sure whether this is a ReviewBoard bug or you're intentionally
> backing out these changes...

Huh.  Unintentionally backing out.  That's distressing…
Fixed.

> :::
> browser/components/extensions/test/browser/browser_ext_browserAction_simple.
> js:37
> (Diff revision 8)
> > -    let evt = new CustomEvent("command", {
> > +    let evt = new CustomEvent("click", {});
> Needs to be `MouseEvent`. `EventUtils.synthesizeMouseAtCenter` would be
> better, though. Same for below.

Fixed.

Say, should I change the rest of the "evt = new MouseEvent("click", {});"s to "synthesizeMouseAtCenter"s?

> :::
> browser/components/extensions/test/browser/browser_ext_currentWindow.js:127
> (Diff revision 8)
> > -    let panel = node.querySelector("panel");
> > +    node.dispatchEvent(evt);
> Dispatching the same event twice is definitely not kosher...

Fixed.

> This mostly looks good, but I've only really had a chance to skim. It does
> look like you might have sucked in some unrelated changes or accidentally
> backed out someone else's changeset, though.

Cool.  Thanks for the feedback!

Comment 50

[Kris Maglione [:kmag]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #49)
> Huh.  Unintentionally backing out.  That's distressing…
> Fixed.

Can you push to try again? I suspect this may have been responsible for some 
of the failures.

> Say, should I change the rest of the "evt = new MouseEvent("click", {});"s
> to "synthesizeMouseAtCenter"s?

Probably. It doesn't matter for a lot of things, but real click events (and 
proper synthetic events) also send mouseup/mousedown/command events, which 
often matter in widget code.

Comment 51

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f47e8a3ce917

Comment 52

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

Ugh.  I think I'm in over my head here.  I still think that the approach of using a "view" button instead of a "custom" button is the right one, but between my lack of knowledge of the CustomizableUI stuff and my lack of knowledge of the WebExtensions stuff, I can't seem to make any headway on the 39 remaining test failures, so I'm going to give this bug up.  :(

(I'll attach the latest version of the patch, in the hopes that it'll help the next person…)

Comment 53

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

Attachment: bug1217129.diff

Comment 54

[Kris Maglione [:kmag]]

Our panel code is pretty crazy. I think even the people who know it well are usually in over their heads.

Thanks for the start. I'll see if I can iron out the wrinkles.

Comment 55

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

Awesome, thanks!  :)
And if you'ld like me to test stuff on Windows 10 or Mac OS X, please let me know!

Comment 56

[Kris Maglione [:kmag]]

Gijs brought up a side-effect of this: When the buttons are in a menu, they'll get a slide-in view rather than a panel. I think the SDK currently cheats, and closes the menu, and re-anchors a new panel in its place. I'd rather stick with the slide-in view for now.

Comment 58

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

Speaking as a UX person, the slide-in view is what we want in that case.  :D

Comment 59

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1217129: Part 1 - Add onBeforeDestroyed and onDestroyed callbacks to CustomizableUI widgets. r?gijs

Review commit: https://reviewboard.mozilla.org/r/29991/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29991/

Comment 60

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1217129: Part 2 - Clear open state of view widgets if view opening is prevented. r=gijs

Review commit: https://reviewboard.mozilla.org/r/29993/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29993/

Comment 61

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1217129: Part 3 - Recalculate CustomizableUI panel arrow position after resize. r?gijs

Review commit: https://reviewboard.mozilla.org/r/29995/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29995/

Comment 62

[Kris Maglione [:kmag]]

This still needs tests, but I think they're going to be a bit fiddly, so I don't want to start on them until I know I'm on the right track.

Comment 63

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

Comment on attachment 8705498 [details]
MozReview Request: Bug 1217129: Part 3 - Recalculate CustomizableUI panel arrow position after resize. r?gijs

I mostly like this, but I'm seeing some oddness that I think needs addressing before I can give it a ui-r+…

When the button is on the left side of the screen, the arrow is in the wrong place, as shown in https://www.dropbox.com/s/7pvr4gep7n0qgrq/Screenshot%202016-01-08%2011.12.38.png?dl=0

Every second time I try to open the panel, it seems to have no content, as shown in https://www.dropbox.com/s/wtxsymlxste99z3/Screenshot%202016-01-08%2011.15.47.png?dl=0

Hopefully there are some easy fixes for these, and I look forward to seeing the next version of the patch!  :D

Comment 64

[:Gijs (he/him)]

Comment on attachment 8705497 [details]
MozReview Request: Bug 1217129: Part 2 - Clear open state of view widgets if view opening is prevented. r=gijs

https://reviewboard.mozilla.org/r/29993/#review26905

Comment 65

[:Gijs (he/him)]

Comment on attachment 8705496 [details]
MozReview Request: Bug 1217129: Part 1 - Add onBeforeDestroyed and onDestroyed callbacks to CustomizableUI widgets. r?gijs

https://reviewboard.mozilla.org/r/29991/#review26909

::: browser/components/customizableui/CustomizableUI.jsm:2429
(Diff revision 1)
> +        if (widget.onBeforeDestroyed) {
> +          widget.onBeforeDestroyed(widgetNode);
> +        }

This should be fired after onWidgetAfterDOMChange for sanity, I think?

Why do you need this, though? It isn't used in your third patch, it seems?

::: browser/components/customizableui/CustomizableUI.jsm:2447
(Diff revision 1)
> +      if (widgetNode && widget.onDestroyed) {
> +        widget.onDestroyed(window.document);
> +      }

You don't really need this, either, as there's already an onWidgetDestroyed listener that we fire.

```
this.listener = {
  onWidgetDestroyed: widgetId => {
    if (this.id == widgetId) {
      for (let win of CustomizableUI.windows) {
        let view = win.document.getElementById(this.viewId);
        if (view) {
          view.remove();
        }
      }
      CustomizableUI.removeListener(this.listener);
    }
  }
}
CustomizableUI.addListener(this.listener);
```

I mean, that or use a single "delegate" style listener that kills off the views for every BrowserAction-related widget that gets destroyed. Either way, adding another callback on the widget itself doesn't really seem warranted to me. Feel free to argue with me if you disagree, though! :-)

Comment 66

[:Gijs (he/him)]

Comment on attachment 8705498 [details]
MozReview Request: Bug 1217129: Part 3 - Recalculate CustomizableUI panel arrow position after resize. r?gijs

https://reviewboard.mozilla.org/r/29995/#review26911

I don't think I should really review the web-extensions internal refactoring of the basepopup/openpopup/panelpopup things. But the use of CUI looks reasonable, though I think there's a few things we should change in CUI rather than workaround here. So f+, but probably not r+ right now? :-)

Would you be OK with having someone else review the webextensions bits or would you prefer I dive in? I can do that, but it will take me longer.

::: browser/components/extensions/ext-browserAction.js:64
(Diff revision 1)
> +      label: this.defaults.title || " ",
> +      tooltiptext: this.defaults.title || " ",

While we're here... we shouldn't have the tooltip be the same as the label.

What's the point of a button with no label? TBH, I think that property should be required, but I suppose that's not possible for some reason? Can we know the name of the add-on and fall back on that?

We should probably add support to CUI to allow not having a tooltiptext at all - it seems right now we make buttons always have one, and that's wrong.

As a workaround, in onCreated you could remove the attribute, but that's a little hacky. We should just support having empty string mean "no tooltip".

... I suppose you're already doing that, kind of. IMO we should support it properly, though. Not having a label is an issue, not having a tooltiptext shouldn't be.

::: browser/components/extensions/ext-browserAction.js:106
(Diff revision 1)
> +          // This isn't not a hack, but it seems to provide the correct behavior
> +          // with the fewest complications.
> +          event.preventDefault();
> -            this.emit("click");
> +          this.emit("click");

Why would popup be empty, and why doesn't emitting a click on the button re-enter? Or does that just mean you're notifying listeners rather than actually creating a click event? I don't see the implementation anywhere in your patch, so I have no idea.

::: browser/components/extensions/ext-browserAction.js:126
(Diff revision 1)
>        node.setAttribute("aria-label", tabData.title);

I don't think you need an aria-label when there is a label attribute.

::: browser/components/extensions/ext-utils.js:249
(Diff revision 1)
> -      browser.setAttribute("width", width);
> -      browser.setAttribute("height", height);
> +    // CustomizeableUI panel views do not resize correctly unless we set both
> +    // the attribute and the CSS properties.

Do you know why, and could we fix that instead of hacking around it?

Comment 67

[Kris Maglione [:kmag]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #63)
> When the button is on the left side of the screen, the arrow is in the wrong
> place, as shown in
> https://www.dropbox.com/s/7pvr4gep7n0qgrq/Screenshot%202016-01-08%2011.12.38.
> png?dl=0

Ah. Indeed. I think we need to have CUI adjust the position of the anchor
after we resize, since we don't know the final width as soon as it opens. It
currently only adjusts for height changes.

> Every second time I try to open the panel, it seems to have no content, as
> shown in
> https://www.dropbox.com/s/wtxsymlxste99z3/Screenshot%202016-01-08%2011.15.47.
> png?dl=0
> 
> Hopefully there are some easy fixes for these, and I look forward to seeing
> the next version of the patch!  :D

Intereting. This isn't happening for me. Can you attach your test code?

Comment 68

[Kris Maglione [:kmag]]

(In reply to :Gijs Kruitbosch from comment #65)
> Comment on attachment 8705496 [details]
> MozReview Request: Bug 1217129: Part 1 - Add onBeforeDestroyed and
> onDestroyed callbacks to CustomizableUI widgets. r?gijs
> 
> https://reviewboard.mozilla.org/r/29991/#review26909
> 
> ::: browser/components/customizableui/CustomizableUI.jsm:2429
> (Diff revision 1)
> > +        if (widget.onBeforeDestroyed) {
> > +          widget.onBeforeDestroyed(widgetNode);
> > +        }
> 
> This should be fired after onWidgetAfterDOMChange for sanity, I think?

I'm not sure. I think I'd expect it to fire before the widget has been removed
from the document.

> Why do you need this, though? It isn't used in your third patch, it seems?

I added it mainly for symmetry with onBeforeCreated/onCreated. I'd initially
planned to use this in my patch rather than onDestroyed, but eventually
decided the latter made more sense.

> ::: browser/components/customizableui/CustomizableUI.jsm:2447
> (Diff revision 1)
> > +      if (widgetNode && widget.onDestroyed) {
> > +        widget.onDestroyed(window.document);
> > +      }
> 
> You don't really need this, either, as there's already an onWidgetDestroyed
> listener that we fire.
>...
> I mean, that or use a single "delegate" style listener that kills off the
> views for every BrowserAction-related widget that gets destroyed. Either
> way, adding another callback on the widget itself doesn't really seem
> warranted to me. Feel free to argue with me if you disagree, though! :-)

I thought about using an onWidgetDestroyed listener at first, but it seemed to
leave too many chances for things to get out of sync. It would mean moving my
cleanup code farther away from my creation code, for one thing, which makes
mistakes easier to miss. It would also mean iterating over windows myself,
when CUI already knows which windows to iterate over better than I do, and is
already doing it.

It also just seemed to make sense to have a destroy listener to match the
create listener. If the main purpose of the onBeforeCreated listener is to
create nodes in a document the widget is about to be added to, everyone who
uses it is also going to have to remove those nodes, so it makes sense to have
them all do it the same way (and it doesn't hurt to have the method there to
remind them they have to do it).

Comment 69

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

(In reply to Kris Maglione [:kmag] from comment #67)
> (In reply to Blake Winton (:bwinton) (:☕️) from comment #63)
> > Every second time I try to open the panel, it seems to have no content, as
> > shown in
> > https://www.dropbox.com/s/wtxsymlxste99z3/Screenshot%202016-01-08%2011.15.47.
> > png?dl=0
> Intereting. This isn't happening for me. Can you attach your test code?

https://github.com/bwinton/whimsy/blob/ui-playground/manifest.json#L41
and
https://github.com/bwinton/whimsy/blob/ui-playground/html/catgifs.html
(well, that whole repo, but I think those are the two relevant files…  ;)

Comment 70

[Kris Maglione [:kmag]]

(In reply to :Gijs Kruitbosch from comment #66)
> I don't think I should really review the web-extensions internal refactoring
> of the basepopup/openpopup/panelpopup things. But the use of CUI looks
> reasonable, though I think there's a few things we should change in CUI
> rather than workaround here. So f+, but probably not r+ right now? :-)
> 
> Would you be OK with having someone else review the webextensions bits or
> would you prefer I dive in? I can do that, but it will take me longer.

Honestly, I don't think there's anyone better qualified at this point. Bill is
the only other person who's really worked on this code, and I think that your
front-end knowledge is much better than either of ours, so I'd appreciate it
if you can look into it.

I've also been thinking that it might make sense to move the location bar icon
logic into CUI, which you might have some thoughts on.

> ::: browser/components/extensions/ext-browserAction.js:64
> (Diff revision 1)
> > +      label: this.defaults.title || " ",
> > +      tooltiptext: this.defaults.title || " ",
> 
> While we're here... we shouldn't have the tooltip be the same as the label.
> 
> What's the point of a button with no label? TBH, I think that property
> should be required, but I suppose that's not possible for some reason? Can
> we know the name of the add-on and fall back on that?

Yeah, that occurred to me too, but I decided to save it for a follow-up. I
think the label should probably always be the add-on name, regardless of the
title attribute, but it requires some investigation.

> We should probably add support to CUI to allow not having a tooltiptext at
> all - it seems right now we make buttons always have one, and that's wrong.
> 
> As a workaround, in onCreated you could remove the attribute, but that's a
> little hacky. We should just support having empty string mean "no tooltip".

I was mainly trying to avoid the console errors when it failed to look up the
localized string:

console.error: 
  [CustomizableUI]
  Could not localize property 'click-test_kmaglione_mozilla_com-browser-action.label'.
console.error: 
  [CustomizableUI]
  Could not localize property 'click-test_kmaglione_mozilla_com-browser-action.tooltiptext'.

I agree this isn't a great solution, though. Maybe an empty string should be
treated differently from other falsy values, and not fall back to locale
strings.

> ::: browser/components/extensions/ext-browserAction.js:106
> (Diff revision 1)
> > +          // This isn't not a hack, but it seems to provide the correct behavior
> > +          // with the fewest complications.
> > +          event.preventDefault();
> > -            this.emit("click");
> > +          this.emit("click");
> 
> Why would popup be empty, and why doesn't emitting a click on the button
> re-enter? Or does that just mean you're notifying listeners rather than
> actually creating a click event? I don't see the implementation anywhere in
> your patch, so I have no idea.

The "popup" property is a URL for the popup. It's not a great name, but that's
what the defines.

If there's a popup URL, clicking the button shows a popup, but doesn't send a
click event. If there isn't, it sends a click event, but doesn't show a popup.

This isn't a DOM event, it's an internal event that eventually gets routed to
a WebExtension API event, so it doesn't have any impact on the button node.

> ::: browser/components/extensions/ext-browserAction.js:126
> (Diff revision 1)
> >        node.setAttribute("aria-label", tabData.title);
> 
> I don't think you need an aria-label when there is a label attribute.

I didn't think so either, but I'm pretty sure you're the one who suggested we
add it.

> ::: browser/components/extensions/ext-utils.js:249
> (Diff revision 1)
> > -      browser.setAttribute("width", width);
> > -      browser.setAttribute("height", height);
> > +    // CustomizeableUI panel views do not resize correctly unless we set both
> > +    // the attribute and the CSS properties.
> 
> Do you know why, and could we fix that instead of hacking around it?

I don't. There seems to be some kind of layout black magic involved. I spent a
couple of hours trying to figure it out and eventually gave up.

Comment 71

[Kris Maglione [:kmag]]

Oh, right, one of these days I'll remember I'm supposed to reply to reviews in reviewboard...

Comment 72

[Kris Maglione [:kmag]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #69)
> https://github.com/bwinton/whimsy/blob/ui-playground/manifest.json#L41
> and
> https://github.com/bwinton/whimsy/blob/ui-playground/html/catgifs.html
> (well, that whole repo, but I think those are the two relevant files…  ;)

Odd. That works for me consistently. Might be a Mac-specific problem. I'll test later.

Comment 73

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

(In reply to Kris Maglione [:kmag] from comment #70)
> Honestly, I don't think there's anyone better qualified at this point. Bill
> is
> the only other person who's really worked on this code, and I think that your
> front-end knowledge is much better than either of ours, so I'd appreciate it
> if you can look into it.

I hope that the WebExtension code will come to be seen as just another part of the frontend, so it would be great for more frontend developers to start understanding and reviewing the code. It's probably more work for you in the short-term, Gijs, but I think it's good for Mozilla if all our code looks and feels the same.

Comment 74

[Kris Maglione [:kmag]]

(In reply to :Gijs Kruitbosch from comment #66)
> ::: browser/components/extensions/ext-utils.js:249
> (Diff revision 1)
> > -      browser.setAttribute("width", width);
> > -      browser.setAttribute("height", height);
> > +    // CustomizeableUI panel views do not resize correctly unless we set both
> > +    // the attribute and the CSS properties.
> 
> Do you know why, and could we fix that instead of hacking around it?

I looked into this a bit more. I couldn't reproduce the problem at first, and
then I realized it only happened with "noautohide" set, which I was using for
testing. In that case, the attributes seem to take precedence over the style
properties when set here.

It's extremely flaky and timing-sensitive, though. If I try setting the same
properties in the DOM inspector, rather than in my code, they don't have any
effect. It also depends on me giving the browser initial width and height
properties before the popup is shown (if I don't, the internal popup code adds
width and height attributes to the panel).

I'm going to dig into this a bit more, since Blake's still seeing inconsistent
behavior, and I still don't know what's going on.

Comment 75

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #74)
> (In reply to :Gijs Kruitbosch from comment #66)
> > ::: browser/components/extensions/ext-utils.js:249
> > (Diff revision 1)
> > > -      browser.setAttribute("width", width);
> > > -      browser.setAttribute("height", height);
> > > +    // CustomizeableUI panel views do not resize correctly unless we set both
> > > +    // the attribute and the CSS properties.
> > 
> > Do you know why, and could we fix that instead of hacking around it?
> 
> I looked into this a bit more. I couldn't reproduce the problem at first, and
> then I realized it only happened with "noautohide" set, which I was using for
> testing. In that case, the attributes seem to take precedence over the style
> properties when set here.
> 
> It's extremely flaky and timing-sensitive, though. If I try setting the same
> properties in the DOM inspector, rather than in my code, they don't have any
> effect. It also depends on me giving the browser initial width and height
> properties before the popup is shown (if I don't, the internal popup code
> adds
> width and height attributes to the panel).
> 
> I'm going to dig into this a bit more, since Blake's still seeing
> inconsistent
> behavior, and I still don't know what's going on.

Maybe you'd realized this already, but in case it's helpful:
0) pretty sure the XUL internal code relies on the attributes for certain things, and they might have precedence there over style (but they might not... I just vaguely recall something funny there)
1) we transition arrowpanel popups everywhere but on Linux (because Linux), and then
2) we mess with panel sizing in panel.xml (in browser/components/customizableui) and also listen for the end of those transitions to do some of that. For some funkiness we've already seen here, see bug 1088637 and (reading the symptoms Blake described, perhaps especially:) bug 1029937.

Comment 76

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #70)
> (In reply to :Gijs Kruitbosch from comment #66)
> > I don't think I should really review the web-extensions internal refactoring
> > of the basepopup/openpopup/panelpopup things. But the use of CUI looks
> > reasonable, though I think there's a few things we should change in CUI
> > rather than workaround here. So f+, but probably not r+ right now? :-)
> > 
> > Would you be OK with having someone else review the webextensions bits or
> > would you prefer I dive in? I can do that, but it will take me longer.
> 
> Honestly, I don't think there's anyone better qualified at this point. Bill
> is
> the only other person who's really worked on this code, and I think that your
> front-end knowledge is much better than either of ours, so I'd appreciate it
> if you can look into it.

OK. I will take more time to review the next pass and get to know my way around the rest of the webextensions code a little better.

> I've also been thinking that it might make sense to move the location bar
> icon
> logic into CUI, which you might have some thoughts on.

Yeeeaaaahh... in web extensions, can the same buttons be used in the url bar and outside of it? Basically, CUI has a set of areas and a set of widgets, and in principle all widgets can go to any area. That doesn't play so well with the in-URL bar icons...

Taking a step back, why did you want to do this? :-)

> > ::: browser/components/extensions/ext-browserAction.js:64
> > (Diff revision 1)
> > > +      label: this.defaults.title || " ",
> > > +      tooltiptext: this.defaults.title || " ",
> > 
> > While we're here... we shouldn't have the tooltip be the same as the label.
> > 
> > What's the point of a button with no label? TBH, I think that property
> > should be required, but I suppose that's not possible for some reason? Can
> > we know the name of the add-on and fall back on that?
> 
> Yeah, that occurred to me too, but I decided to save it for a follow-up. I
> think the label should probably always be the add-on name, regardless of the
> title attribute, but it requires some investigation.

Fair.

> > We should probably add support to CUI to allow not having a tooltiptext at
> > all - it seems right now we make buttons always have one, and that's wrong.
> > 
> > As a workaround, in onCreated you could remove the attribute, but that's a
> > little hacky. We should just support having empty string mean "no tooltip".
> 
> I was mainly trying to avoid the console errors when it failed to look up the
> localized string

Yes. I did figure that was why you were doing this - sorry for not making it explicit.

> I agree this isn't a great solution, though. Maybe an empty string should be
> treated differently from other falsy values, and not fall back to locale
> strings.

Yup, that's what I figured, too.

> > ::: browser/components/extensions/ext-browserAction.js:126
> > (Diff revision 1)
> > >        node.setAttribute("aria-label", tabData.title);
> > 
> > I don't think you need an aria-label when there is a label attribute.
> 
> I didn't think so either, but I'm pretty sure you're the one who suggested we
> add it.

Well, that's embarrassing.

Reading the bug (bug 1197422) again... this is because of bug 1123760... is this code used for in-urlbar-buttons ? That's somewhat surprising to me. But the problem in bug 1123760 occurs at least in part because the buttons themselves have no label attribute - only a tooltiptext. I *think* that it might be OK when they do have a label. I can test that later this week.

Comment 77

[Kris Maglione [:kmag]]

(In reply to :Gijs Kruitbosch from comment #75)
> Maybe you'd realized this already, but in case it's helpful:
> 0) pretty sure the XUL internal code relies on the attributes for certain
> things, and they might have precedence there over style (but they might
> not... I just vaguely recall something funny there)

Yeah, the attributes were definitely causing most of the weird behavior. I
eventually figured out the two main issues:

1) If the panel doesn't start out large enough to accommodate the arrow and
other decorations, the popup manager sees a resize event, notices that the
panel is larger than the layout code thinks it should be, assumes the OS has
resized it, and adds width and height attributes to the panel, to respect what
it assumes is the user-chosen size.

That logic was apparently added to allow for popups resizable with titlebars
and normal window decorations, so it probably shouldn't apply to non-resizable
popups like ours. But I really don't want to block this on fixing a layout bug
like that, so my short term solution is to create the popup with a
reasonably-sized browser that doesn't trigger that behavior.

2) Setting the width and height attributes of the browser before the popup is
shown always results in the correct size. Changing the attributes after it's
shown doesn't correctly update the size in the case of the PanelUI popup. They
also have strange interactions with the CSS properties. The CSS width property
seems to override the width attribute, but the height attribute seems to
override the CSS height property.

Leaving out the attributes altogether and sticking entirely to CSS sizing
seems to work, as long as I don't trigger the panel sizing issue.

> 1) we transition arrowpanel popups everywhere but on Linux (because Linux),
> and then

Yeah, I noticed that. I've run into themes that turn on transitions for popups
on Linux and break everything, so I'm not surprised.

I don't think that was causing the issues I was seeing, but I did try testing
with all of the transition code/CSS disabled, even though it look like it was
being used.

> 2) we mess with panel sizing in panel.xml (in
> browser/components/customizableui) and also listen for the end of those
> transitions to do some of that. For some funkiness we've already seen here,
> see bug 1088637 and (reading the symptoms Blake described, perhaps
> especially:) bug 1029937.

I did notice that code, but I had a hard time following it. It looked like
most of it wasn't used in popups without subviews, so I figured it wasn't
related. Thanks for the context, though. I'll double check to make sure we're
handling that correctly.

Comment 78

[Kris Maglione [:kmag]]

(In reply to :Gijs Kruitbosch from comment #76)
> 
> OK. I will take more time to review the next pass and get to know my way
> around the rest of the webextensions code a little better.

Thanks.

> > I've also been thinking that it might make sense to move the location bar
> > icon
> > logic into CUI, which you might have some thoughts on.
> 
> Yeeeaaaahh... in web extensions, can the same buttons be used in the url bar
> and outside of it? Basically, CUI has a set of areas and a set of widgets,
> and in principle all widgets can go to any area. That doesn't play so well
> with the in-URL bar icons...

Yeah, the more I think about it, the more I think you're right. *Technically*,
there's no reason the urlbar buttons couldn't be used elsewhere, and I think
that would be the easiest way to let users remove urlbar buttons they don't
want, but I'm also not sure it's a good idea.

> Taking a step back, why did you want to do this? :-)

Well, for a few reasons.

Mainly, it feels strange to use a completely different implementation for page
action and browser action buttons that do essentially the same thing.
Especially in the case of the panel code panel code, where they currently look
and behave differently, but ideally should be the same.

Maybe the better solution is a separate module with a similar (but simpler)
interface, and ideally using as much of the same PanelUI code as possible.

I could try to mimic the behavior and styling of the panels in WebExtension
code, or somehow try to reuse the panel logic from there, but that seems
certain to cause trouble down the road. And it would be nice for other urlbar
button code to be able to build on the same implementation.

> > > ::: browser/components/extensions/ext-browserAction.js:64
> > > (Diff revision 1)
> > > > +      label: this.defaults.title || " ",
> > > > +      tooltiptext: this.defaults.title || " ",
> > > 
> > > While we're here... we shouldn't have the tooltip be the same as the label.
> > > 
> > > What's the point of a button with no label? TBH, I think that property
> > > should be required, but I suppose that's not possible for some reason? Can
> > > we know the name of the add-on and fall back on that?
> > 
> > Yeah, that occurred to me too, but I decided to save it for a follow-up. I
> > think the label should probably always be the add-on name, regardless of the
> > title attribute, but it requires some investigation.
> 
> Fair.

I thought about this a bit more and came to the conclusion that we should
probably default to the extension name for both the label and the tooltip.
Chrome doesn't really have a concept of labels for toolbar buttons, so the
title attribute just sets the tooltip.

I think, in the case of the label shown in the main menu, the title is also
the property that makes the most sense. The extension name might not always be
relevant, and we don't currently have a separate API to set the label.

It might be a good idea to add a label property in the future, but for now I
think I'm going to stick with the title (defaulting to the add-on name) for
both.

> > I agree this isn't a great solution, though. Maybe an empty string should be
> > treated differently from other falsy values, and not fall back to locale
> > strings.
> 
> Yup, that's what I figured, too.

I wrote a patch for this behavior, but I don't actually need it if we're going
to be defaulting to the add-on name instead. Do you still want it?
 
> Reading the bug (bug 1197422) again... this is because of bug 1123760... is
> this code used for in-urlbar-buttons ? That's somewhat surprising to me. But
> the problem in bug 1123760 occurs at least in part because the buttons
> themselves have no label attribute - only a tooltiptext. I *think* that it
> might be OK when they do have a label. I can test that later this week.

Ah, you're right. This got added to the toolbar button code when I updated it
to match the implementation of the urlbar button code. I guess it can go,
then.

Comment 79

[Kris Maglione [:kmag]]

Blake, can you test again with this build when you get a chance? http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-3083106c1a4632dc883061ddb3a920f48ef87c1b/try-macosx64/

I did test on OS-X, but I couldn't reproduce your intermittent panel sizing problem in the first place, so I don't know if it's fixed.

Comment 80

[Kris Maglione [:kmag]]

Comment on attachment 8705496 [details]
MozReview Request: Bug 1217129: Part 1 - Add onBeforeDestroyed and onDestroyed callbacks to CustomizableUI widgets. r?gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29991/diff/1-2/

Comment 81

[Kris Maglione [:kmag]]

Comment on attachment 8705497 [details]
MozReview Request: Bug 1217129: Part 2 - Clear open state of view widgets if view opening is prevented. r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29993/diff/1-2/

Comment 82

[Kris Maglione [:kmag]]

Comment on attachment 8705498 [details]
MozReview Request: Bug 1217129: Part 3 - Recalculate CustomizableUI panel arrow position after resize. r?gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29995/diff/1-2/

Comment 83

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1217129: Part 4a - Allow creating CustomizableUI widgets without tooltips. r?gijs

Review commit: https://reviewboard.mozilla.org/r/30379/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30379/

Comment 84

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1217129: Part 4b - [webext] Use the extension name as the default title value for browserAction/pageAction popups. r?billm

This brings our behavior closer in line with Chrome's.

Review commit: https://reviewboard.mozilla.org/r/30381/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30381/

Comment 85

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1217129: Part 5 - [webext] Use CustomizableUI views for BrowserAction popups. r?gijs ui-r?bwinton

This version addresses some popup sizing bugs, and also a few other issues I
ran into when debugging Blake's problems:

 * The standalone popup needs a max width of 800px for Chrome compatibility,
   which is wider than our default max width.

 * I added a flex attribute to our browser so that it fills the entire space
   of the slide-in panel. This is only necessary for browsers with content
   that is shorter than the height of the panel when it gets its desired
   width, but becomes longer when it doesn't, so it didn't show up in my
   initial tests.

 * I also added an extra pixel to the width calculations, since I noticed that
   a lot of single lines of text were unexpectedly wrapping without it. I'll
   look into this more in a follow-up bug.

I also added some comments, and renamed a couple of variables, where things
seemed unclear.

The test changes are mostly just updates to older browser action tests to use
newer helpers, rather than ad-hoc events, to open/close/click the widgets. A
few tests also needed updates to explicitly close the panel when they were
done with it.

Review commit: https://reviewboard.mozilla.org/r/30383/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30383/

Comment 86

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1217129: Part 6 - [webext] Test browserAction popups in both the toolbar and in the menu panel. r?gijs r?billm

r? Bill for the test refactoring, Gijs for the PanelUI interaction.

Review commit: https://reviewboard.mozilla.org/r/30385/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30385/

Comment 87

[Kris Maglione [:kmag]]

I'm still planning to add popup sizing tests, but I might save that for a follow-up bug, since we have a few other bug specifically about popup sizing. I think everything else is covered now.

Comment 88

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

(In reply to Kris Maglione [:kmag] from comment #79)
> Blake, can you test again with this build when you get a chance?
> http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 3083106c1a4632dc883061ddb3a920f48ef87c1b/try-macosx64/
> 
> I did test on OS-X, but I couldn't reproduce your intermittent panel sizing
> problem in the first place, so I don't know if it's fixed.

Looks awesome!  ui-r=me!  :D

Comment 89

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

Just FYI, I'm afraid I won't be able to get to this until Wednesday.

Comment 90

[:Gijs (he/him)]

Comment on attachment 8705496 [details]
MozReview Request: Bug 1217129: Part 1 - Add onBeforeDestroyed and onDestroyed callbacks to CustomizableUI widgets. r?gijs

https://reviewboard.mozilla.org/r/29991/#review27237

Don't forget to also update MDN (there still doesn't seem to be a way to get jsdoc-style things onto MDN automagically, which is frustrating).

Comment 91

[:Gijs (he/him)]

Comment on attachment 8706563 [details]
MozReview Request: Bug 1217129: Part 4a - Allow creating CustomizableUI widgets without tooltips. r?gijs

https://reviewboard.mozilla.org/r/30379/#review27239

::: browser/components/customizableui/CustomizableUI.jsm:1332
(Diff revision 1)
>        node.setAttribute("label", this.getLocalizedProperty(aWidget, "label"));

We should ensure that this still throws an error if you pass "" for the label.

Comment 92

[:Gijs (he/him)]

Comment on attachment 8706565 [details]
MozReview Request: Bug 1217129: Part 5 - [webext] Use CustomizableUI views for BrowserAction popups. r?gijs ui-r?bwinton

https://reviewboard.mozilla.org/r/30383/#review27245

Almost, but I still have questions.

::: browser/components/extensions/ext-browserAction.js:29
(Diff revision 1)
> -  this.id = makeWidgetId(extension.id) + "-browser-action";
> +  this.id = `${makeWidgetId(extension.id)}-browser-action`;
> +  this.viewId = `PanelUI-webext-${makeWidgetId(extension.id)}-browser-action-view`;

Should we avoid calling makeWidgetId twice here?

::: browser/components/extensions/ext-browserAction.js:127
(Diff revision 1)
>      let title = tabData.title || this.extension.name;
>      node.setAttribute("tooltiptext", title);
>      node.setAttribute("label", title);

This seems unnecessary the first time the node is created, and this overwrites the empty tooltip fallback that we now use further up.

::: browser/components/extensions/ext-utils.js:135
(Diff revision 1)
> -  let popupURI = Services.io.newURI(popupURL, null, extension.baseURI);
> +    let popupURI = Services.io.newURI(popupURL, null, extension.baseURI);

This can throw if the add-on provides an invalid URL. What happens in this case, and is that the right thing? Is there a test for what happens in this case?

::: browser/components/extensions/ext-utils.js:156
(Diff revision 1)
> -  const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +  destroy() {

I wonder if this should null out things so we don't accidentally leak and/or try to reuse this object.

::: browser/components/extensions/ext-utils.js:171
(Diff revision 1)
> +      case this.DESTROY_EVENT:

This is undefined for the base popup. I guess event.type should never be undefined, so behaviourally that shouldn't matter, but it's an interesting weirdness here. Can you add a comment?

::: browser/components/extensions/ext-utils.js:210
(Diff revision 1)
> +    // will be if and when we popup debugging.

"if and when we popup debugging"

? :-)

If you mean bug 950936, that's looking to land pretty soon.

::: browser/components/extensions/ext-utils.js:294
(Diff revision 1)
> +      panel.openPopup(button, "bottomcenter topright", 0, 0, false, false);

This should anchor on the icon inside the button.

::: browser/components/extensions/test/browser/browser_ext_currentWindow.js:119
(Diff revision 1)
> -    let panel = node.querySelector("panel");
> +    let panel = getBrowserActionPopup(extension, win);
>      if (panel) {
>        panel.hidePopup();
>      }
>    }

Shouldn't/Couldn't this be closeBrowserActionPopup(extension, win); ?

::: browser/components/extensions/test/browser/browser_ext_getViews.js:125
(Diff revision 1)
> -    let panel = node.querySelector("panel");
> +    let panel = getBrowserActionPopup(extension, win);
>      if (panel) {
>        panel.hidePopup();
>      }

Ditto.

::: browser/components/extensions/test/browser/browser_ext_getViews.js:131
(Diff revision 1)
> +  // The popup occasionally closes prematurely if we open it immediately here.
> +  // I'm not sure what causes it to close (it's something internal, and seems to
> +  // be focus-related, but it's not caused by JS calling hidePopup), but even a
> +  // short timeout seems to consistently fix it.
> +  yield new Promise(resolve => win1.setTimeout(resolve, 10));

Pretty sure this has occasionally hit us in Linux tests. Do you see this on other OSes? Can you file a followup bug with more details inasmuch as you have them? We should try to get to the bottom of this at some point. :-\

Comment 93

[:Gijs (he/him)]

Comment on attachment 8705498 [details]
MozReview Request: Bug 1217129: Part 3 - Recalculate CustomizableUI panel arrow position after resize. r?gijs

https://reviewboard.mozilla.org/r/29995/#review27257

So... I don't really know what this does. Looks OK. But presumably this affects other panels as well. Can you elaborate on exactly what this fixes? There might be other bugs on file about this issue, but at the moment it's hard for me to decide if that is or isn't the case. :-)

::: browser/components/customizableui/content/panelUI.xml:382
(Diff revision 2)
>            if (!this.ignoreMutations && !this.showingSubView && !this._transitioning) {
>              let height;
>              if (this.showingSubViewAsMainView) {
>                height = this._heightOfSubview(this._mainView);
>              } else {
>                height = this._mainView.scrollHeight;
>              }
>              this._viewContainer.style.height = height + "px";
>            }

For clarity, can you factor this out into its own method as well, and use the same kind of positive check as for shouldSetPosition() ?

Comment 94

[:Gijs (he/him)]

Comment on attachment 8706566 [details]
MozReview Request: Bug 1217129: Part 6 - [webext] Test browserAction popups in both the toolbar and in the menu panel. r?gijs r?billm

https://reviewboard.mozilla.org/r/30385/#review27259

The limited bits you asked me to review here look OK to me.

::: browser/components/extensions/test/browser/.eslintrc:13
(Diff revision 1)
> +    // Browser window globals.
> +    "PanelUI": false,
> +

Hm, this should really go into browser.eslintrc, right?

Comment 95

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/30385/#review27367

Comment 96

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/30383/#review27245

> Should we avoid calling makeWidgetId twice here?

Probably. It's cheap enough, but storing it in a variable would at least make it easier to read.

> This seems unnecessary the first time the node is created, and this overwrites the empty tooltip fallback that we now use further up.

I agree that it's unnecessary, but I don't really have a better solution. The rest of the changes this function makes are necessary, so we still have to call it. Adding a special case to skip these attributes the first time doesn't seem worth it, and I can't skip passing the values to `createWidget`.

Although, if it comes to it, it's not entirely unnecessary. There will be cases where the default name changes between the time we create the widget and the time we create a node for a specific window, e.g., when the default add-on changes the default name and then we open a new browser window.

It doesn't eliminate the fallback, though. It now falls back to the extension name in all cases.

> This can throw if the add-on provides an invalid URL. What happens in this case, and is that the right thing? Is there a test for what happens in this case?

I don't think it ever throws when there's a base URI to resolve relative to. I can't think of any string that would cause it to, anyway. There are currently tests for URLs that throw a security error in the next line, though.

> I wonder if this should null out things so we don't accidentally leak and/or try to reuse this object.

Yeah, I think it should.

> This is undefined for the base popup. I guess event.type should never be undefined, so behaviourally that shouldn't matter, but it's an interesting weirdness here. Can you add a comment?

Yeah. I'll add a getter to the base class with a doc comment. It should probably throw a not implemented error too.

> "if and when we popup debugging"
> 
> ? :-)
> 
> If you mean bug 950936, that's looking to land pretty soon.

Yuuuup... That's unexpectedly good news. I'll make a note to check this against that when it lands.

> This should anchor on the icon inside the button.

There's no button here, just an icon. This is only used for urlbar popups. I guess that was a badly-chosen argument name.

> Shouldn't/Couldn't this be closeBrowserActionPopup(extension, win); ?

Yeah. I think I wound up changing one or both of these in the next commit.

> Pretty sure this has occasionally hit us in Linux tests. Do you see this on other OSes? Can you file a followup bug with more details inasmuch as you have them? We should try to get to the bottom of this at some point. :-\

I was seeing it on Linux, yeah. I just tried it on a Mac and couldn't reproduce it, but that Mac is much slower than my development machine, and this feels like a race, so that may not mean anything.

I'll try to look into it some more and see if I can get more details. Right now, I don't think I even have enough to file a bug.

It does only seem to happen the first time the test opens the popup. If I use --run-until-failure, and it doesn't hit immediately, then it never does.

Comment 97

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/30379/#review27239

> We should ensure that this still throws an error if you pass "" for the label.

Hm. Yeah. I was assuming that would be checked by `normalizeWidget`, but it can't, since it doesn't know about localized properties.

Comment 98

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/29995/#review27257

When the toolbar button is placed on the left side of the screen, and the panel is opened, and then eventually resized, it grows to the right, since there isn't room to grow to the left. At that point, the arrow is still positioned relative to the right of the panel, though, so it no longer points to the anchor.

This is the issue that Blake reported.

I suspect this may affect some other panels, but probably not very many. It should only be an issue for panels which change size after they're shown, and rely on automatic sizing to their contents rather than `panel.sizeTo`, which already calls `adjustArrowPosition`.

I guess, ideally, all arrow panels should automatically do this when they're resized, but I think that may depend on the layout engine emitting some more useful events. The PanelUI popups already have mutation observers for size changes, so it seems to make sense to do the calculation there. I don't know if it would make sense to do the same for arrow panels in general...

> For clarity, can you factor this out into its own method as well, and use the same kind of positive check as for shouldSetPosition() ?

Yeah, makes sense.

Comment 99

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/29991/#review27237

Yup, I was already planning on it.

Comment 100

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

Comment on attachment 8706564 [details]
MozReview Request: Bug 1217129: Part 4b - [webext] Use the extension name as the default title value for browserAction/pageAction popups. r?billm

https://reviewboard.mozilla.org/r/30381/#review27925

Sorry for the delay. I'll be honest, I didn't look at the test changes much. It seems like mostly code movement? In theory Reviewboard should help here, but it was pretty hard to follow still.

Comment 101

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

Comment on attachment 8706566 [details]
MozReview Request: Bug 1217129: Part 6 - [webext] Test browserAction popups in both the toolbar and in the menu panel. r?gijs r?billm

https://reviewboard.mozilla.org/r/30385/#review27927

Comment 102

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #100)
> Comment on attachment 8706564 [details]
> MozReview Request: Bug 1217129: Part 4b - [webext] Use the extension name as
> the default title value for browserAction/pageAction popups. r?billm
> 
> https://reviewboard.mozilla.org/r/30381/#review27925
> 
> Sorry for the delay. I'll be honest, I didn't look at the test changes much.
> It seems like mostly code movement? In theory Reviewboard should help here,
> but it was pretty hard to follow still.

Yeah, it was nearly all code movement to allow running a second set of tests.
I was hoping reviewboard would do a better job of showing that. It wound up
doing completely different things for the two test files with pretty much the
same set of changes.

Thanks everyone for the reviews.

Comment 103

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/93266f211716db6c1b5782e6e8d5803baaae1d27
Bug 1217129: Part 1 - Add onDestroyed callback to CustomizableUI widgets. r=gijs

https://hg.mozilla.org/integration/fx-team/rev/3b5fb2845c3bb9a03bab0561fd9e0624ffbca424
Bug 1217129: Part 2 - Clear open state of view widgets if view opening is prevented. r=gijs

https://hg.mozilla.org/integration/fx-team/rev/7d8dee4c335baa4dbf663a00fe020b9cfccdd9c9
Bug 1217129: Part 3 - Recalculate CustomizableUI panel arrow position after resize. r=gijs

https://hg.mozilla.org/integration/fx-team/rev/71f46fe62f59385d6a39d426fd571ed08e46a403
Bug 1217129: Part 4a - Allow creating CustomizableUI widgets without tooltips. r=gijs

https://hg.mozilla.org/integration/fx-team/rev/7ad8b56958c54787313ecbe60eb1e8fbafb2f376
Bug 1217129: Part 4b - [webext] Use the extension name as the default title value for browserAction/pageAction popups. r=billm

https://hg.mozilla.org/integration/fx-team/rev/628af985c7eba7e4665a1b62f9a60f6a6fd39822
Bug 1217129: Part 5 - [webext] Use CustomizableUI views for BrowserAction popups. r=gijs ui-r=bwinton

https://hg.mozilla.org/integration/fx-team/rev/106365a3847c66c2122563abb423c29685ea059c
Bug 1217129: Part 6 - [webext] Test browserAction popups in both the toolbar and in the menu panel. r=gijs r=billm

Comment 104

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/dc5742de08235de4bc270189dc248d72db476068
Bug 1217129: Follow-up: Fix test broken by rebase.

Comment 105

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93266f211716
https://hg.mozilla.org/mozilla-central/rev/3b5fb2845c3b
https://hg.mozilla.org/mozilla-central/rev/7d8dee4c335b
https://hg.mozilla.org/mozilla-central/rev/71f46fe62f59
https://hg.mozilla.org/mozilla-central/rev/7ad8b56958c5
https://hg.mozilla.org/mozilla-central/rev/628af985c7eb
https://hg.mozilla.org/mozilla-central/rev/106365a3847c
https://hg.mozilla.org/mozilla-central/rev/dc5742de0823

Comment 106

[Carsten Book [:Tomcat]]

Hi Kris, had to back this out from mozilla-central and the other trees for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6659354&repo=fx-team also after the follow up

Comment 107

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/8cb42e7a16b4

Comment 108

[Kris Maglione [:kmag]]

I can't reproduce this, even with --run-until-failure, and the only failure I can find in treeherder is in the m-c to fx-team merge. Were there others?

Comment 109

[Kris Maglione [:kmag]]

This isn't showing up on try either, so I think that failure was a fluke:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f77dcdbd995d

I'm going to re-land. I'll file an intermittent failure bug if it happens again.

Comment 110

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/8c646d5103344b1146a757dc21637a2e9921f6ce
Bug 1217129: Part 1 - Add onDestroyed callback to CustomizableUI widgets. r=gijs

https://hg.mozilla.org/integration/fx-team/rev/65913f5a537fefcbc32702a8d32c13c95f3f5dfb
Bug 1217129: Part 2 - Clear open state of view widgets if view opening is prevented. r=gijs

https://hg.mozilla.org/integration/fx-team/rev/e2fcc19b6f27308451610a59b15bf919c1372c06
Bug 1217129: Part 3 - Recalculate CustomizableUI panel arrow position after resize. r=gijs

https://hg.mozilla.org/integration/fx-team/rev/5979b6fe12500500c00985819e10d545abb76584
Bug 1217129: Part 4a - Allow creating CustomizableUI widgets without tooltips. r=gijs

https://hg.mozilla.org/integration/fx-team/rev/77ae13dc32ae43283188e287693b36cb00c599b4
Bug 1217129: Part 4b - [webext] Use the extension name as the default title value for browserAction/pageAction popups. r=billm

https://hg.mozilla.org/integration/fx-team/rev/b62be3fd000c780e23a70aa9758a07409159ec3f
Bug 1217129: Part 5 - [webext] Use CustomizableUI views for BrowserAction popups. r=gijs ui-r=bwinton

https://hg.mozilla.org/integration/fx-team/rev/48e077b052981e34081e4b8158678fee4f52e88c
Bug 1217129: Part 6 - [webext] Test browserAction popups in both the toolbar and in the menu panel. r=gijs r=billm

Comment 111

[Kris Maglione [:kmag]]

Looks like this is only happening intermittently on Windows 8 e10s. This doesn't seem to be related to the changes in the patch, so I'm just going to back out the changed test until I figure it out, as soon as the trees are open again.

Comment 112

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/f55593fee5fcba46d7bc3afea9fd75f5d03dde59
Bug 1217129: Back out a changed test for intermittent bc4-e10s failures. a=philor for a CLOSED TREE

Comment 113

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c646d510334
https://hg.mozilla.org/mozilla-central/rev/65913f5a537f
https://hg.mozilla.org/mozilla-central/rev/e2fcc19b6f27
https://hg.mozilla.org/mozilla-central/rev/5979b6fe1250
https://hg.mozilla.org/mozilla-central/rev/77ae13dc32ae
https://hg.mozilla.org/mozilla-central/rev/b62be3fd000c
https://hg.mozilla.org/mozilla-central/rev/48e077b05298
https://hg.mozilla.org/mozilla-central/rev/f55593fee5fc

Comment 114

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/8fc9a5eeede3b4551dbcbf0a77fe40349914c83e
Bug 1217129: [webext] Follow-up: Fix race in pageAction tests.

Comment 115

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And a free ESLint fix because I'm nice:
https://hg.mozilla.org/integration/fx-team/rev/bc225865b5e2

Comment 116

[Kris Maglione [:kmag]]

(In reply to Wes Kocher (:KWierso) from comment #115)
> And a free ESLint fix because I'm nice:
> https://hg.mozilla.org/integration/fx-team/rev/bc225865b5e2

Ugh. Thanks. My editor's ESLint plugin keeps breaking

Comment 117

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8fc9a5eeede3
https://hg.mozilla.org/mozilla-central/rev/bc225865b5e2

Comment 118

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox Firefox 44.0a1 (2015-10-21) under Windows 10 64-bit.
During my testing I hit only on Bug 1259093 and I did not encounter any other new issues.

Verified fixed on Firefox 46 beta 4 (20160322075646) and  Firefox 48.0a1 (2016-03-26) under Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11.