Closed Bug 267664 Opened 20 years ago Closed 17 years ago

SVG <a> element hack is painfully broken - implement it properly

Categories

(Core :: SVG, defect, P1)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jay, Assigned: jwatt)

References

()

Details

(Whiteboard: [Hixie-P1])

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041102
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041102

events in anchor element are not handled

Reproducible: Always
Steps to Reproduce:
1.visit uri
2. mouse over graphic
3.

Actual Results:  
nothing

Expected Results:  
alert box should be raised

put the event capturing in a graphical element, and it is captured

by replacing

<a xlink:href="square.svg" onmouseover="onM(evt)" onmouseout="outM(evt)">

with 

<a xlink:href="http://www.peepo.co.uk/launch/love.svg"><polygon fill="blue"
stroke="none" points="5,10  145,10 115,75"  onmouseover="alert(90)"
onmouseout="outM()" /></a>
The XBL binding overrides the handlers...
Depends on: 267657
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [Hixie-P1]
Attachment #172982 - Attachment mime type: image/xml+svg → image/svg+xml
As the earliest open bug on the 'a' element, I'm hijacking this bug (widening its scope) for the work necessary to create a proper implementation of the element. I anticipate marking several bugs as dups of this one once I'm done.
Assignee: general → jwatt
Depends on: 334587
No longer depends on: 267657
OS: MacOS X → All
Hardware: Macintosh → All
Summary: anchor doesn't handle event → SVG <a> element hack is painfully broken - implement it properly
So this patch requires the patch in bug 334587, but given that patch, this is a fairly simple fix.

This patch also fixes at least six other bugs.
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 219096 [details] [diff] [review]
work in progress - works, but needs polished

Could you just ditch the svgBindings.xml file?
Not really. I have a separate patch that adds different stuff back to it. It's only empty for the purposes of this bug.
could someone preferably jwatt confirm that this bug is in fact resolved?

Once I have the latest version I shall endeavour to check for myself.
However the process seems faulty where this bug appears to be automatically resolved without patch or comment.

please excuse if this is merely a misunderstanding on my part.
Jonathan,

Well, I think it was a misunderstanding.  You need to carefully re-read the email you received (which I also received).  The email says that this Bug (267664) depends on Bug 334587 and that Bug 334587 has been closed, not Bug 267664.

Regards,
Jeff
This bug is not fixed (Jeff is right, it was bug 334587 that was fixed).
oops sorry, slightly hectic here at the moment.

thanks!
jwatt

why have these bugs not been marked as dependent?
or else wait until this one is resolved/

it's rather more difficult for others to check that they are actually resolved at a later date...
as for a start one has to remember...

sorry if this is yet another misunderstanding...
Attached patch patch (obsolete) — Splinter Review
Attachment #219096 - Attachment is obsolete: true
Attachment #250708 - Flags: superreview?
Attachment #250708 - Flags: review?(tor)
Attachment #250708 - Flags: superreview? → superreview?(jst)
Attachment #250708 - Flags: superreview?(jst) → superreview?(bugmail)
When checking in testcases, check in the ones from the duplicates too, please.
Flags: in-testsuite?
You added the new element at the end of the classinfo structure.  I think that binary compatibility note is just for stable branches; at least, we've been merrily adding new SVG elements to the current SVG chunk in the trunk without anyone screaming.

If you're removing the only binding in svgBindings.xml, shouldn't you remove the file entirely?

<svg:a> should respect a "transform" attribute set, but this patch won't do that because the frame constructed is a nsSVGGenericContainerFrame.  You can handle this on a followup patch if you'd like.
jwatt you might want to consider whether to add bug 366654 as a dup of this bug.

apologies if you already considered this.
jwatt you might want to consider whether to add Bug 275254 as a dup of this bug.

apologies if you already considered this.

