362060 - Rustico style change broke install.js, severely for themes (Download error -228)

Status: RESOLVED FIXED

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

Description

[Giorgio Maone [:ma1]]

Nested <span>s broke install.js (for the second time, looking at the comments in code): the href attribute of the install link can't be properly retrieved because of the new DOM layout.

A JS error happens every time the "Install now" text (rather than its surrounding buttonish link) is clicked, and since this is probably the most instinctive user action, it produced the effect which caused Wladimir Palant to reopen bug 335794.

Patch following in minutes.

Comment 1

[Wladimir Palant]

Attachment: Proposed patch: ignore nesting level

Changed install.js not to rely on page structure - go up in the DOM until you find the actual link with the href attribute.

Sorry mao, only noticed that you wanted to write a patch for this after I was done.

Comment 2

[Giorgio Maone [:ma1]]

Comment on attachment 246764 [details] [diff] [review]
Proposed patch: ignore nesting level

Thanks Wladimir, I was updating my very old CVS tree in the meanwhile :)

Comment 3

[Wladimir Palant]

Attachment: Proposed patch: ignore nesting level

Argh, forgot that Internet Explorer doesn't support the "in" operator. Not that this code gets to run in Internet Explorer in the first place - event.target isn't supported (current version throws an exception, with this patch it will return prematurely and simply let the user download). Fixed this and tested in Internet Explorer as well this time. Note that there is still a JavaScript exception if you try to download extensions in Opera (InstallTrigger undefined).

Comment 4

[Giorgio Maone [:ma1]]

Attachment: Same as 246769 but cross-browser

(In reply to comment #3)

> Argh, forgot that Internet Explorer doesn't support the "in" operator. Not that
> this code gets to run in Internet Explorer in the first place - event.target
> isn't supported (current version throws an exception, with this patch it will
> return prematurely and simply let the user download). Fixed this and tested in
> Internet Explorer as well this time. Note that there is still a JavaScript
> exception if you try to download extensions in Opera (InstallTrigger
> undefined).

Early fall-back if you're concerned about cross-browser compatibility.
BTW, which IE version doesn't support the "in" operator?

Comment 5

[Wladimir Palant]

(In reply to comment #4)
> BTW, which IE version doesn't support the "in" operator?

5.0 doesn't.

Comment 6

[Giorgio Maone [:ma1]]

(In reply to comment #5)
> (In reply to comment #4)
> > BTW, which IE version doesn't support the "in" operator?
> 
> 5.0 doesn't.
>

MS stuff is always fun :)

According to MSDN, JScript should support ECMA "in" operator since IE 3.0:
http://msdn.microsoft.com/library/en-us/script56/html/js56jsoprIn.asp
http://msdn.microsoft.com/library/en-us/script56/html/js56jsoriversioninformation.asp

Their implementation looks buggy, though:
https://sourceforge.net/forum/forum.php?thread_id=1396543&forum_id=256492

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 246771 [details] [diff] [review]
Same as 246769 but cross-browser

That looks fine, r=shaver.

Comment 8

[Cameron]

This was checked in and tagged for staging.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #4)
> Created an attachment (id=246771) [edit]

I wonder why params is an Array instead of a plain Object. Not related to this bug though ...

Comment 10

[Giorgio Maone [:ma1]]

Attachment: Refactoring to take care of themes

The same style change had an even worse impact on themes, whose installation currently fails with a "228 download error" in the majority of cases (when user clicks on the text inside the button). This patch cures both the symptoms respecting the DRY principle.

Comment 11

[Giorgio Maone [:ma1]]

*** Bug 357205 has been marked as a duplicate of this bug. ***

Comment 12

[Giorgio Maone [:ma1]]

Attachment: Same as 246895 but diffed against 1.10

Diffed against 1.10 per Cameron request

Comment 13

[Mike Shaver (:shaver emeritus)]

Comment on attachment 246924 [details] [diff] [review]
Same as 246895 but diffed against 1.10

r=shaver, thanks a bunch.

Cam: commit, tag to staging, test the crap out of it, and comment here for prod-tag confirmation?  There's a dear.

Comment 14

[Giorgio Maone [:ma1]]

Attachment: Ooops

Correction to 1.12. Cameron, I had to do some cross/site magic to test this, it would be great if you confirmed *both* for extensions and themes (no JS errors at least) before committing. Thanks!!!

Comment 15

[Cameron]

(In reply to comment #14)
> Created an attachment (id=246947) [edit]
> Ooops
> 
> Correction to 1.12. Cameron, I had to do some cross/site magic to test this, it
> would be great if you confirmed *both* for extensions and themes (no JS errors
> at least) before committing. Thanks!!!
> 
Tested this and it appears to actually work :D

Comment 16

[Mike Shaver (:shaver emeritus)]

Comment on attachment 246947 [details] [diff] [review]
Ooops

r=shaver.

mao: can you write some sort of test (could use selenium or, really, anything!) to verify installation correctness for themes and extensions in a more mechanical way?  Clearly, we could use some better coverage here. :(

Comment 17

[Giorgio Maone [:ma1]]

(In reply to comment #16)
> (From update of attachment 246947 [details] [diff] [review] [edit])
> r=shaver.
> 
> mao: can you write some sort of test (could use selenium or, really, anything!)
> to verify installation correctness for themes and extensions in a more
> mechanical way?  Clearly, we could use some better coverage here. :(
>

I'll be happy to do it if you're not in a terrible hurry, because I'm under very heavy deadlines these days. Catch-up in IRC.

Comment 18

[Cameron]

This was checked in and tagged. Please verify once it reaches production.

Comment 19

[Robert Strong (they/them - no direct email)]

*** Bug 362252 has been marked as a duplicate of this bug. ***

Comment 20

[Robert Strong (they/them - no direct email)]

*** Bug 362306 has been marked as a duplicate of this bug. ***

Comment 21

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

*** Bug 357205 has been marked as a duplicate of this bug. ***