Last Comment Bug 767353 - Empty link properties for non-text links since seamonkey 2.10
: Empty link properties for non-text links since seamonkey 2.10
Status: RESOLVED FIXED
[good first bug][mentor=Neil][lang=js...
:
Product: SeaMonkey
Classification: Client Software
Component: Page Info (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.13
Assigned To: Sergey
:
Mentors:
Depends on:
Blocks: 649599
  Show dependency treegraph
 
Reported: 2012-06-22 06:36 PDT by Sergey
Modified: 2012-07-04 12:29 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed


Attachments
link-properties-missing.png (64.53 KB, image/png)
2012-06-22 06:36 PDT, Sergey
no flags Details
Suggested patch (853 bytes, patch)
2012-07-03 11:23 PDT, Sergey
neil: review+
philip.chee: approval‑comm‑aurora+
philip.chee: approval‑comm‑beta+
Details | Diff | Review

Description Sergey 2012-06-22 06:36:25 PDT
Created attachment 635726 [details]
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 neil@parkwaycc.co.uk 2012-06-22 06:39:59 PDT
Regression from core bug 649599. I guess we'd better audit all calls to GetAttributeNS...
Comment 2 Phoenix 2012-06-22 06:43:52 PDT
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 2012-06-22 19:53:41 PDT
> 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 neil@parkwaycc.co.uk 2012-06-25 16:18:42 PDT
(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 2012-07-03 11:23:17 PDT
Created attachment 638812 [details] [diff] [review]
Suggested patch

Attached patch fixed the problem for me.
Comment 6 neil@parkwaycc.co.uk 2012-07-03 11:50:12 PDT
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 neil@parkwaycc.co.uk 2012-07-03 14:12:16 PDT
Pushed comm-central changeset ba0e8ddca94f.
Comment 8 Philip Chee 2012-07-04 08:37:44 PDT
Comment on attachment 638812 [details] [diff] [review]
Suggested patch

a=me regression fix.
Comment 9 neil@parkwaycc.co.uk 2012-07-04 12:29:28 PDT
Pushed comm-aurora changeset d977cf0ce73b.
Pushed comm-beta changeset bd4a603a168e.
I messed up the attribution on the c-a changeset though. Sorry about that.

Note You need to log in before you can comment on or make changes to this bug.