1083094 - Add events for content loaded when changing the category in preferences

Status: RESOLVED WORKSFORME

(Firefox :: Settings UI, defect)

Description

[Daniel Gherasim]

While working on mozmill automated tests for the in-content preferences, I often get failures when trying to change the category and access an element really fast.

This is the bug we filed for our tests: bug 1073483.

So what we need is event/events we could wait for that tell us the content of a category is completely loaded.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Jared, as we talked two days ago it would be great if we could get such an event. Right now this is kinda blocking us from getting in-content preferences tests implemented via Mozmill. Would there be a chance for you to get this added in the next time?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch (with actual patch)

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

remote: You can view the progress of your build at the following URL:
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dc0e05b09f13
remote: Alternatively, view them on TBPL (soon to be deprecated):
remote:   https://tbpl.mozilla.org/?tree=Try&rev=dc0e05b09f13

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8509005 [details] [diff] [review]
Patch

>+  let event = new CustomEvent("PreferencesSearchEvent", {});

Event names don't usually contain the term "event".

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 8509005 [details] [diff] [review]
> Patch
> 
> >+  let event = new CustomEvent("PreferencesSearchEvent", {});
> 
> Event names don't usually contain the term "event".

I used the same naming convention as http://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.js#323 ("AboutHomeSearchEvent"). I can remove the "Event" suffix though.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > Comment on attachment 8509005 [details] [diff] [review]
> > Patch
> > 
> > >+  let event = new CustomEvent("PreferencesSearchEvent", {});
> > 
> > Event names don't usually contain the term "event".
> 
> I used the same naming convention as
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/
> aboutHome.js#323 ("AboutHomeSearchEvent").

Which is not at all a convention. :) By the way, the second CustomEvent parameter is optional.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8509033 [details] [diff] [review]
Patch v1.1

It's not clear to me how you ended up implementing a search event while this bug was about category switching. gotoPref seems like the natural place for dispatching such an event. However, maybe mozmill could just listen to the select event?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Henrik, can you try listening for the 'select' event and then running your test after a setTimeout(0)? The setTimeout may be necessary to ensure that all 'select' listeners have completed before you run your mozmill test code.

Comment 12

[Andrei Eftimie]

Moving the flag over to Daniel since he has worked on this.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> Henrik, can you try listening for the 'select' event and then running your
> test after a setTimeout(0)? The setTimeout may be necessary to ensure that
> all 'select' listeners have completed before you run your mozmill test code.

Would you mind to be more specific in which kind of other select events we should expect and have to wait for? Maybe some pointers to the code might also be helpful. Thanks.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/preferences.js#41 shows an event listener being added to the `categories` element. You can add the same event listener, but you should run your test in a setTimeout to make sure that it is delayed until all of the listeners have completed.

> categories.addEventListener("select", function categoriesSelect() {
>   categories.removeEventListener("select", categoriesSelect);
>   setTimeout(function() {
>     // add test code here
>   }, 0);
> });

Does that help?

Comment 15

[Daniel Gherasim]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/
> in-content/preferences.js#41 shows an event listener being added to the
> `categories` element. You can add the same event listener, but you should
> run your test in a setTimeout to make sure that it is delayed until all of
> the listeners have completed.
> 
> > categories.addEventListener("select", function categoriesSelect() {
> >   categories.removeEventListener("select", categoriesSelect);
> >   setTimeout(function() {
> >     // add test code here
> >   }, 0);
> > });
> 
> Does that help?

Hi Jared. I think the real problem we've got in mozmill is related to the elements in the sync category. For the "Sign In", "Create Account" and "Using an older...|, the click event fails sometime if we do it really fast after accessing the category.

Basically we do
==== 
Access 'Sync' > Click on 'Sign In'/'Create Account'/... > wait for the new tab (fails sometime)
====
This is reproducible locally if I slow down my proccessor speed.

I believe that those are the only affected as I ran
===== 
Access 'category-privacy' > Click on 'Learn More' > wait for the new tab 
====
with no events waiting when switching the category and never failed in 100 runs.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

For the Sync pane, you need to add a "weave:service:ready" observer. You'll have to wait for the "weave:service:ready" notification before interacting with the items on the Sync pane.

browser/components/preferences/in-content/sync.js does this in init():
> let onReady = function () {
>   Services.obs.removeObserver(onReady, "weave:service:ready");
>   window.removeEventListener("unload", onUnload, false);
>   this._init();
> }.bind(this);
> Services.obs.addObserver(onReady, "weave:service:ready", false);

The other potential fix that we could do is to have preferences.js delay dispatching the "Initialized" event until it has observed the "weave:service:ready" notification, but I'm not sure what else that may impact.

Dao, what do you think about delaying the "Initialized" event?

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> For the Sync pane, you need to add a "weave:service:ready" observer. You'll
> have to wait for the "weave:service:ready" notification before interacting
> with the items on the Sync pane.

Is that event always sent when opening the category? I don't think so. AFAIK while working on the TPS tests, this event is sent once at the beginning when you actually have logged in. Is it also sent if you are not logged in? Also when we switch between categories, come back to Sync and the event is not getting send again, we will timeout waiting for it.

Comment 18

[Daniel Gherasim]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> For the Sync pane, you need to add a "weave:service:ready" observer. You'll
> have to wait for the "weave:service:ready" notification before interacting
> with the items on the Sync pane.
> 

So I see it works waiting for that event when we access about:preferences, but that doesn't help us here. As Henrik stated, it doesn't help us when switching to the sync category, stll fails. Maybe the elements are somehow loaded different then the other categories ? Given that the content depends on if we are logged in or not.

Comment 19

[Marco Mucci [:MarcoM]]

Hi Jared, can you mark this bug for QE verification.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> > For the Sync pane, you need to add a "weave:service:ready" observer. You'll
> > have to wait for the "weave:service:ready" notification before interacting
> > with the items on the Sync pane.
> 
> Is that event always sent when opening the category? I don't think so. AFAIK
> while working on the TPS tests, this event is sent once at the beginning
> when you actually have logged in. Is it also sent if you are not logged in?
> Also when we switch between categories, come back to Sync and the event is
> not getting send again, we will timeout waiting for it.

(In reply to Daniel Gherasim from comment #18)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> > For the Sync pane, you need to add a "weave:service:ready" observer. You'll
> > have to wait for the "weave:service:ready" notification before interacting
> > with the items on the Sync pane.
> > 
> 
> So I see it works waiting for that event when we access about:preferences,
> but that doesn't help us here. As Henrik stated, it doesn't help us when
> switching to the sync category, stll fails. Maybe the elements are somehow
> loaded different then the other categories ? Given that the content depends
> on if we are logged in or not.

So this is what needs to be done:
When opening the in-content preferences, you should wait for the "Initialized" event, which is sent after all of the panes are loaded. Further, you will need to wait for the "weave:service:ready" observer notification to know that the Sync pane has finished loading. This notification will fire whether the user is logged in to Sync or not.

After waiting for the above two events to happen (you can probably *just* wait for "weave:service:ready"), then you will need to listen for the "select" event on the #categories element. After the "select" event, the desired pane will be showing and you can interact with it.

The "weave:service:ready" notification will only fire once, when the in-content preferences are loaded. It will not fire again when the Sync pane is visited.

Does that help you?

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

mozmill test breakages don't normally cause backouts of patches, and are not run through TBPL. As such, the in-content prefs triage today decided to remove this as a blocker from shipping in-content preferences.

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> So this is what needs to be done:
> When opening the in-content preferences, you should wait for the
> "Initialized" event, which is sent after all of the panes are loaded.
> Further, you will need to wait for the "weave:service:ready" observer
> notification to know that the Sync pane has finished loading. This
> notification will fire whether the user is logged in to Sync or not.
True, this fires once you open the about:preferences the first time, waiting for this and for Initilized event in open method should be enough.
> After waiting for the above two events to happen (you can probably *just*
> wait for "weave:service:ready"), then you will need to listen for the
> "select" event on the #categories element. After the "select" event, the
> desired pane will be showing and you can interact with it.
Waiting only for the first two events when opening the preferences pane was enough for me.

> Does that help you?
Yes, I ran the testcase in a loop for 2000 times and didn't failed. 

Though this needs to be implemented in bug 1005811, I'll close this as Fixed and bug 1073483 as invalid.

Comment 23

[Dão Gottwald [:dao]]

Nothing landed here.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The proposed patch never landed because it was trying to do something else, as what I can read via the comments. So the way right now is somewhat a workaround but we could live with it, as it looks like. It may be good to have a dedicated event when panes are getting switched, but it looks like that beside our tests, no other add-on is affected yet. So not sure if having it makes sense.