Closed Bug 1217129 Opened 4 years ago Closed 4 years ago

Use CustomizableUI view widget for BrowserAction panels

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox46 verified, firefox48 verified)

VERIFIED FIXED
mozilla46
Iteration:
46.1 - Dec 28
Tracking Status
firefox46 --- verified
firefox48 --- verified

People

(Reporter: designakt, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webextension-polish])

Attachments

(11 files, 1 obsolete file)

38.54 KB, image/gif
Details
339.05 KB, image/gif
Details
1.11 MB, image/gif
Details
22.17 KB, patch
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
bwinton
: ui-review-
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Gijs
: review+
billm
: review+
Details
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)
I'll take this one…
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. ui-r=maritz, r=billm
Attachment #8677474 - Flags: review?(wmccloskey)
Attachment #8677474 - Flags: review?(mjaritz)
Attachment #8677474 - Flags: review?(mjaritz) → ui-review?(mjaritz)
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 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
Attachment #8677474 - Flags: review?(wmccloskey)
Attachment #8677474 - Flags: review?(mjaritz)
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
Attachment #8677474 - Flags: review?(mjaritz)
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
Attachment #8677474 - Flags: review?(wmccloskey) → review+
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.
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…
Attachment #8677474 - Flags: review?(mjaritz)
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 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.)
Attachment #8677474 - Flags: review?(wmccloskey)
Attachment #8677474 - Flags: review?(mjaritz)
Attachment #8677474 - Flags: review+
Attachment #8677474 - Flags: feedback?(kmaglione+bmo)
Attached image 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).
Attachment #8677474 - Flags: ui-review?(mjaritz) → ui-review-
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.
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 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
Attachment #8677474 - Flags: feedback?(kmaglione+bmo) → review?(mjaritz)
Attachment #8677474 - Flags: ui-review?(mjaritz)
Attachment #8677474 - Flags: ui-review-
Attachment #8677474 - Flags: review?(mjaritz)
Attachment #8677474 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8677474 [details]
MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!

