767353 - Empty link properties for non-text links since seamonkey 2.10

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect)

Description

[Sergey]

Attachment: link-properties-missing.png

Seamonkey since v2.10 has empty link properties for non-text links (2.9.1 works, but 2.10b1 does not). Text links work fine.

Steps to reproduce:
1. Open http://www.seamonkey-project.org/
2. Right-click on the top-left logo and select "Properties"
3. See image properties, but NO link properties.

Attached screenshot should explain the issue.

It's not limited to images. Link properties of "Recommended" links on the right side of http://youtube.com/ are shown empty too.

Comment 1

[[e-mail]]

Regression from core bug 649599. I guess we'd better audit all calls to GetAttributeNS...

Comment 2

[Phoenix]

Confirming on trunk
User agent: Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/16.0 Firefox/16.0a1 SeaMonkey/2.13a1
Build identifier: 20120619003009

Comment 3

[Philip Chee]

> 13:58	fedorauser	NeilAway: Is there anything else I can help to fix that bug?
> 14:04	NeilAway	fedorauser: well, how's your JavaScript programming?
> 14:04	NeilAway	fedorauser: this line: http://mxr.mozilla.org/comm-central/source/suite/browser/metadata.js#243
> 14:05	NeilAway	fedorauser: it stopped working because the getAttributeNS now returns null for no attribute (but it still returns "" for an empty attribute)
> 14:05	NeilAway	fedorauser: whereas before bug 649599 it would return "" for either no or an empty attribute
> 14:06	fedorauser	(... != "" || ... != null)?
> 14:06	fedorauser	oops, (... != "" && ... != null)?
> 14:12	NeilAway	fedorauser: well, fortunately we have an operator that converts both "" and null to true
> 14:16	NeilAway	fedorauser: better still, the if statement accepts any value except undefined, null, NaN, 0 or false as true
> 14:16	NeilAway	fedorauser: see http://james.padolsey.com/javascript/truthy-falsey/
> 14:17	NeilAway	fedorauser: oh, I forgot ""
> 14:17	NeilAway	(rather important, of course!)
> 14:23	fedorauser	You mean just using: if (elem.getAttributeNS(...)) ? Hm... Can getAttributeNS return zero somehow?
> 14:25	RattyAway	fedorauser: attributes are strings.
> 14:26	fedorauser	from the look of line #243 I thought that should be: if (elem.getAttributeNS(XLinkNS, "href") )
> 14:27	fedorauser	And, yes, I think that looks better than my "(... != "" && ... != null)" :)
> 14:54	NeilAway	fedorauser: yeah that should work, it can only return a string or null, so you don't have to worry about false, undefined, 0 or NaN

> Regression from core bug 649599. I guess we'd better audit all calls to GetAttributeNS...
I just checked /suite/ that appears to be the only occurence of depending on GetAttributeNS() returning "" for no attribute.

Comment 4

[[e-mail]]

(In reply to Philip Chee from comment #3)
> > Regression from core bug 649599. I guess we'd better audit all calls to GetAttributeNS...
> I just checked /suite/ that appears to be the only occurrence of depending on
> GetAttributeNS() returning "" for no attribute.
Don't worry, I didn't spot switch (elem.getAttributeNS(XLinkNS,"show")) either.

Comment 5

[Sergey]

Attachment: Suggested patch

Attached patch fixed the problem for me.

Comment 6

[[e-mail]]

Comment on attachment 638812 [details] [diff] [review]
Suggested patch

[Approval Request Comment]
Regression caused by (bug #): 649599
User impact if declined: Wrong properties for links containing other elements
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Low
String changes made by this patch: None

Comment 7

[[e-mail]]

Pushed comm-central changeset ba0e8ddca94f.

Comment 8

[Philip Chee]

Comment on attachment 638812 [details] [diff] [review]
Suggested patch

a=me regression fix.

Comment 9

[[e-mail]]

Pushed comm-aurora changeset d977cf0ce73b.
Pushed comm-beta changeset bd4a603a168e.
I messed up the attribution on the c-a changeset though. Sorry about that.