Closed Bug 1259093 Opened 8 years ago Closed 8 years ago

Short flicker for webextension's opening animation

Categories

(WebExtensions :: Untriaged, defect)

Product:
WebExtensions
Component:
Untriaged
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 unaffected, firefox46 wontfix, firefox47 wontfix, firefox48 wontfix, firefox51 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox45 --- unaffected
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- wontfix
firefox51 --- verified

People

(Reporter: vtamas, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: triaged)

Attachments

(6 files)

Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.
58 bytes, text/x-review-board-request
jaws
: review+
bwinton
: review+
Gijs
: review+
designakt
: ui-review+
Details
Bug 1259093: Part 1 - Allow async initialization from ViewShowing events.
58 bytes, text/x-review-board-request
Gijs
: review+
Details
test-popup.xpi
27.54 KB, application/x-xpinstall
Details
WebExtensionPanelFlickerTestWin10-good.gif
297.88 KB, image/gif
Details
WebExtensionPanelFlickerTestWin10-notsogood.gif
301.48 KB, image/gif
Details
Bug 1259093: Part 2 - Reparent view nodes before dispatching ViewShowing event.
58 bytes, text/x-review-board-request
Gijs
: review+
Details 
[Note]
This is a follow-up bug for Bug 1199056

[Affected versions]:
Firefox 48.0a1 (2016-03-23)
Firefox 47.0a2 (2016-03-23)
Firefox 46 beta 4 (20160322075646)

[Affected platforms]:
Ubuntu 12.04 32-bit

[Steps to reproduce]:
1.Launch Firefox with clean profile.
2.Create the xpinstall.signatures.dev-root pref in about:config and set it to true.
3.Install the following webextension: https://addons.allizom.org/en-US/firefox/addon/__ms14adfasdfg_name__/
4.Click multiple times on the webextention’s icon from toolbar.

[Expected Results]:
There are no Ui issues encountered.

[Actual Results]:
- The opening animation is a bit delayed and some flickers are displayed intermittently in the top left corner of the drop-down.
- See the attached screencast
- This problem happens for 4 of 5 attempts

[Regression range]:
Last good buildid: 20160120030239
First bag buildid: 20160121030208

Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2e50b83954e62d52d2ef294e850c4380d457d96a&tochange=977d78a8dd78afbc0153d37fd9887c3a200dce6a

So, as Kris mentioned in Bug 1199056, this issue was regressed by Bug 1217129


[Additional notes]:
- This issue reproduces only on Ubuntu (Windows and Mac platforms are not affected).
- In Firefox 47.0a2 this bug reproduces for 4 of 5 attempts.
Maybe we should try keeping the panel hidden, or opacity: 0, until the iframe loads. It would be nice if we could make the opening as seamless as possible.

The only thing that really worries me is that hidden elements have strange effects on XBL bindings (especially browsers), and I know that non-opaque panels tend to have issues on Linux. I'm not sure there are any better solutions that don't require massively refactoring the CustomizableUI code, though.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bwinton)
Are these iframes supposed to have remote content or local content? Or a mixture (e.g. local content that does an XHR to foo.com) ?

The problem with waiting for the load of anything that isn't from disk (and even then, in some cases!) is that if you're on a shitty internet connection, you might be waiting for a long time and/or forever. That seems like a bad deal. UI responsiveness shouldn't be based on network or disk latency, IMO.

Can we hide the iframe with opacity: 0 until it's loaded? Or would that leave the same flicker?

If this only affects (old versions of, too!) Ubuntu I'm also pretty OK with just wontfixing. It's not worth spending that much time on such a tiny fraction of the user population if nobody else sees this, especially if mitigation is particularly hard on exactly that platform (semitransparency and various linux window managers are not friends, as implied in Kris' comment)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch (away 24-29/3, incl.) from comment #2)
> Are these iframes supposed to have remote content or local content? Or a
> mixture (e.g. local content that does an XHR to foo.com) ?

They can currently have remote content, but they're generally expected to be
local.

> The problem with waiting for the load of anything that isn't from disk (and
> even then, in some cases!) is that if you're on a shitty internet
> connection, you might be waiting for a long time and/or forever. That seems
> like a bad deal. UI responsiveness shouldn't be based on network or disk
> latency, IMO.

Yeah, that's a fair point. If we do this, I think we should do it with a
reasonably short timeout. Maybe 200-300ms?

> Can we hide the iframe with opacity: 0 until it's loaded? Or would that
> leave the same flicker?

I don't think that would help. The problem seems to be that the panel opens,
showing only an arrow and a zero-height content area, until the iframe loads
and resizes to fit its content.

> If this only affects (old versions of, too!) Ubuntu I'm also pretty OK with
> just wontfixing. It's not worth spending that much time on such a tiny
> fraction of the user population if nobody else sees this, especially if
> mitigation is particularly hard on exactly that platform (semitransparency
> and various linux window managers are not friends, as implied in Kris'
> comment)

