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

Status: RESOLVED FIXED

(Core :: SVG, defect, P1)

Description

[jonathan chetwynd]

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>

Comment 1

[Boris Zbarsky [:bzbarsky]]

The XBL binding overrides the handlers...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase. Unhovering the green square should alert.

Comment 3

[Jonathan Watt [:jwatt]]

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.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: work in progress - works, but needs polished

Comment 5

[Jonathan Watt [:jwatt]]

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.

Comment 6

[[e-mail]]

Comment on attachment 219096 [details] [diff] [review]
work in progress - works, but needs polished

Could you just ditch the svgBindings.xml file?

Comment 7

[Jonathan Watt [:jwatt]]

Not really. I have a separate patch that adds different stuff back to it. It's only empty for the purposes of this bug.

Comment 8

[jonathan chetwynd]

please also see https://bugzilla.mozilla.org/show_bug.cgi?id=342058

Comment 9

[jonathan chetwynd]

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.

Comment 10

[Jeff Schiller]

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

Comment 11

[Jonathan Watt [:jwatt]]

This bug is not fixed (Jeff is right, it was bug 334587 that was fixed).

Comment 12

[jonathan chetwynd]

oops sorry, slightly hectic here at the moment.

thanks!

Comment 16

[jonathan chetwynd]

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...

Comment 18

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 19

[Boris Zbarsky [:bzbarsky]]

When checking in testcases, check in the ones from the duplicates too, please.

Comment 20

[tor]

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.

Comment 21

[jonathan chetwynd]

jwatt you might want to consider whether to add bug 366654 as a dup of this bug.

apologies if you already considered this.

Comment 22

[jonathan chetwynd]

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 :-(

Comment 23

[Jonathan Watt [:jwatt]]

Attachment: patch - domclassinfo order changed

(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.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 25

[Jonathan Watt [:jwatt]]

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.

Comment 26

[Jonathan Watt [:jwatt]]

_debug_ build?? What's wrong with me today. I mean _nightly_ build of course.

Comment 27

[jonathan chetwynd]

#25 will do, cheers!

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 29

[Jonathan Watt [:jwatt]]

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.

Comment 30

[jonathan chetwynd]

jwatt,

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

Comment 31

[jonathan chetwynd]

oops verified two ~:"

anyway well on the way...