575241 - Install buttons for extensions and themes are lazily initialized (sometimes stay in download state)

Status: RESOLVED WONTFIX

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

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

With the release of AMO 5.11.2 last week we now lazily initialize the install buttons on AMO. Nearly all the time you can see that the "Download" button is shown for a split of a second before it turns into "Add to Firefox".

Since that change has been pushed our Mozmill tests are broken for extension and theme installation. On Saturday I have also run those tests manually and one time the button never turned into "Add to Firefox" for the Walnut theme. A reload of the page solved that problem.

List of all the 5.11.2 bug fixes:
https://bugzilla.mozilla.org/buglist.cgi?resolution=FIXED&classification=Server%20Software&query_format=advanced&product=addons.mozilla.org&target_milestone=5.11.2

Steps:
1. Open the AMO page for Walnut
2. Check the install button state

Comment 1

[Jeff Balogh (:jbalogh)]

We start out with Download Now to handle the no-js case.  We don't know your platform/browser/version until we start running javascript on the page.  This would go away (and look nicer in general) if we started out with Add to Firefox, but that's a product decision. => fligtar

Comment 2

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

It would be nice to have at least an attribute set for the uninitialized case, so external applications can distinct between those two states. Right now I can't find such an attribute.

Stephen, wouldn't Selenium have the same problem with existing tests we have?

Comment 3

[Mike Shaver (:shaver emeritus)]

You could run JS and then document.write a stylesheet rule in the head, which would block the parser and avoid a flash or user-visible transition, too.

Comment 4

[u279076]

Henrik and I discussed a workaround to this issue for Mozmill.  Aaron will create a patch for that workaround and attach it here.

Comment 5

[Aaron Train [:aaronmt]]

Attachment: Temporary workaround: sleep

I've attached a temporary workaround as Anthony requested

Comment 6

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

(In reply to comment #4)
> Henrik and I discussed a workaround to this issue for Mozmill.  Aaron will
> create a patch for that workaround and attach it here.

Eventually we do not need the workaround for Mozmill. I have checked it once again and the class names of the install button link itself can help us here. We would simply have to wait for "add installer".

The issue for normal users will still remain.

Comment 7

[u279076]

Comment on attachment 454600 [details] [diff] [review]
Temporary workaround: sleep

Patch looks good.  Over to you Henrik.  Should Aaron add the button class specifier checking to this patch?

Comment 8

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

Comment on attachment 454600 [details] [diff] [review]
Temporary workaround: sleep

> (From update of attachment 454600 [details] [diff] [review])
> Patch looks good.  Over to you Henrik.  Should Aaron add the button class
> specifier checking to this patch?

As already said on IRC, yes. AMO will probably not change those class names so we have to use those instead of silly sleep calls.

Comment 9

[Aaron Train [:aaronmt]]

Attachment: Patch - Wait for class change on link

- sleeps
+ Wait for class change on the download link

Comment 10

[u279076]

Comment on attachment 454642 [details] [diff] [review]
Patch - Wait for class change on link

>+  // XXX: Bug 575241
>+  // AMO Lazy install buttons: wait for class change
>+  var downloadLink = new elementslib.XPath(controller.tabs.activeTab,
>+                                          "//div[@id='addon-summary']/div/div/div/p/a");
Even though in code this is a link, in UI and UX it is a button.  Please rename this to installAddonButton (or something like that).

>+ 
>+  controller.waitForEval("subject.installDownloadClass > -1", gTimeout, 100, 
>+                        {installDownloadClass: downloadLink.getNode().getAttribute('class').indexOf("installer")});
Since this is lazy, we may need to do the comparison in the "".  In other words:

subject.installAddonButtonClass.indexOf('installer') > -1, TIMEOUT, 100,
{installAddonButtonClass: installAddonButton.getNode()});

Henrik, please advise.

Comment 11

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

Comment on attachment 454642 [details] [diff] [review]
Patch - Wait for class change on link

(In reply to comment #10)
> Since this is lazy, we may need to do the comparison in the "".  In other
> words:
> 
> subject.installAddonButtonClass.indexOf('installer') > -1, TIMEOUT, 100,
> {installAddonButtonClass: installAddonButton.getNode()});

You missed the ".getAttribute('class')" part. But yes, it should be done inside the eval expression. Also please use "!= -1".

Comment 12

[Aaron Train [:aaronmt]]

Attachment: Patch v1.1 - Wait for class change on link

Changes as per requested, let's try this again.

Comment 13

[Aaron Train [:aaronmt]]

Attachment: Patch v1.2 - Wait for class change on button

Forgot a contributor line

Comment 14

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

Comment on attachment 454708 [details] [diff] [review]
Patch v1.2 - Wait for class change on button

>+  controller.waitForEval("subject.installAddonButtonClass.indexOf('prominent') != -1", TIMEOUT, 100, 

I'm not sure why you have changed all instances to use 'prominent' now. We have used 'installer' before which was fine. But this change will definitely break all of our tests.

I will update all instances to use 'installer' again and check it in. Next time please also run negative tests (i.e. disabling Javascript in that case).

Comment 15

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

Landed on default and mozilla1.9.2:
http://hg.mozilla.org/qa/mozmill-tests/rev/08819ecee03e
http://hg.mozilla.org/qa/mozmill-tests/rev/f68b84d6e37d

Please come up with a patch for mozilla1.9.1 too. Thanks.

Comment 16

[Wil Clouser [:clouserw]]

Thanks for filing this.  Due to resource constraints we are closing bugs which we won't realistically be able to fix.  If you have a patch that applies to this bug please reopen.

For more info see http://micropipes.com/blog/2014/09/24/the-great-add-on-bug-triage/