I'm not sure it only affects Ubuntu. It'll probably show up on other, slower
systems, or even faster systems under certain load conditions. I haven't seen
this on Linux, Windows, or OS-X, but if Vasilica is seeing it, I suspect that
a fair number of others will see it too.
(In reply to Kris Maglione [:kmag] from comment #3)
> (In reply to :Gijs Kruitbosch (away 24-29/3, incl.) from comment #2)
> > Can we hide the iframe with opacity: 0 until it's loaded? Or would that
> > leave the same flicker?
> 
> I don't think that would help. The problem seems to be that the panel opens,
> showing only an arrow and a zero-height content area, until the iframe loads
> and resizes to fit its content.

Oh, there was a screencast in the bug this was cloned from: bug 1199056
attachment 8733352 [details]. I didn't realize it wasn't in this bug too.
Pre-load the iframe on mousedown on the button, and append it into the panel view when loaded? I thought that add-ons had to provide preferred sizes for their content for these panels? Would there be bad side-effects if we loaded the panel even when the panel is not shown (ie does that violate implied contracts with the webextensions API?)
(In reply to :Gijs Kruitbosch (away 24-29/3, incl.) from comment #5)
> Pre-load the iframe on mousedown on the button, and append it into the panel
> view when loaded?

That might be doable... I think it would probably require having two
<browser>s and swapping their docshells, but I'll give it a try.

> I thought that add-ons had to provide preferred sizes for their content for
> these panels?

Unfortunately not. They're just web pages with pretty much no constraints. They generally don't even have statically-sized content.

> Would there be bad side-effects if we loaded the panel even when the panel
> is not shown (ie does that violate implied contracts with the webextensions
> API?)

Unfortunately yes, it does. There are a lot of Chrome add-ons that depend on
loading a fresh popup document for every click.
(In reply to Kris Maglione [:kmag] from comment #6)
> Unfortunately yes, it does. There are a lot of Chrome add-ons that depend on
> loading a fresh popup document for every click.

That's not so bad. What I meant was, if we load the document before the click, and potentially end up loading it when a click doesn't happen (e.g. mousedown without corresponding mouseup resulting in a click) is that going to be Bad?
(In reply to :Gijs Kruitbosch (away 24-29/3, incl.) from comment #7)
> That's not so bad. What I meant was, if we load the document before the
> click, and potentially end up loading it when a click doesn't happen (e.g.
> mousedown without corresponding mouseup resulting in a click) is that going
> to be Bad?

That's a good question. I think as long as we make sure to unload the iframe relatively quickly in that case, the extension should just be able to treat it as a popup that was opened and then immediately closed. It's probably a rare enough corner case that it shouldn't matter much in practice.
(In reply to Kris Maglione [:kmag] from comment #3)

> I'm not sure it only affects Ubuntu. It'll probably show up on other, slower
> systems, or even faster systems under certain load conditions. I haven't seen
> this on Linux, Windows, or OS-X, but if Vasilica is seeing it, I suspect that
> a fair number of others will see it too.

I've also reproduced this issue on Mac OS X 10.11 using Firefox 48.0a1 (2016-03-24) when the webextension icon is positioned to the left side of url bar (or tabs bar) and because of that, the webextension panel will have to appear in the right side. When the panel is opened, it bounces a bit and another arrow is displayed for one second in the right corner.

So, this issue is not an Ubuntu specific bug.
OS: Linux → All
Summary: [Ubuntu] Short flicker for webextension's opening animation → Short flicker for webextension's opening animation
(I think Gijs answered the question for me, so I'm un-needinfo-ing myself.  ;)
Flags: needinfo?(bwinton)
Flags: blocking-webextensions?
status-firefox46: affected → wontfix
Flags: blocking-webextensions? → blocking-webextensions?(amckay)
(In reply to Kris Maglione [:kmag] from comment #8)
> (In reply to :Gijs Kruitbosch (away 24-29/3, incl.) from comment #7)
> > That's not so bad. What I meant was, if we load the document before the
> > click, and potentially end up loading it when a click doesn't happen (e.g.
> > mousedown without corresponding mouseup resulting in a click) is that going
> > to be Bad?
> 
> That's a good question. I think as long as we make sure to unload the iframe
> relatively quickly in that case, the extension should just be able to treat
> it as a popup that was opened and then immediately closed. It's probably a
> rare enough corner case that it shouldn't matter much in practice.

Well... it depends. :-)

The bookmarks popup in Firefox does nothing when opened.

On the other hand, the pocket one (which is also implemented as a panel + iframe...) will "pocket" a page when clicked, if you're logged in, AIUI ? Which is not "nothing"...

I don't know how common that interaction model is and if we should worry about it. It's going to be tricky to get right either way.
Flags: blocking-webextensions?(amckay) → blocking-webextensions-
Whiteboard: triaged
Assignee: nobody → kmaglione+bmo
(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #11)
> (In reply to Kris Maglione [:kmag] from comment #8)
> > (In reply to :Gijs Kruitbosch (away 24-29/3, incl.) from comment #7)
> > > That's not so bad. What I meant was, if we load the document before the
> > > click, and potentially end up loading it when a click doesn't happen (e.g.
> > > mousedown without corresponding mouseup resulting in a click) is that going
> > > to be Bad?
> > 
> > That's a good question. I think as long as we make sure to unload the iframe
> > relatively quickly in that case, the extension should just be able to treat
> > it as a popup that was opened and then immediately closed. It's probably a
> > rare enough corner case that it shouldn't matter much in practice.
> 
> Well... it depends. :-)
> 
> The bookmarks popup in Firefox does nothing when opened.
> 
> On the other hand, the pocket one (which is also implemented as a panel +
> iframe...) will "pocket" a page when clicked, if you're logged in, AIUI ?
> Which is not "nothing"...
> 
> I don't know how common that interaction model is and if we should worry
> about it. It's going to be tricky to get right either way.

Was there an answer to this question? Is the page we preload this way always local? Even if not, can it make remote (http) requests? IOW, is the preloading outside-world observable?

I likely won't get to finishing this review until tomorrow, maybe Monday.

I'm also concerned that it might be better for there to be (additional?) other reviewers for this patch, both because of my sketchy webextensions knowledge and because at the moment a lot of the changes here are authored by you and reviewed by me, and so our bus factor is scarily low. More folks should be familiar with this code and there should be more of a shared understanding of all its intricacies (which seem to be monotonically increasing at a scary speed for something so seemingly "simple" as showing some extension-provided content in a panel). The recent changes to the panelmultiview bindings (cf. bug 1252224) show what happened last time we did that - trying not to make the same mistake again. :-)
Flags: needinfo?(kmaglione+bmo)
(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #14)
> Was there an answer to this question? Is the page we preload this way always
> local? Even if not, can it make remote (http) requests? IOW, is the
> preloading outside-world observable?

For the moment, we do allow remote pages to be loaded into popups, but Chrome
doesn't, so I don't think anyone does it in practice. Either way, the popups
can make remote requests and load remote resources, yes.

> I'm also concerned that it might be better for there to be (additional?)
> other reviewers for this patch, both because of my sketchy webextensions
> knowledge and because at the moment a lot of the changes here are authored
> by you and reviewed by me, and so our bus factor is scarily low.

Fair enough.

> More folks should be familiar with this code and there should be more of a
> shared understanding of all its intricacies (which seem to be monotonically
> increasing at a scary speed for something so seemingly "simple" as showing
> some extension-provided content in a panel).

It turns out that loading an auto-sized browser into a panel isn't actually
all that simple...
Flags: needinfo?(kmaglione+bmo)
Attachment #8780335 - Flags: review?(jaws)
Attachment #8780335 - Flags: review?(bwinton)
Attachment #8780335 - Flags: feedback?(mwein)
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review68894

r- because I think we should be changing the spec here to disallow remote pages.

::: browser/components/extensions/ext-browserAction.js:24
(Diff revision 2)
>  var {
>    EventManager,
>    IconDetails,
>  } = ExtensionUtils;
>  
> +const POPUP_PRELOAD_TIMEOUT = 200;

200ms is long enough that we will need to show a loading throbber here. See https://www.nngroup.com/articles/response-times-3-important-limits/ for more information on user research as it pertains to response times.

::: browser/components/extensions/ext-browserAction.js:98
(Diff revision 2)
> +        /* eslint-disable mozilla/balanced-listeners */
> +        node.addEventListener("mousedown", this);
> +        node.addEventListener("mouseup", this);
> +        /* eslint-enable mozilla/balanced-listeners */

Why are we opting out of this eslint rule? if this is necessary, an explanation comment should accompany it. I would rather though that this is not opted-out.

::: browser/components/extensions/ext-browserAction.js:148
(Diff revision 2)
> +      case "mousedown":
> +        if (event.button == 0) {
> +          // Begin pre-loading the browser for the popup, so it's more likely to
> +          // be ready by the time we get a complete click.
> +          let tab = window.gBrowser.selectedTab;
> +          let popupURL = this.getProperty(tab, "popup");
> +
> +          if (popupURL) {
> +            this.pendingPopup = this.getPopup(window, popupURL);
> +          } else {
> +            this.clearPopup();

If we're early in the liftime of webextensions, and it feels as such to me, can we make the change now to not allow remote pages in these panels?

They will make Firefox look like the slow bad guy here and it will be very easy for devs to be lazy and use the simple remote-page implementation and never come back to make their panel local.

Also, I'm not a fan of preloading on mousedown because it will start the load about 50-120ms earlier than waiting for the mouseup, which on a network request is helpful but not enough to justify the consequences of extensions that will trigger actions on load (such as Pocket). This also adds a lot more complexity to our code.

::: browser/components/extensions/ext-utils.js:285
(Diff revision 2)
> -  resizeBrowser() {
> +  resizeBrowser(force = false) {
> +    if (this.ignoreResizes) {
> +      return;

What is the outcome of resizeBrowser(true) when this.ignoreResizes is set to true? That seems pretty hairy, but right now ignore takes precendece (not obvious from reading the function signature).
Attachment #8780335 - Flags: review?(jaws) → review-
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review68894

> 200ms is long enough that we will need to show a loading throbber here. See https://www.nngroup.com/articles/response-times-3-important-limits/ for more information on user research as it pertains to response times.

Showing a throbber would defeat the purpose of this change. We could reduce the timeout to 100ms, but I'd rather not, given that we're only meant to hit this code in an edge case, and when we do, an extra 100ms with no response would be better than the flash we'd get if we tried to show a throbber, and then re-style and resize the popup.

> Why are we opting out of this eslint rule? if this is necessary, an explanation comment should accompany it. I would rather though that this is not opted-out.

Because the listeners are necessary until the node is destroyed, and once it's destroyed, there's no real need to remove them, and no particularly simple way to do so.

The rule is mainly there to catch `removeEventListener` calls that we forgot to add, or that don't match their `addEventListener` calls. In the cases where a remove call isn't needed, it generally doesn't justify a comment.

> If we're early in the liftime of webextensions, and it feels as such to me, can we make the change now to not allow remote pages in these panels?
> 
> They will make Firefox look like the slow bad guy here and it will be very easy for devs to be lazy and use the simple remote-page implementation and never come back to make their panel local.
> 
> Also, I'm not a fan of preloading on mousedown because it will start the load about 50-120ms earlier than waiting for the mouseup, which on a network request is helpful but not enough to justify the consequences of extensions that will trigger actions on load (such as Pocket). This also adds a lot more complexity to our code.

We could prevent them from loading remote pages, but that wouldn't prevent them from loading remote resources, or just loading remote iframes into local HTML pages, so it doesn't really buy us anything.

The fact that it starts the load 50-120ms before the mouseup is what makes this work. Without this patch, even on fast machines there's often a slight noticeable flicker (especially when we're styling the panel arrow to match the popup) when the panel opens. On slow machines, it's much worse. With the preload, though, even on the slow machines I've tested on, the popup feels as if it opens instantly, with the correct size and styling from the start.

I know this adds a fair amount of complexity, but I think it's worth it for the end results.

> What is the outcome of resizeBrowser(true) when this.ignoreResizes is set to true? That seems pretty hairy, but right now ignore takes precendece (not obvious from reading the function signature).

It's a no-op when ignoreResizes is set to true, since at that point the browser isn't visible, and the resize would have no useful effect. The `force` is for the times when an immediate resize is necessary, such as just before we show the popup, or when DOMContentLoaded or load fires in a visible browser. In the case of mutation-related events, we don't pass `force`, and the resizes are throttled. Maybe `ignoreThrottling` would be a better name.
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review68846

I think I'm beginning to understand most of this.  Overall this looks good to me, but there are some parts that are a bit complicated, and I think some comments could help.

Most of my comments below refer to the naming of variables.

::: browser/components/extensions/ext-browserAction.js:24
(Diff revision 2)
>  var {
>    EventManager,
>    IconDetails,
>  } = ExtensionUtils;
>  
> +const POPUP_PRELOAD_TIMEOUT = 200;

Could you add a short comment explaining this, and perhaps append `_MS` to the name? Is it essentially the amount of time we are willing to wait for the popup to pre-load before we give up on loading it?

::: browser/components/extensions/ext-browserAction.js:41
(Diff revision 2)
>    let widgetId = makeWidgetId(extension.id);
>    this.id = `${widgetId}-browser-action`;
>    this.viewId = `PanelUI-webext-${widgetId}-browser-action-view`;
>    this.widget = null;
>  
> +  this.pendingPopup = null;

What does it mean for the popup to be `pending`?

::: browser/components/extensions/ext-browserAction.js:148
(Diff revision 2)
> +  handleEvent(event) {
> +    let button = event.target;
> +    let window = button.ownerDocument.defaultView;
> +
> +    switch (event.type) {
> +      case "mousedown":

Just wondering - have you considered doing this on "mouseover"?

::: browser/components/extensions/ext-browserAction.js:166
(Diff revision 2)
> +        break;
> +
> +      case "mouseup":
> +        if (event.button == 0) {
> +          this.clearPopupTimeout();
> +          // If we have a pending pre-loaded popup, cancel it after we've wated

nit: s/wated/waited

::: browser/components/extensions/ext-browserAction.js:180
(Diff revision 2)
> +  },
> +
> +  /**
> +   * Returns a potentially pre-loaded popup for the given URL in the given
> +   * window. If a matching pre-load popup already exists, returns that.
> +   * Otherwise, initializes a new one.

nit: s/initializes/initialize

::: browser/components/extensions/ext-browserAction.js:203
(Diff revision 2)
> +        return pendingPopup;
> +      }
> +      pendingPopup.destroy();
> +    }
> +
> +    let fixedWidth = this.widget.areaType == CustomizableUI.TYPE_MENU_PANEL;

What's the need for the addition of the fixedWidth argument, and since it's a boolean I think it should be renamed to something like `isFixedWidth`.

::: browser/components/extensions/ext-utils.js:114
(Diff revision 2)
>      this.borderColor = doc.defaultView.getComputedStyle(arrowContent).borderTopColor;
>  
>      this.browser = null;
> -    this.browserReady = this.createBrowser(viewNode, popupURI);
> +    this.browserInitialized = false;
> +    this.browserLoaded = false;
> +    this.browserReady = this.createBrowser(viewNode, popupURL);

Could we make use of the member variables that these arguments are stored in?

::: browser/components/extensions/ext-utils.js:137
(Diff revision 2)
>        this.browser = null;
>        this.viewNode = null;
>      });
>    }
>  
> +  destroyBrowser(browser) {

nit: Since this only handles removing event listeners, could you rename this to something like `removeBrowserEventListeners`?

::: browser/components/extensions/ext-utils.js:248
(Diff revision 2)
>      // starts out smaller than 30px by 10px. This isn't an issue now, but it
>      // will be if and when we popup debugging.
>  
>      viewNode.appendChild(this.browser);
>  
> +    let initBrowser = browser => {

nit: Likewise, since this only handles setting up event listeners, could we rename this to something like `addBrowerEventListeners`?

::: browser/components/extensions/ext-utils.js:285
(Diff revision 2)
> -      this.browser.addEventListener("MozScrolledAreaChanged", this, true);
>      });
>    }
> +
>    // Resizes the browser to match the preferred size of the content (debounced).
> -  resizeBrowser() {
> +  resizeBrowser(force = false) {

nit: Since `force` is a boolean, could we rename it to something like `shouldForceResize`?  Also, since `resizeTimeout` is an ID, perhaps `resizeTimeoutID`?

::: browser/components/extensions/ext-utils.js:470
(Diff revision 2)
> +    this.tempPanel = panel;
> +
> +    this.browser.classList.add("webextension-preload-browser");
> +  }
> +
> +  attach(viewNode) {

Could you maybe add a comment for this method explaining what it does and why it's necessary?

::: browser/components/extensions/ext-utils.js:523
(Diff revision 2)
> +      this.destroyBrowser(browser);
> +
> +      this.ignoreResizes = false;
> +      this.resizeBrowser(true);
> +
> +      this.tempPanel.remove();

How is `tempPanel` used?

::: toolkit/components/extensions/Extension.jsm:766
(Diff revision 2)
>  
> -    let eventHandler = docShell.chromeEventHandler;
>      let listener = event => {
> -      if (event.target != docShell.contentViewer.DOMDocument) {
> -        return;
> -      }
> +      if (event.target === contentWindow.document) {
> +        contentWindow.removeEventListener("unload", listener, true);
> +        Promise.resolve().then(() => {

Why do we need the Promise.resolve().then(...) here?

::: toolkit/components/extensions/ExtensionUtils.jsm:372
(Diff revision 2)
>                dump(`Promise resolved after context unloaded\n`);
>              } else {
>                runSafe(resolve, value);
>              }
>            },
>            value => {

nit: Could we rename `value` to `error` here?
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review68846

> What does it mean for the popup to be `pending`?

That it hasn't opened yet.

> Just wondering - have you considered doing this on "mouseover"?

No, that would have way too many false positives.

> nit: s/initializes/initialize

"initializes" matches the tense of the rest of the paragraph.

> What's the need for the addition of the fixedWidth argument, and since it's a boolean I think it should be renamed to something like `isFixedWidth`.

I'm not sure what you mean by "in addition to".

> Could we make use of the member variables that these arguments are stored in?

We could, but as a rule, access to lexical variables is faster, and therefore generally preferable.

> nit: Since this only handles removing event listeners, could you rename this to something like `removeBrowserEventListeners`?

I could, but there's no particular reason to think that this will continue to only be used for event listeners. That certainly won't be the case when this becomes a remote browser.

> nit: Likewise, since this only handles setting up event listeners, could we rename this to something like `addBrowerEventListeners`?

It initially did some other things as well, and it will likely need to do other things in the future.

> How is `tempPanel` used?

It's not. It's just a stub container.

> Why do we need the Promise.resolve().then(...) here?

So we don't destroy the context until the event has finished its entire capture and bubble phases.

> nit: Could we rename `value` to `error` here?

We could, but we don't know for certain that `value` is an error here.
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review68974

This is a somewhat cursory review after the detailed reviews by Jared and Matthew. r- for the nested event loop and, AFAICT, "never" destroying/unloading the popup if the mouseup happens outside of the button.

::: browser/components/extensions/ext-browserAction.js:150
(Diff revision 2)
> +          // Begin pre-loading the browser for the popup, so it's more likely to
> +          // be ready by the time we get a complete click.
> +          let tab = window.gBrowser.selectedTab;
> +          let popupURL = this.getProperty(tab, "popup");

AIUI we never seem to clear the popup if the mouseup happens on a different element? So if I mousedown on a button, then move the mouse off the button and mouseup again, we don't clear it? That seems like a bug (and one that we should (a) fix and (b) have an automated test for).

::: browser/components/extensions/ext-utils.js:474
(Diff revision 2)
> +    // Wait until the browser element is fully initialized, and give it at least
> +    // a short grace period to finish loading its initial content, if necessary.
> +    //
> +    // In practice, the browser that was created by the mousdown handler should
> +    // nearly always be ready by this point.
> +    let timeout = Date.now() + POPUP_LOAD_TIMEOUT;
> +    while (!this.browserInitialized ||
> +           (!this.destroyed && !this.browserLoaded && Date.now() < timeout)) {
> +      Services.tm.mainThread.processNextEvent(true);
> +    }

Ewwww. Please please please can we engineer this without the nested event loops, which are evil? Why do we need this method call to be sync? I thought all the APIs here were async, so making this async should be fine?
Attachment #8780335 - Flags: review?(gijskruitbosch+bugs) → review-
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review68974

> AIUI we never seem to clear the popup if the mouseup happens on a different element? So if I mousedown on a button, then move the mouse off the button and mouseup again, we don't clear it? That seems like a bug (and one that we should (a) fix and (b) have an automated test for).

Yeah, for some reason I was expecting the mouseup to still be dispatched to the element in that case, but apparently that's only guaranteed for mouseup events dispatched to the document.

> Ewwww. Please please please can we engineer this without the nested event loops, which are evil? Why do we need this method call to be sync? I thought all the APIs here were async, so making this async should be fine?

It's not for the sake of the API, it's for the sake of the CustomizableUI code which requires the ViewShow events to be handled synchronously, and shows the view immediately after they return.

I don't like nested event loops any more than you do, but, like it or not, they're part of the platform. We spin the event loop for every `window.open` call until the window is ready, for instance, which is basically the same situation as this. Except that in this case, we almost never actually hit the nested event loop in practice.
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review68974

> It's not for the sake of the API, it's for the sake of the CustomizableUI code which requires the ViewShow events to be handled synchronously, and shows the view immediately after they return.
> 
> I don't like nested event loops any more than you do, but, like it or not, they're part of the platform. We spin the event loop for every `window.open` call until the window is ready, for instance, which is basically the same situation as this. Except that in this case, we almost never actually hit the nested event loop in practice.

We control the CUI code. Let's change it so the viewshow callback can be async.
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

(In reply to Kris Maglione [:kmag] from comment #17)
> > 200ms is long enough that we will need to show a loading throbber here.
> > See https://www.nngroup.com/articles/response-times-3-important-limits/
> > for more information on user research as it pertains to response times.
> 
> Showing a throbber would defeat the purpose of this change. We could reduce
> the timeout to 100ms, but I'd rather not, given that we're only meant to hit
> this code in an edge case, and when we do, an extra 100ms with no response
> would be better than the flash we'd get if we tried to show a throbber, and
> then re-style and resize the popup.

I'll leave this decision to Markus, since he's been designing the UX flows for
these panels.
Attachment #8780335 - Flags: ui-review?(mjaritz)
Comment on attachment 8780975 [details]
Bug 1259093: Part 1 - Allow async initialization from ViewShowing events.

https://reviewboard.mozilla.org/r/71486/#review69060

::: browser/components/customizableui/content/panelUI.xml:194
(Diff revision 1)
> +            };
> +
> +            let evt = new CustomEvent("ViewShowing", { bubbles: true, cancelable: true, detail });
> -          viewNode.dispatchEvent(evt);
> +            viewNode.dispatchEvent(evt);
> -          if (evt.defaultPrevented) {
> +
> +            let cancel = evt.defaultPrevented;

If it's been canceled, couldn't we just return immediately without waiting for the blockers?

::: browser/components/customizableui/content/panelUI.xml:196
(Diff revision 1)
> +              let results = yield Promise.all(detail.blockers);
> +              cancel = cancel || results.some(val => val === false);

Should we catch the blockers? Or just forward any rejections (and make them block showing the subview)?
Attachment #8780975 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Kris Maglione [:kmag] from comment #25)
> Comment on attachment 8780335 [details]
> Bug 1259093: Part 2 - Preload browserAction popups to prevent flicker during
> opening.
> 
> (In reply to Kris Maglione [:kmag] from comment #17)
> > > 200ms is long enough that we will need to show a loading throbber here.
> > > See https://www.nngroup.com/articles/response-times-3-important-limits/
> > > for more information on user research as it pertains to response times.
> > 
> > Showing a throbber would defeat the purpose of this change. We could reduce
> > the timeout to 100ms, but I'd rather not, given that we're only meant to hit
> > this code in an edge case, and when we do, an extra 100ms with no response
> > would be better than the flash we'd get if we tried to show a throbber, and
> > then re-style and resize the popup.
> 
> I'll leave this decision to Markus, since he's been designing the UX flows
> for
> these panels.

If you take about edge-case. How often would that happen? Or in what circumstances?
Does it happen if the content of the panel doesn't in under 100ms?
Why do our panel load so quick, but some add-on panels take considerable time?

How can I see how those different cases will behave? Deciding this only in text is rather difficult.
Flags: needinfo?(kmaglione+bmo)
Attached file test-popup.xpi — 
(In reply to Markus Jaritz [:maritz] (UX) from comment #27)
> If you take about edge-case. How often would that happen? Or in what
> circumstances?

It's hard to say. So far, I haven't seen any panel load that slowly, but it
will definitely happen from time to time, on slow systems, or under high load
conditions.

> Does it happen if the content of the panel doesn't in under 100ms?
> Why do our panel load so quick, but some add-on panels take considerable
> time?

Well, this version of the patch only waits for the DOMContentLoaded event, so,
generally speaking, the only thing that should affect this is system load. The
one snag is that <script> nodes block the loading of the rest of the DOM until
they're fully loaded and executed, and popups can technically load remote
scripts if they so choose.

> How can I see how those different cases will behave? Deciding this only in
> text is rather difficult.

It's probably easiest if you test yourself. Here's my latest try build[1], and
I'm attaching one of the add-ons I've been testing with.

[1]: https://archive.mozilla.org/pub/firefox/try-builds/maglione.k@gmail.com-54cbf3ed1eb4da66e660be3ba3e76acb79ffe66f/try-macosx64/firefox-51.0a1.en-US.mac.dmg
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8780975 [details]
Bug 1259093: Part 1 - Allow async initialization from ViewShowing events.

https://reviewboard.mozilla.org/r/71486/#review69060

> If it's been canceled, couldn't we just return immediately without waiting for the blockers?

I was on the fence about that. My thought was that it probably made sense to let them finish any pending operations before we bailed out, but since the only cleanup we actually do is clearing the `open` attribute, it probably doesn't matter.

> Should we catch the blockers? Or just forward any rejections (and make them block showing the subview)?

I think that catching and making them block the subview probably makes the most sense.
Thanks for helping me experience it. :-)
Definitely looks nicer than the in the current Nightly. I a new profile I was able to get the same instant opening than with our native panels, where as in my regular profile with a lot of tabs and videos running in the background I was able to slow it down a little to experience the delay.
Definitely noticeable delay, but better than having something pop up instantly only to be replaced right thereafter.
If loading the panel takes even longer, what would happen then? Would the refuse to show until it is loaded, would a spinner appear, or a half loaded panel?
Tested with good performance.
Tested with medium performance.
(In reply to Markus Jaritz [:maritz] (UX) from comment #30)
> If loading the panel takes even longer, what would happen then? Would the
> refuse to show until it is loaded, would a spinner appear, or a half loaded
> panel?

For the moment, if the load takes longer than 200ms, we just show whatever's ready at that point. We could change it to show a spinner, instead, though, with some fairly simple CSS.
I think the spinner is a good idea. However if the content doesn't load wouldn't the user just be presented with a never ending spinner and not potentially know why.
(In reply to drhen123 from comment #34)
> I think the spinner is a good idea. However if the content doesn't load
> wouldn't the user just be presented with a never ending spinner and not
> potentially know why.

The alternative is that the user would be potentially presented with an empty panel and have no idea either.

Perhaps we could put in some text in to the panel if the load takes longer than 3 seconds, like how Gmail does when it is slow to load. Gmail says "This is taking longer than usual. Try reloading the page.", but we could do "This is taking longer than usual." and maybe in the future add a link to report a slow add-on.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #35)
> (In reply to drhen123 from comment #34)
> > I think the spinner is a good idea. However if the content doesn't load
> > wouldn't the user just be presented with a never ending spinner and not
> > potentially know why.
> 
> The alternative is that the user would be potentially presented with an
> empty panel and have no idea either.
> 
> Perhaps we could put in some text in to the panel if the load takes longer
> than 3 seconds, like how Gmail does when it is slow to load. Gmail says
> "This is taking longer than usual. Try reloading the page.", but we could do
> "This is taking longer than usual." and maybe in the future add a link to
> report a slow add-on.

Like you said the alternative is a empty panel, so I think putting some text to alert the user would maybe help them understand what's happening.
I like the idea of reporting the add-on if it is slow. On smaller panels would they possibly have to resize to fit all this text on though? Otherwise the text may be small and unreadable.
If I understand that right, there are 2 possible outcomes if loading isn't finished after 200ms:

(In reply to Kris Maglione [:kmag] from comment #33)
> we just show whatever's ready at that point.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #35)
> the user would be potentially presented with an empty panel

I think at that point we should consider showing whatever is ready and help the developer handle longer loading of additional elements themselves. Like with gmail, they deal with the longer loading while showing the first pieces of UI. That means we should recommend developers that they show UI as soon as possible and load content step by step if needed. (Like it is common with app- or web-dev anyway.)

Unless of course nothing has loaded, then it is up to us to help the user figure out what's wrong. (And at that point we probably can decide the size of the panel ourselves as nothing has loaded to indicate the size.) - "This is taking longer than usual."
Attachment #8780335 - Flags: review?(bwinton)
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review69424

(Given the r-s, I don't think we need my input here, too…  :)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #35)
> Perhaps we could put in some text in to the panel if the load takes longer
> than 3 seconds, like how Gmail does when it is slow to load. Gmail says
> "This is taking longer than usual. Try reloading the page.", but we could do
> "This is taking longer than usual." and maybe in the future add a link to
> report a slow add-on.

(In reply to Markus Jaritz [:maritz] (UX) from comment #38)
> If I understand that right, there are 2 possible outcomes if loading isn't
> finished after 200ms:
> 
> (In reply to Kris Maglione [:kmag] from comment #33)
> > we just show whatever's ready at that point.
> 
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #35)
> > the user would be potentially presented with an empty panel
> 
> I think at that point we should consider showing whatever is ready and help
> the developer handle longer loading of additional elements themselves. Like
> with gmail, they deal with the longer loading while showing the first pieces
> of UI. That means we should recommend developers that they show UI as soon
> as possible and load content step by step if needed. (Like it is common with
> app- or web-dev anyway.)
> 
> Unless of course nothing has loaded, then it is up to us to help the user
> figure out what's wrong. (And at that point we probably can decide the size
> of the panel ourselves as nothing has loaded to indicate the size.) - "This
> is taking longer than usual."

I think we should get some telemetry on panel loading times in Nightly and
Aurora before we make a decision about this. I'm not expecting it to be a
common enough problem to be worth the added complexity, and if it turns out to
be, I'd rather we do something to prevent it (or let developers show their own
"This is taking longer than expected..." message) than hack around it.

I'm happy to implement the spinner now (but ideally in a follow-up bug) if
that's what we want, but I think anything more ambitious should wait for
telemetry.
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review69424

I think Gijs mainly wanted additional reviewers so we know there will be people who can review/maintain this code if the two of us get hit by a bus. I agree that this particular patch doesn't really need three reviewers, though.
Yep.  I'll definitely take a look at the next iteration of the patch.  I just figured I'ld skip this version.  ;)
(In reply to Blake Winton (:bwinton) (:☕️) from comment #42)
> Yep.  I'll definitely take a look at the next iteration of the patch.  I
> just figured I'ld skip this version.  ;)

Oh, the r-es were for the last version. mozreview just didn't update, even though they're flagged r? on Bugzilla...
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review69596

Almost r+, but the double promise queue wait still feels hacky. Note also the two other points I commented where I *think* (and I could be wrong...) we're risking races that will throw errors if the popup gets destroyed at the "wrong" time.

::: browser/components/extensions/ext-utils.js:124
(Diff revision 3)
> +    this.browserReady = this.createBrowser(viewNode, popupURL);
>    }
>  
>    destroy() {
> -    this.browserReady.then(() => {
> -      this.browser.removeEventListener("DOMWindowCreated", this, true);
> +    this.destroyed = true;
> +    this.browserLoadedDeferred.reject(new Error("Popup destroyed"));

This will throw if the promise has already been resolved, right?

::: browser/components/extensions/ext-utils.js:533
(Diff revision 3)
> +      // so that we can be sure the view node has been moved to its final
> +      // position in the DOM tree by the PanelUI code. If we swap the docShells

Can we wait for this more explicitly? This feels very hacky. :-(

::: browser/components/extensions/ext-utils.js:547
(Diff revision 3)
> +        this.tempPanel.remove();
> +        this.tempPanel = null;

This will throw if we get destroyed while waiting for the two promise queues, no? In fact, it might throw a bit sooner (I assume this.browser will be null?), so we should probably guard for that?
Attachment #8780335 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review69596

> This will throw if the promise has already been resolved, right?

No, the spec requires that resolving or rejecting a promise which has already been settled is silently ignored.

> Can we wait for this more explicitly? This feels very hacky. :-(

Ideally, but I couldn't think of another way to do it that didn't feel just as hacky. The best alternative I have at this point is to do the reparenting before we dispatch the `ViewShowing` event and undo it if we cancel it, but I don't know if that's a good idea.

> This will throw if we get destroyed while waiting for the two promise queues, no? In fact, it might throw a bit sooner (I assume this.browser will be null?), so we should probably guard for that?

I don't think it's possible to get to this point and have `tempPanel` be null. There shouldn't actually be a chance for anything to call `destroy()` after we check `this.destroyed`, since promise handlers are guaranteed to run before we re-enter the main event loop where anything would have a chance to call it. And even if something does call it, there are also two chained resolution handlers in the destroy() code before the panel is removed, so this should still run first.

If I'm wrong, I'd rather we just see the error, since it might be useful information, and it won't have any other negative effects.
Comment on attachment 8781789 [details]
Bug 1259093: Part 2 - Reparent view nodes before dispatching ViewShowing event.

https://reviewboard.mozilla.org/r/72120/#review69806

rs=me
Attachment #8781789 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review69808

::: browser/components/extensions/test/browser/browser_ext_popup_api_injection.js:92
(Diff revision 4)
>    yield extension.awaitMessage("from-popup-a");
> +  yield promisePopupShown(getBrowserActionPopup(extension));

Doesn't this race? I'm assuming the message is sent by the document loaded in the browser in the panel, and so it's conceivable that the popupshowing/popupshown event for which we want to wait has fired by the time we start listening for it.

::: browser/components/extensions/test/browser/browser_ext_popup_api_injection.js:97
(Diff revision 4)
>    yield extension.awaitMessage("from-popup-a");
> +  yield promisePopupShown(getBrowserActionPopup(extension));
>    yield closeBrowserAction(extension);
>  
>    yield clickPageAction(extension);
>    yield extension.awaitMessage("from-popup-b");

And why don't we need to do the same popup waiting here?
Attachment #8780335 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review69596

> I don't think it's possible to get to this point and have `tempPanel` be null. There shouldn't actually be a chance for anything to call `destroy()` after we check `this.destroyed`, since promise handlers are guaranteed to run before we re-enter the main event loop where anything would have a chance to call it. And even if something does call it, there are also two chained resolution handlers in the destroy() code before the panel is removed, so this should still run first.
> 
> If I'm wrong, I'd rather we just see the error, since it might be useful information, and it won't have any other negative effects.

Now that we're no longer waiting before doing this, I think this isn't an issue anymore.
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review69808

> Doesn't this race? I'm assuming the message is sent by the document loaded in the browser in the panel, and so it's conceivable that the popupshowing/popupshown event for which we want to wait has fired by the time we start listening for it.

It doesn't, because `promisePopupShown` checks if the popup is already shown and resolves immediately if it is.

> And why don't we need to do the same popup waiting here?

Because pageAction popups always open a new popup, so the helper just waits for that popup to be shown if it's not already. browserAction popups sometimes open in a sub-view, which the helper doesn't know how to handle yet. It would be nice to fix that, though.
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review70478

::: browser/components/extensions/ext-browserAction.js:98
(Diff revision 4)
>        onCreated: node => {
>          node.classList.add("badged-button");
>          node.classList.add("webextension-browser-action");
>          node.setAttribute("constrain-size", "true");
>  
> +        node.onmousedown = event => this.handleEvent(event);

Please use addEventListener here, as this style is generally frowned upon due to its requirement that only one event listener can be attached.

`node.onmousedown = event => this.handleEvent(event);` 
vs
`node.addEventListener("mousedown", this);`

is also less characters.

::: browser/components/extensions/ext-browserAction.js:153
(Diff revision 4)
> +          } else {
> +            this.clearPopup();

Is this necessary? It seems if we don't have a popupURL there won't be any timeouts created nor any pendingPopups.

::: browser/components/extensions/ext-utils.js:125
(Diff revision 4)
> -      this.browser.removeEventListener("load", this, true);
> -      this.browser.removeEventListener("DOMTitleChanged", this, true);
> +    return this.browserReady.then(() => {
> +      this.destroyBrowser(this.browser);

Why does destroy() have to wait for browserReady to resolve before the browser can be destroyed?

::: browser/components/extensions/ext-utils.js:130
(Diff revision 4)
> -      this.browser.removeEventListener("load", this, true);
> -      this.browser.removeEventListener("DOMTitleChanged", this, true);
> -      this.browser.removeEventListener("DOMWindowClose", this, true);
> -      this.browser.removeEventListener("MozScrolledAreaChanged", this, true);
> +    return this.browserReady.then(() => {
> +      this.destroyBrowser(this.browser);
> +      this.browser.remove();
> +
>        this.viewNode.removeEventListener(this.DESTROY_EVENT, this);
>        this.viewNode.style.maxHeight = "";

Can this use this.viewNode.style.removeProperty("max-height"); here instead?

::: browser/components/extensions/ext-utils.js:132
(Diff revision 4)
>        this.panel.style.setProperty("--panel-arrowcontent-background", "");
>        this.panel.style.setProperty("--panel-arrow-image-vertical", "");

Same with removeProperty here too.

::: browser/components/extensions/ext-utils.js:287
(Diff revision 4)
> -      this.browser.addEventListener("DOMWindowClose", this, true);
> -      this.browser.addEventListener("MozScrolledAreaChanged", this, true);
>      });
>    }
> +
>    // Resizes the browser to match the preferred size of the content (debounced).

Can you explain what "debounced" means again? I'm still not sure what this is implying.

::: browser/components/extensions/ext-utils.js:560
(Diff revision 4)
> -    return !this.viewNode.classList.contains("cui-widget-panelview");
> +    return "ViewHiding";
>    }
>  
>    closePopup() {
> +    if (this.attached) {
> -    CustomizableUI.hidePanelForNode(this.viewNode);
> +      CustomizableUI.hidePanelForNode(this.viewNode);

When does the browser get unloaded and destroyed in this case? We should make sure to unload the browser at some point to limit the ability for add-ons to unknowingly consume memory.

It may also be better to deterministically destroy the browser so as to make it easier for developers and users to know when the browser will be loading the content again.

If for example we destroy the browser after a 5 second delay, then it can be confusing to users who close and then immediately reopen a browser as to why the content is stale, versus waiting some indefinite amount of time and seeing fresh content. If on the other hand we destroy the browser immediately when the popup is closed, then developers and users can have a better reasoning that each time the popup opens the content will be fresh.
Attachment #8780335 - Flags: review?(jaws) → review-
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review70696

Sorry, I hit publish too soon or accidentally, not sure. But either way, I didn't mean to mark this as r-.

My biggest question in this latest round of feedback is about destorying the browser when the popup is closed. I want to make sure that we are not keeping browsers loaded indefinitely.
Attachment #8780335 - Flags: review- → review+
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review70478

> Can you explain what "debounced" means again? I'm still not sure what this is implying.

Sorry, I didn't mean to hit publish so soon. I looked this up and know what it is now.

We should be using DeferredTask.jsm instead of writing custom debouncing functions. Can you file a follow-up to replace these debouncing functions with DeferredTask.jsm usage?
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review70750

Looks good to me, for the parts I understood.  But Jaws brought up some good points I hadn't considered, too.
Attachment #8780335 - Flags: review?(bwinton) → review+
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review70478

> Please use addEventListener here, as this style is generally frowned upon due to its requirement that only one event listener can be attached.
> 
> `node.onmousedown = event => this.handleEvent(event);` 
> vs
> `node.addEventListener("mousedown", this);`
> 
> is also less characters.

I mainly did this because it doesn't require an ESLint override (which would make it significantly more characters than the `addEventListener` alternative.

> Is this necessary? It seems if we don't have a popupURL there won't be any timeouts created nor any pendingPopups.

In practice, no, it shouldn't be necessary. It's just barely possible that there are some races where it might have an effect, but it's mainly just there as a minor precaution, and for symmetry with the ViewShowing handler, since it does no harm.

> Why does destroy() have to wait for browserReady to resolve before the browser can be destroyed?

We don't finish setting up the initial browser element until we get the first load event for about:blank. If we try to destroy it before then, the initial setup handler will run after the destroy code, and we won't clean up properly.

> Can this use this.viewNode.style.removeProperty("max-height"); here instead?

Assigning an empty string is equivalent to calling `removeProperty`. `removeProperty` probably does make more sense than `setProperty` in the other cases, though.

> Sorry, I didn't mean to hit publish so soon. I looked this up and know what it is now.
> 
> We should be using DeferredTask.jsm instead of writing custom debouncing functions. Can you file a follow-up to replace these debouncing functions with DeferredTask.jsm usage?

It refers to the behavior of mechanical button switches, which tend to bounce several times, very quickly, every time that you press them, which requires digital devices like keyboards to have "debouncing" controllers to remove the extra presses. "Rate limiting" might be a better term.

I don't think DeferredTask is really a good fit here, since when this is called, we need to resize immediately, and then only after a delay. I'm hoping to eventually replace this with a reflow observer, which should remove the need for debouncing entirely.

> When does the browser get unloaded and destroyed in this case? We should make sure to unload the browser at some point to limit the ability for add-ons to unknowingly consume memory.
> 
> It may also be better to deterministically destroy the browser so as to make it easier for developers and users to know when the browser will be loading the content again.
> 
> If for example we destroy the browser after a 5 second delay, then it can be confusing to users who close and then immediately reopen a browser as to why the content is stale, versus waiting some indefinite amount of time and seeing fresh content. If on the other hand we destroy the browser immediately when the popup is closed, then developers and users can have a better reasoning that each time the popup opens the content will be fresh.

The browser gets destroyed as soon as the popup is hidden, and it gets a fresh load every time it's opened. A lot of extensions depend on that.
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

https://reviewboard.mozilla.org/r/71064/#review70696

No worries. I've done that at least a half dozen times since they changed the review flow.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a149f06f8b4e6c033f21891c3d8dbae3f391311
Bug 1259093: Part 1 - Allow async initialization from ViewShowing events. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/f432e51a87a27253cb8b60398df04c0adac6dbc5
Bug 1259093: Part 2 - Reparent view nodes before dispatching ViewShowing event. r/rs=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/72db19af567dbe4d665bc0658fc03c2f42cda3e7
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening. r=Gijs r=jaws r=bwinton f=mattw
Blocks: 1294474
Markus,

I landed this before I got a final ui-r for the sake of fixing bug 1294474, but we still need to decide whether we want to show a spinner if the popup is slow. If we do, I'll file a follow-up bug for that. Either way, I'm going to file a follow-up to get telemetry data for the opening perf.
Flags: needinfo?(mjaritz)
Blocks: 1297167
Comment on attachment 8780335 [details]
Bug 1259093: Part 3 - Preload browserAction popups to prevent flicker during opening.

Showing what's ready after a certain time is a good call. It allows devs to handle any further loading themselves. Great.
A spinner should only be an option if nothing at all has loaded after a certain time (200ms). But I do not know how often that would be the case.
Would be great to have metrics for that.
Flags: needinfo?(mjaritz)
Attachment #8780335 - Flags: ui-review?(mjaritz) → ui-review+
Blocks: webext-popups
I was able to reproduce the initial issue on Firefox 50.0a2 (2016-09-12) under Ubuntu 16.04 32-bit.

Verified fixed on Firefox 51.0a1 (2016-09-12) under Ubuntu 16.04 32-bit, Windows 10 64-bit and Mac OS X 10.11. The panel is smoothly opened and there are no longer UI glitches.

I’ve also noticed a few issues while testing using test-popup.xpi from (https://bugzilla.mozilla.org/show_bug.cgi?id=1259093#c28) :
 - Intermittently, while hovering over the top side of the webextension panel is displayed a white section: http://screencast.com/t/xVKVz7owf (reproduces on Windows and Ubuntu)
 - the webextension panel is continuously opened when clicking the icon from URL bar https://www.dropbox.com/s/xjvcu2powcpej3j/gif.mov?dl=0 (reproduces on Mac and Windows)


Kris, are these issues already tracked or should I file new bugs?
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: needinfo?(kmaglione+bmo)
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #64)
> I was able to reproduce the initial issue on Firefox 50.0a2 (2016-09-12)
> under Ubuntu 16.04 32-bit.
> 
> Verified fixed on Firefox 51.0a1 (2016-09-12) under Ubuntu 16.04 32-bit,
> Windows 10 64-bit and Mac OS X 10.11. The panel is smoothly opened and there
> are no longer UI glitches.
> 
> I’ve also noticed a few issues while testing using test-popup.xpi from
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1259093#c28) :
>  - Intermittently, while hovering over the top side of the webextension
> panel is displayed a white section: http://screencast.com/t/xVKVz7owf
> (reproduces on Windows and Ubuntu)

That's more of a bug with the extension itself. It has a hover effect for the body that makes its background transparent, which isn't something we even try to handle. If the mouse is hovering over the panel contents when we copy the background color, then the hover color takes effect, rather than the default color.

>  - the webextension panel is continuously opened when clicking the icon from
> URL bar https://www.dropbox.com/s/xjvcu2powcpej3j/gif.mov?dl=0 (reproduces
> on Mac and Windows)

Hm. There was a bug to handle this for browserActions, but I don't think there's one for pageActions. Can you please file one?

Thanks.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #65)

> That's more of a bug with the extension itself. It has a hover effect for
> the body that makes its background transparent, which isn't something we
> even try to handle. If the mouse is hovering over the panel contents when we
> copy the background color, then the hover color takes effect, rather than
> the default color.

It worth filing a new bug also for this issue?

> Hm. There was a bug to handle this for browserActions, but I don't think
> there's one for pageActions. Can you please file one?
> Thanks.

Filed Bug 1302746.
Flags: needinfo?(kmaglione+bmo)
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #66)
> (In reply to Kris Maglione [:kmag] from comment #65)
> 
> > That's more of a bug with the extension itself. It has a hover effect for
> > the body that makes its background transparent, which isn't something we
> > even try to handle. If the mouse is hovering over the panel contents when we
> > copy the background color, then the hover color takes effect, rather than
> > the default color.
> 
> It worth filing a new bug also for this issue?

I don't think so. It may be worth documenting that the behavior is undefined when the background changes based on hover state, though.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1341947
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: