The default bug view has changed. See this FAQ.

Empty link properties for non-text links since seamonkey 2.10

RESOLVED FIXED in seamonkey2.13

Status

SeaMonkey
Page Info
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Sergey, Assigned: Sergey)

Tracking

Trunk
seamonkey2.13

SeaMonkey Tracking Flags

(seamonkey2.10 wontfix, seamonkey2.11 fixed, seamonkey2.12 fixed)

Details

(Whiteboard: [good first bug][mentor=Neil][lang=js][level=beginner])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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

5 years ago
Regression from core bug 649599. I guess we'd better audit all calls to GetAttributeNS...
Blocks: 649599

Comment 2

5 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Version: SeaMonkey 2.11 Branch → Trunk

Comment 3

5 years ago
> 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.
Whiteboard: [good first bug][mentor=Neil][lang=js][level=beginner]

Updated

5 years ago
Component: General → Page Info
QA Contact: general → page-info

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
Created attachment 638812 [details] [diff] [review]
Suggested patch

Attached patch fixed the problem for me.

Updated

5 years ago
Attachment #638812 - Flags: review?(neil)

Comment 6

5 years ago
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
Attachment #638812 - Flags: review?(neil)
Attachment #638812 - Flags: review+
Attachment #638812 - Flags: approval-comm-beta?
Attachment #638812 - Flags: approval-comm-aurora?

Comment 7

5 years ago
Pushed comm-central changeset ba0e8ddca94f.
Assignee: nobody → sergemp
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 8

5 years ago
Comment on attachment 638812 [details] [diff] [review]
Suggested patch

a=me regression fix.
Attachment #638812 - Flags: approval-comm-beta?
Attachment #638812 - Flags: approval-comm-beta+
Attachment #638812 - Flags: approval-comm-aurora?
Attachment #638812 - Flags: approval-comm-aurora+

Comment 9

5 years ago
Pushed comm-aurora changeset d977cf0ce73b.
Pushed comm-beta changeset bd4a603a168e.
I messed up the attribution on the c-a changeset though. Sorry about that.
status-seamonkey2.10: --- → wontfix
status-seamonkey2.11: --- → fixed
status-seamonkey2.12: --- → fixed
Target Milestone: --- → seamonkey2.13
You need to log in before you can comment on or make changes to this bug.