please ignore comment #21 :-(
(In reply to comment #20)
> You added the new element at the end of the classinfo structure.  I think that
> binary compatibility note is just for stable branches

According to jst, binary compat is no longer necessary on trunk.

> If you're removing the only binding in svgBindings.xml, shouldn't you remove
> the file entirely?

See comment 7.

> <svg:a> should respect a "transform" attribute set, but this patch won't do
> that because the frame constructed is a nsSVGGenericContainerFrame.  You can
> handle this on a followup patch if you'd like.

Yeah, thanks for pointing that out. Due to my limited time atm I'd prefer just to get this in and let other people build on it.
Attachment #250708 - Attachment is obsolete: true
Attachment #251164 - Flags: superreview?(bugmail)
Attachment #251164 - Flags: review?(tor)
Attachment #250708 - Flags: superreview?(bugmail)
Attachment #250708 - Flags: review?(tor)
Attachment #251164 - Flags: review?(tor) → review+
Comment on attachment 251164 [details] [diff] [review]
patch - domclassinfo order changed

>+nsSVGAElement::IsLink(nsIURI** aURI) const
...
>+    if (aURI) {
>+      nsCOMPtr<nsIURI> baseURI = GetBaseURI();
>+      // Get absolute URI
>+      // XXX: should really be using href->GetStringValue(), but nsSVGElement::
>+      // ParseAttribute has set the nsAttrValue type to eSVGValue, so we need
>+      // to use the more expensive ToString (generates, rather than fetches).
>+      nsAutoString hrefStr;
>+      href->ToString(hrefStr);
>+      nsContentUtils::NewURIWithDocumentCharset(aURI, hrefStr,
>+                                                GetOwnerDoc(), baseURI);
>+      // must promise out param is non-null if we return true
>+      return !!*aURI;
>+    }
>+    return PR_TRUE;

IIRC we decided not to let the return value of IsLink depend on if aURI was non-null or not, no? In fact, when looking at the other IsLink implementations they assume that aURI is never null.

>Index: mozilla/dom/public/nsDOMClassInfoID.h
>@@ -234,16 +234,17 @@ enum nsDOMClassInfoID {
> 
>   eDOMClassInfo_BeforeUnloadEvent_id,
> 
> #ifdef MOZ_SVG
>   // The SVG document
>   eDOMClassInfo_SVGDocument_id,
> 
>   // SVG element classes
>+  eDOMClassInfo_SVGAElement_id,
>   eDOMClassInfo_SVGCircleElement_id,
>   eDOMClassInfo_SVGClipPathElement_id,

I can never remember what the last verdict is here, if it's ok to not add at the end or not. Please get jst to sign off on that this is ok before checking in.


>Index: mozilla/layout/svg/base/src/resources/content/svgBindings.xml

Is there a reason to keep this file around?

sr=sicking with that fixed and jsts approval for the enum change.
Attachment #251164 - Flags: superreview?(jonas) → superreview+
Good catch on aURI. I've fixed that. Regarding jst and svgBindings.xml, tor already commented on that. See my reply in comment 23. Thanks a lot sicking!

Jonathan, once a debug build has popped out you can double check the bugs marked as duplicates here.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
_debug_ build?? What's wrong with me today. I mean _nightly_ build of course.
#25 will do, cheers!
So now we have two almost, but not exactly the same implementations
for ::IsLink:
http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsSVGAElement.cpp#239
http://lxr.mozilla.org/seamonkey/source/content/xml/content/src/nsXMLElement.cpp#210

Could SVG elements inherit XMLElement and use nsXMLElement::IsLink (with some
minor changes). Or if not (for example if only nsSVGAElement should support XLink)
maybe there could be still some helper method to implement IsLink(...) for XLinks.
Sheesh! Some people are never happy. You finally finish a year long battle through a chain of bugs so you can finally implement one little element and all you get is "there's a little bit of code duplication here." :-) Didn't I pay for my sins by removing the considerably larger duplication from PostHandleEvent on my way to get to this point? ;-)

Seriously though, you can see the differences so if you want to make a patch to unify the tiny bit of duplication then feel free. For <svg:a> the type can be "simple", empty or omitted; for generic XML the type _must_ be set to "simple". (unless we decide to support XLink 1.1.) Also the way the href is fetched is different. (I'd be happy enough to have -moz-target added to SVG so ignore that difference.) Frankly I think you'd find the effort/gain balance isn't worth it, but if you think it is then go for it.
jwatt,

I've checked the dupes, verified one and reopened two.
don't feel able to verify this bug until those bugs are fixed.
oops verified two ~:"

anyway well on the way...
Blocks: 268135
Blocks: 578459
You need to log in before you can comment on or make changes to this bug.