360220 - readers menulist has no icon if the last-selected reader is a web reader

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Asaf Romano (gone)]

regression from bug 358878: the readers menulist has no icon if the last-selected reader is a web reader.

This is happening because we set the icon of the web reader after it has been selected.

Comment 1

[Asaf Romano (gone)]

Attachment: patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 245186 [details] [diff] [review]
patch

I don't really understand why this is needed. Is the problem that FeedWriter.js sets the attribute after inserting the element into the document? If so, can't it wait to appendChild until after the icon load?

Comment 3

[Asaf Romano (gone)]

That's the problem, yes, but i don't think we should let the user wait until we have the icon (Plus I think this is something we want menulist to do regardless of this bug).

Comment 4

[Asaf Romano (gone)]

And as discussed on IRC, the 1.8.1.x patch is going to be less generic.

Comment 5

[Daniel Veditz [:dveditz]]

Don't regress this if bug 358878 ends up on the branch

Comment 6

[Daniel Veditz [:dveditz]]

"fixed" on the 1.8.1.1 by not causing the regression in bug 358878's patch -- need QA to verify.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Steps to reproduce (new profile):
1) Visit http://planet.mozilla.org/atom.xml (or some other feed)
2) Choose one of the web readers from the dropdown list (e.g. My Yahoo)
3) Click Subscribe Now
4) Click back OR load a feed in a new tab

Expected results: The reader selected at 2 should be selected in the dropdown, and it should have an icon.

Actual results with builds affected by this bug: The reader is selected, but doesn't have an icon. It does have an icon in the dropdown, and switching to another reader and then back brings back the icon.

Comment 8

[Asaf Romano (gone)]

Attachment: unit test

Comment 9

[Neil Deakin]

Comment on attachment 248094 [details] [diff] [review]
unit test

OK, I guess.

Comment 10

[Asaf Romano (gone)]

mozilla/toolkit/content/widgets/menulist.xml 1.26
mozilla/testing/mochitest/tests/test_bug360220.xul 1.1

Comment 11

[Robert Sayre]

Attachment: move the test into toolkit

Comment 12

[Asaf Romano (gone)]

Comment on attachment 256232 [details] [diff] [review]
move the test into toolkit

r=mano.

Comment 13

[Robert Sayre]

/cvsroot/mozilla/toolkit/content/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/toolkit/content/tests/widgets/Makefile.in,v
done
Checking in widgets/Makefile.in;
/cvsroot/mozilla/toolkit/content/tests/widgets/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/content/tests/widgets/test_bug360220.xul,v
done
Checking in widgets/test_bug360220.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_bug360220.xul,v  <--  test_bug360220.xul
initial revision: 1.1
done