Looks perfect! Thx.
Attachment #8677474 - Flags: ui-review?(mjaritz) → ui-review+
Attachment #8677474 - Flags: feedback?(kmaglione+bmo) → feedback+
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".
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. :)
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 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
Attachment #8677474 - Flags: review?(wmccloskey)
Attachment #8677474 - Flags: review?(mjaritz)
Attachment #8677474 - Flags: review-
Attachment #8677474 - Flags: feedback+
(Kris: I ended up using `panel.state`, which uses `.popupState` internally, because it seems like it'll break less if things change.  :)
Attachment #8677474 - Flags: review?(mjaritz)
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.
Attachment #8677474 - Flags: review?(wmccloskey) → review+
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
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?
(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.
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.
(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…
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.
(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.
(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.
Attachment #8677474 - Attachment description: MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. ui-r=maritz, r=billm → MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. ui-r=maritz, r=billm, r=kmag
Attachment #8677474 - Flags: review?(mjaritz)
Attachment #8677474 - Flags: review?(kmaglione+bmo)
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/
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.
Attachment #8677474 - Flags: ui-review?(mjaritz)
Attachment #8677474 - Flags: ui-review+
Attachment #8677474 - Flags: review?(wmccloskey)
Attachment #8677474 - Flags: review?(mjaritz)
Attachment #8677474 - Flags: review+
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 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.
Attachment #8677474 - Flags: review?(wmccloskey) → review+
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)
Attachment #8677474 - Flags: ui-review?(mjaritz) → ui-review-
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.
Attachment #8677474 - Flags: review?(kmaglione+bmo)
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…)
Attachment #8677474 - Flags: review?(mjaritz)
Attachment #8677474 - Flags: review?(kmaglione+bmo)
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 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?
Attachment #8677474 - Flags: ui-review-
Attachment #8677474 - Flags: review?(mjaritz)
Attachment #8677474 - Flags: review?(kmaglione+bmo)
Attachment #8677474 - Flags: review+
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.
(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. :)

(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 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/
Attachment #8677474 - Attachment description: MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. ui-r=maritz, r=billm, r=kmag → MozReview Request: Bug 1217129 - Toggle WebExtensions panels on click instead of re-opening them. WIP!
Attachment #8677474 - Flags: review?(wmccloskey)
Attachment #8677474 - Flags: review?(mjaritz)
Attachment #8677474 - Flags: review?(kmaglione+bmo)
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.  ;)
Attachment #8677474 - Flags: review?(wmccloskey)
Attachment #8677474 - Flags: review?(mjaritz)
Attachment #8677474 - Flags: review?(kmaglione+bmo)
(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?
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.
(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!
(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.
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…)
Assignee: bwinton → nobody
Status: ASSIGNED → NEW
Flags: blocking-webextensions?
Attached patch bug1217129.diffSplinter Review
Attachment #8677474 - Attachment is obsolete: true
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.
Assignee: nobody → kmaglione+bmo
Awesome, thanks!  :)
And if you'ld like me to test stuff on Windows 10 or Mac OS X, please let me know!
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.
Summary: While a panel is open, the button should stay pressed → Use CustomizableUI view widget for BrowserAction panels
Duplicate of this bug: 1199052
Speaking as a UX person, the slide-in view is what we want in that case.  :D
Iteration: --- → 45.3 - Dec 14
Iteration: 45.3 - Dec 14 → 46.1 - Dec 28
Flags: blocking-webextensions? → blocking-webextensions+
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.
Attachment #8705498 - Flags: ui-review?(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
Attachment #8705498 - Flags: ui-review?(bwinton) → ui-review-
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
Attachment #8705497 - Flags: review?(gijskruitbosch+bugs) → review+
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! :-)
Attachment #8705496 - Flags: review?(gijskruitbosch+bugs)
Attachment #8705498 - Flags: review?(gijskruitbosch+bugs)
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?
(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?
(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).
(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…  ;)
(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.
Oh, right, one of these days I'll remember I'm supposed to reply to reviews in reviewboard...
(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.
(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.
(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.
(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.
(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.
(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.
(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.
Blake, can you test again with this build when you get a chance? http://archive.mozilla.org/pub/firefox/try-builds/maglione.k@gmail.com-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.
Flags: needinfo?(bwinton)
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/
Attachment #8705496 - Flags: review?(gijskruitbosch+bugs)
Attachment #8705497 - Attachment description: MozReview Request: Bug 1217129: Part 2 - Clear open state of view widgets if view opening is prevented. r?gijs → MozReview Request: Bug 1217129: Part 2 - Clear open state of view widgets if view opening is prevented. r=gijs
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/
Attachment #8705498 - Attachment description: MozReview Request: Bug 1217129: Part 3 - [webext] Use CustomizableUI views for BrowserAction popups. r?gijs ui-r?bwinton → MozReview Request: Bug 1217129: Part 3 - Recalculate CustomizableUI panel arrow position after resize. r?gijs
Attachment #8705498 - Flags: review?(gijskruitbosch+bugs)
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/
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/
Attachment #8706565 - Flags: review?(gijskruitbosch+bugs)
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/
Attachment #8706566 - Flags: review?(wmccloskey)
Attachment #8706566 - Flags: review?(gijskruitbosch+bugs)
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.
(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/maglione.k@gmail.com-
> 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
Flags: needinfo?(bwinton)
Just FYI, I'm afraid I won't be able to get to this until Wednesday.
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).
Attachment #8705496 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8706563 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Attachment #8706565 - Flags: review?(gijskruitbosch+bugs)
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. :-\
Attachment #8705498 - Flags: review?(gijskruitbosch+bugs) → review+
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() ?
Attachment #8706566 - Flags: review?(gijskruitbosch+bugs) → review+
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?
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.
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.
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.
https://reviewboard.mozilla.org/r/29991/#review27237

Yup, I was already planning on it.
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.
Attachment #8706564 - Flags: review?(wmccloskey) → review+
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
Attachment #8706566 - Flags: review?(wmccloskey) → review+
(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.
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
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
Status: RESOLVED → REOPENED
Flags: needinfo?(kmaglione+bmo)
Resolution: FIXED → ---
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?
Flags: needinfo?(kmaglione+bmo) → needinfo?(cbook)
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.
Flags: needinfo?(cbook)
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
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.
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
(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
Blocks: 1190663
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.
Status: RESOLVED → VERIFIED
Depends on: 1370967
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.