bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

load iframe prior to showing flyout

RESOLVED FIXED in Firefox 24

Status

()

Firefox
SocialAPI
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

24 Branch
Firefox 24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

6 years ago
On first startup of firefox, the flyout panel will open empty.

The only way a provider can fill the flyout is by opening it, which results in an empty panel for a few seconds while the initial content loads.  We added support for data urls in the open, which is one way to handle this, but for large infrastructure sites like facebook that probably wont work well.

The proposal is to add a new message similar to social.ambient-notification that provides a url for the flyout.  We will load that url into the flyout (without open) when we receive it.  The flyout itself can grab a port an notify the worker when it is ready.
(Assignee)

Comment 1

6 years ago
We should consider calling this message social.configure that can send a key/value pair.  It can later replace much of the manifest, potentially everything but the worker url.

Comment 2

6 years ago
More important than being empty I think is that when you open it again it contains the contents of the last loaded URL until the new one has loaded. That is, open URL A, then open URL B, the content for A will show until B finishes loading. This happens even if you've closed the flyout in between.

For us the data urls will be the best workaround. We want the flyouts to open as quickly as possible. Having a single flyout URL which just listens to the worker could work, but wouldn't be optimal for us since we have at least two very distinct flyouts.
(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> We should consider calling this message social.configure that can send a
> key/value pair.  It can later replace much of the manifest, potentially
> everything but the worker url.

Agree a new config message could make alot of sense.  We could also roll the ambient stuff into it too.  Not sure it makes sense to try and get that into 17 though.

Maybe a lower impact change for 17 could be to add the flyout url to the manfifest.  If supplied, we can pre-load the flyout with the URL before it is opened - then assuming they call openFlyout with the same URL, it'll be good.  Not specified == exactly as now.

(In reply to edA-qa mort-ora-y from comment #2)
> More important than being empty I think is that when you open it again it
> contains the contents of the last loaded URL until the new one has loaded.

Yeah, that sucks, but you can probably work around that by "resetting" the panel when hidden (we send a custom event in that case).

> For us the data urls will be the best workaround. We want the flyouts to
> open as quickly as possible. Having a single flyout URL which just listens
> to the worker could work, but wouldn't be optimal for us since we have at
> least two very distinct flyouts.

data urls can be handy, but I think the only way to have low-latency will be to use a constant URL  and use port messages to have it change context as necessary - also leveraging the custom show/hide events as necessary.  Ditto for the ambient stuff.  So I think Shane might be correct in that the URLs for all panels (and even recommend while we are at it) could move to being statically specified and rely on the messaging to adjust the dynamic content.  This could well be faster and more responsive than data urls.

Comment 4

6 years ago
Perhaps a different defect, but it seems very related to this, the size of the opened panel relates to the previous document that was loaded. It does not resize when a new document is loaded. I say this is related since I suspect the same mechanism is at work here and might be fixed in the same manner. Or should I enter it as a new defect.

Comment 5

6 years ago
About using ports, I'm not certain how this will work. Would the flyout then be permanently open (just in memory) or would it still open/close. In particular how can I do the workflow of pressing a button in the sidebar and opening a particular content? The sidebar has to deal with the situation where it is open and closed, but also must know when it is opened to send the appropriate message. This sounds like it might be hairy.

That is, how do I replicate the behaviour I'd have now with URLs? When I call openPanel now I can specify the url string complete with query parameters, the panel will be opened, and the content loaded. It seems like it'd be difficult for the sidebar to manage this in multiple steps.
(In reply to edA-qa mort-ora-y from comment #4)
> Perhaps a different defect, but it seems very related to this, the size of
> the opened panel relates to the previous document that was loaded. It does
> not resize when a new document is loaded. I say this is related since I
> suspect the same mechanism is at work here and might be fixed in the same
> manner. Or should I enter it as a new defect.

This sounds like a new defect - we do perform a resize as the panel is shown, but it is possible that is before the load event and the load event may not trigger the code that notices dynamic changes.

(In reply to edA-qa mort-ora-y from comment #5)
> About using ports, I'm not certain how this will work. Would the flyout then
> be permanently open (just in memory) or would it still open/close. In
> particular how can I do the workflow of pressing a button in the sidebar and
> opening a particular content? The sidebar has to deal with the situation
> where it is open and closed, but also must know when it is opened to send
> the appropriate message. This sounds like it might be hairy.

All you should need to deal with is a new port being created by the panel, and that port later being closed.  If the panel content lives longer than the panel itself, then the port will not be closed.  If the panel content has been unloaded, then it will be able to establish a new port when it loads.

I expect we will want to avoid making explicit promises about the lifetimes of the panels so Firefox is free to manage them based on things such as memory pressure etc.

> That is, how do I replicate the behaviour I'd have now with URLs? When I
> call openPanel now I can specify the url string complete with query
> parameters, the panel will be opened, and the content loaded. It seems like
> it'd be difficult for the sidebar to manage this in multiple steps.

It would probably be even more difficult to have this be performant.  Note that facebook uses a "static" URL for each of the panels so I'm reasonably confident it can be made to work.  If you believe you have requirements which go beyond what Facebook needs, it would be good if you could help us understand them so we can offer suggestions of potential changes on our side to accommodate them.
Whiteboard: [Fx17]
Updating the summary to be a little clearer.
OS: Mac OS X → All
Hardware: x86 → All
Summary: flyout empty on first load after startup → flyout very slow to load on first load after startup

Updated

6 years ago
Duplicate of this bug: 801495

Comment 9

6 years ago
I'll list our current uses of the flyout, perhaps this helps decide how it should be done. From my side, whether I have a static URL and use messages, or dynamic URLs (data: url perhaps) doesn't matter. I can easily adjust.

1. Sidebar control menu: This is essentially a popup menu offering selections of what to show in the sidebar (primarily which "group" to show). This is attached to a button in the sidebar and appears when pressed.

2. Article preview: We provide lists of articles and on hover we show a bit of textual preview with perhaps some extra information about the article. These currently have no interaction and are quickly replaced by a new info, or dismissed onmouseout.

3. Share details: When the user shares an article we show a share screen. There is one (may be more) buttons which can be pressed to reveal additional information about the article (like who tweeted it).
(Assignee)

Comment 10

6 years ago
Created attachment 689345 [details] [diff] [review]
preload proposal

Here is a patch that adds a loadPanel function.  This can be called from the sidebar to preload the panel.  Once the panel is loaded, the callback will be called, at which time the panel can be opened (if desired).
Attachment #689345 - Flags: feedback?(mhammond)
Attachment #689345 - Flags: feedback?(jaws)
Comment on attachment 689345 [details] [diff] [review]
preload proposal

Review of attachment 689345 [details] [diff] [review]:
-----------------------------------------------------------------

This implementation looks fine, but I'm wondering if we really should be changing the API incrementally without a bigger picture of other changes that might be desired?  For example, maybe we should insist on a single, fixed URL for panels (maybe from the manifest) which would avoid needing this specific change.
Attachment #689345 - Flags: feedback?(mhammond)
Comment on attachment 689345 [details] [diff] [review]
preload proposal

Same feedback as Mark, I think we should discuss this some more.
Attachment #689345 - Flags: feedback?(jaws)
(Assignee)

Comment 13

6 years ago
The primary requirement here is that we are able to have ui loaded prior to displaying the flyout.  You can see the issue and workaround that Facebook has done in their sidebar on the first open of the panel.  They quickly close the panel, wait for content to load, then show it again.  

We could do this via the openPanel api by delaying the actual open until the load callback happens, that would avoid the need for a new api.  However, being able to preload the panel can help avoid ui delay.  The downside of a preload is memory consumption when the panel may never be opened.

We shouldn't add anything new to the manifest, since that requires an update to the manifest to make changes.  At most, we could have a social.config message that sets what the panel url should be so we can preload, but that is unnecessary as well since we can do this via the api call.

If we're going to discuss in depth, lets to schedule that so this issue doesn't languish.
(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> If we're going to discuss in depth, lets to schedule that so this issue
> doesn't languish.

IIUC, you are considering a number of api-related changes - it probably makes sense for us to schedule some time when we can consider them all as a coherent set.  I don't think it makes much sense to consider individual ideas in isolation (which is why I created the [needs-api-change] whiteboard marker - ideally we should be looking at all such bugs together.)  We could even reduce the set further by ensuring bugs we want to consider as part of 1.5 have that [1.5] marker - currently there is only 1 bug with both annotations and this bug isn't it ;)
(Assignee)

Updated

6 years ago
Whiteboard: [1.5]
(Assignee)

Updated

6 years ago
Whiteboard: [1.5] → [1.5][needs-api-change]
(Assignee)

Comment 15

6 years ago
Based on irc conversation with markh; 

We'll provide a preloadPanel api that a provider can use to preload a url into the flyout.  This is useful for providers that will use a single url for the flyout and update the content (our recommended approach).  Using this, there shouldn't be any major delay when opening flyout panels.  

The openPlanel api will change slightly in that if the url needs to be loaded it will wait for document load before calling openPopup.  This will avoid a blank panel appearing.  Providers should indicate load with an animated icon or something.  We can later figure out how to indicate load in chrome if that is better ux.
(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> The openPlanel api will change slightly in that if the url needs to be
> loaded it will wait for document load before calling openPopup.

Just to clarify - the API need not change, only the implementation.

> Providers should indicate load with an
> animated icon or something.

Depending on the use-case, this might not be suitable - the load event must fire before any such icon would be seen (IIUC).  This *would* be fine if the content is fairly light-weight and all the heavy lifting is done via XHR - but I imagine providers which use a different URL each time the flyout is used probably isn't going to be setup to take advantage of this (ie, for those providers, once load fires it is ready to roll)

> We can later figure out how to indicate load in chrome if that is better ux.

I suspect we will end up needing this - it seems a poor UX if it takes a number of seconds from click to seeing anything happen.

TBH, I'm still not convinced we shouldn't simply force the provider to use the same URL for every flyout invocation - it is in our interests to prevent providers using the API in such a way that high latency is guaranteed.  However, Shane feels strongly that keeping the flexibility is important and I'm trying to pick my battles :)
(Assignee)

Comment 17

6 years ago
(In reply to Mark Hammond (:markh) from comment #16)

> > Providers should indicate load with an
> > animated icon or something.
> 
> Depending on the use-case, this might not be suitable - the load event must
> fire before any such icon would be seen (IIUC).  

I'm suggesting UI in the sidebar that opens the flyout, not ui in the flyout.  A nice usable example of this is the facebook sidebar.  With the delay in calling openPopup a provider has everything necessary to do this:

set loading icon
callback:
  unset loading icon
call openPanel with callback


As well, I dont think it is a requirement that a provider do this, but it will be better ux to indicate the load is in progress.
So instead of a load event here (and elsewhere), it sounds like we really just want the readystate event.  IMO it is probably fine to make the callback etc once the readystate hits either "interactive" or "complete" ("interactive" is probably OK as even though some images etc might still be coming in, the panel can be interacted with and people are probably quite used to seeing and interacting with stuff in this state)

Thoughts?
oops - comment 18 was probably more appropriate for bug 820718 than this one.
(Assignee)

Comment 20

6 years ago
Created attachment 694159 [details] [diff] [review]
preload and openPopup delay
Attachment #689345 - Attachment is obsolete: true
Attachment #694159 - Flags: feedback?(mhammond)
Attachment #694159 - Flags: feedback?(jaws)
Comment on attachment 694159 [details] [diff] [review]
preload and openPopup delay

Review of attachment 694159 [details] [diff] [review]:
-----------------------------------------------------------------

As I just mentioned, I think we should consider a readystate of "interactive" or "complete" is enough to fire the callback and open the panel (eg, the fact that not every persons profile image has been fetched and rendered need not hold up the panel opening).  Also, I think it is probably worth grabbing the tests from bug 820718 so we can ensure the semantics are what we are trying to achieve (and nail that bug at the same time).  But the general direction looks reasonable...
Attachment #694159 - Flags: feedback?(mhammond) → feedback+
(Assignee)

Comment 22

6 years ago
(In reply to Mark Hammond (:markh) from comment #21)
> Comment on attachment 694159 [details] [diff] [review]
> preload and openPopup delay
> 
> Review of attachment 694159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As I just mentioned, I think we should consider a readystate of
> "interactive" or "complete" is enough to fire the callback and open the
> panel (eg, the fact that not every persons profile image has been fetched
> and rendered need not hold up the panel opening).  Also, I think it is
> probably worth grabbing the tests from bug 820718 so we can ensure the
> semantics are what we are trying to achieve (and nail that bug at the same
> time).  But the general direction looks reasonable...

I tried that out, readystate == "complete" works, but "interactive" is too early. "interactive" happens after parsing, but at least facebook is depending on the callback happening after resources are loaded which maps to "complete".  

I don't want to combine bug 820718 with this because it is a different problem and needs a bit more thought.  I'll post an updated patch there along with my thoughts about it.
(Assignee)

Comment 23

6 years ago
Created attachment 694618 [details] [diff] [review]
preload and openPopup delay

modified to use readystate
Attachment #694159 - Attachment is obsolete: true
Attachment #694159 - Flags: feedback?(jaws)
Attachment #694618 - Flags: feedback?(mhammond)
Attachment #694618 - Flags: feedback?(jaws)
(In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> I tried that out, readystate == "complete" works, but "interactive" is too
> early. "interactive" happens after parsing

Hrm - so in reality there is probably no advantage to using readystate over load?

> but at least facebook is
> depending on the callback happening after resources are loaded which maps to
> "complete".  

Can you explain this a bit more - in what they are they depending on it?

> I don't want to combine bug 820718 with this because it is a different
> problem and needs a bit more thought.  I'll post an updated patch there
> along with my thoughts about it.

Can we at least get some tests (either via that bug or new ones) in with this patch?
(Assignee)

Comment 25

6 years ago
(In reply to Mark Hammond (:markh) from comment #24)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> > I tried that out, readystate == "complete" works, but "interactive" is too
> > early. "interactive" happens after parsing
> 
> Hrm - so in reality there is probably no advantage to using readystate over
> load?

Probably not, though it works with that as well as with load.

> Can you explain this a bit more - in what they are they depending on it?

They seem to be relying on javascript being loaded, or some dom being ready, during the callback.  If I use "interactive", their flyouts never show anything.

> > I don't want to combine bug 820718 with this because it is a different
> > problem and needs a bit more thought.  I'll post an updated patch there
> > along with my thoughts about it.
> 
> Can we at least get some tests (either via that bug or new ones) in with
> this patch?

See the other bug, I updated the patch.  IMO The existing tests for the flyout are enough to handle the changes here.
(In reply to Shane Caraveo (:mixedpuppy) from comment #25)
> > Hrm - so in reality there is probably no advantage to using readystate over
> > load?
> 
> Probably not, though it works with that as well as with load.

OK - sorry for flipping back and forth here, but IMO there is no advantage to us ending up with *both* load and readystate handlers that are basically doing the same thing.

> 
> > Can you explain this a bit more - in what they are they depending on it?
> 
> They seem to be relying on javascript being loaded, or some dom being ready,
> during the callback.  If I use "interactive", their flyouts never show
> anything.

Yeah, that makes sense.

> See the other bug, I updated the patch.  IMO The existing tests for the
> flyout are enough to handle the changes here.

I'm not so sure - in this bug we are subtly changing the semantics.  This implies to me the existing tests did *not* cover the old semantics, and obviously also do not cover the new semantics.

I'm honestly not *trying* to be contrary :)
Comment on attachment 694618 [details] [diff] [review]
preload and openPopup delay

Review of attachment 694618 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +486,5 @@
> +      iframe.setAttribute("src", aURL);
> +    } else {
> +      // we still need to set the src to trigger onhashchange for ref changes
> +      iframe.setAttribute("src", aURL);
> +      cb();

This should be:
> iframe.contentWindow.addEventListener("hashchange", function hashChange() {
>   event.target.removeEventListener("hashchange", hashChange);
>   cb();
> });
Attachment #694618 - Flags: feedback?(jaws) → feedback+
Comment on attachment 694618 [details] [diff] [review]
preload and openPopup delay

Review of attachment 694618 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +502,5 @@
>        this._createFrame();
>      panel.hidden = false;
>      let iframe = panel.firstChild;
>  
> +    this.load(aURL, function _doOpen() {

This callback function doesn't need a name.
(Assignee)

Comment 29

6 years ago
(In reply to Jared Wein [:jaws] from comment #27)
> Comment on attachment 694618 [details] [diff] [review]
> preload and openPopup delay
> 
> Review of attachment 694618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-social.js
> @@ +486,5 @@
> > +      iframe.setAttribute("src", aURL);
> > +    } else {
> > +      // we still need to set the src to trigger onhashchange for ref changes
> > +      iframe.setAttribute("src", aURL);
> > +      cb();
> 
> This should be:
> > iframe.contentWindow.addEventListener("hashchange", function hashChange() {
> >   event.target.removeEventListener("hashchange", hashChange);
> >   cb();
> > });

Setting the src here is a convenience if the panel uses hashes, but this code path doesn't *require* a hash change.

As well, per irc conversation with Mark, we're going to continue using load rather than readystate, I just haven't updated the patch yet.
(Assignee)

Updated

6 years ago
Assignee: nobody → mixedpuppy
(Assignee)

Comment 30

6 years ago
Created attachment 696771 [details] [diff] [review]
preload and openPopup delay

adds a test, reverts to using the load event.
Attachment #694618 - Attachment is obsolete: true
Attachment #694618 - Flags: feedback?(mhammond)
Attachment #696771 - Flags: review?(jaws)
Comment on attachment 696771 [details] [diff] [review]
preload and openPopup delay

Review of attachment 696771 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: browser/base/content/test/social/browser_social_flyout.js
@@ +21,5 @@
>    testOpenCloseFlyout: function(next) {
>      let panel = document.getElementById("social-flyout-panel");
> +    panel.addEventListener("popupshowing", function onShowing() {
> +      panel.removeEventListener("popupshowing", onShowing);
> +      is(panel.firstChild.contentDocument.readyState, "complete", "panel is loaded prior to showing");

Are we sure that this test would have failed before this patch? Maybe it would suffice if we made the test panel delay "complete" for a determinate amount of time?
(Assignee)

Comment 32

6 years ago
(In reply to Jared Wein [:jaws] from comment #31)
> Comment on attachment 696771 [details] [diff] [review]
> preload and openPopup delay
> 
> Review of attachment 696771 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: browser/base/content/test/social/browser_social_flyout.js
> @@ +21,5 @@
> >    testOpenCloseFlyout: function(next) {
> >      let panel = document.getElementById("social-flyout-panel");
> > +    panel.addEventListener("popupshowing", function onShowing() {
> > +      panel.removeEventListener("popupshowing", onShowing);
> > +      is(panel.firstChild.contentDocument.readyState, "complete", "panel is loaded prior to showing");
> 
> Are we sure that this test would have failed before this patch? Maybe it
> would suffice if we made the test panel delay "complete" for a determinate
> amount of time?

I ran the test without the rest of the patch, it does fail in that case.


happy new year!
Comment on attachment 696771 [details] [diff] [review]
preload and openPopup delay

Review of attachment 696771 [details] [diff] [review]:
-----------------------------------------------------------------

Happy new year to you too :)

Looking again at this, the code for "loadPanel" and for "openPanel" are near exact with the exception of the function being called in the last line. Part of me thinks it would be nice to factor that out, but I also think that can make the code harder to follow and will likely result in some awkwardly named function that does multiple things. Therefore, I think it's fine how it is.
Attachment #696771 - Flags: review?(jaws) → review+
(Assignee)

Comment 34

6 years ago
/me feeling lame, here we are happy new year'ing over bug reports.
(Assignee)

Updated

6 years ago
Depends on: 797209
(Assignee)

Comment 35

6 years ago
The patch bitrotted, and updating the patch reflects increased problems from bug 797209.  Making that a blocker to landing this patch.
(Assignee)

Comment 36

5 years ago
reviving this patch, but removing the api change
Summary: flyout very slow to load on first load after startup → load iframe prior to showing flyout
Whiteboard: [1.5][needs-api-change]
Version: 18 Branch → 24 Branch
(Assignee)

Comment 37

5 years ago
Created attachment 756694 [details] [diff] [review]
preload and openPopup delay

This patch delays openPopup until the page loads, avoiding a blank panel on first open.  This fixes a flickering issue with the panel on first open which can be seen prominently with Facebook.

The patch was already reviewed by felipe, but since I let it bitrot I thought I would re-request the review.  should be a quick one.
Attachment #696771 - Attachment is obsolete: true
Attachment #756694 - Flags: review?(felipc)
Comment on attachment 756694 [details] [diff] [review]
preload and openPopup delay

Review of attachment 756694 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +556,5 @@
>      let panel = this.panel;
>      if (!panel.firstChild)
>        this._createFrame();
>      panel.hidden = false;
>      let iframe = panel.firstChild;

do you need to remove this block of code above from open? :)
(Assignee)

Comment 40

5 years ago
Created attachment 756750 [details] [diff] [review]
preload and openPopup delay

a little cleanup per irc discussion
Attachment #756694 - Attachment is obsolete: true
Attachment #756694 - Flags: review?(felipc)
Attachment #756750 - Flags: review?(felipc)
Attachment #756750 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/50c0dccd3856
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
You need to log in before you can comment on or make changes to this bug.