Last Comment Bug 267664 - SVG <a> element hack is painfully broken - implement it properly
: SVG <a> element hack is painfully broken - implement it properly
Status: RESOLVED FIXED
[Hixie-P1]
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: P1 normal with 4 votes (vote)
: ---
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
: Hixie (not reading bugmail)
Mentors:
http://www.peepo.co.uk/temp/anchor-sc...
: 316248 320724 (view as bug list)
Depends on: 334587
Blocks: 268135 578459
  Show dependency treegraph
 
Reported: 2004-11-04 04:11 PST by jonathan chetwynd
Modified: 2010-07-13 13:31 PDT (History)
10 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase. Unhovering the green square should alert. (477 bytes, image/svg+xml)
2005-01-31 10:27 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details
work in progress - works, but needs polished (22.64 KB, patch)
2006-04-19 20:58 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details | Diff | Review
patch (22.49 KB, patch)
2007-01-06 12:43 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details | Diff | Review
patch - domclassinfo order changed (23.20 KB, patch)
2007-01-11 03:02 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
tor: review+
jonas: superreview+
Details | Diff | Review

Description jonathan chetwynd 2004-11-04 04:11:57 PST
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 [:bz] (Out June 25-July 6) 2004-11-14 23:45:13 PST
The XBL binding overrides the handlers...
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-01-31 10:27:55 PST
Created attachment 172982 [details]
Testcase.  Unhovering the green square should alert.
Comment 3 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2006-04-19 18:04:43 PDT
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] (Away Jun. 27 - Jul. 13) 2006-04-19 20:58:38 PDT
Created attachment 219096 [details] [diff] [review]
work in progress - works, but needs polished
Comment 5 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2006-04-19 21:01:33 PDT
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 neil@parkwaycc.co.uk 2006-04-20 03:35:50 PDT
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] (Away Jun. 27 - Jul. 13) 2006-04-20 03:47:14 PDT
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 2006-06-19 21:50:49 PDT
please also see https://bugzilla.mozilla.org/show_bug.cgi?id=342058

Comment 9 jonathan chetwynd 2007-01-04 14:25:50 PST
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 2007-01-04 14:34:35 PST
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] (Away Jun. 27 - Jul. 13) 2007-01-04 15:30:15 PST
This bug is not fixed (Jeff is right, it was bug 334587 that was fixed).
Comment 12 jonathan chetwynd 2007-01-05 08:37:59 PST
oops sorry, slightly hectic here at the moment.

thanks!
Comment 13 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2007-01-06 08:48:38 PST
*** Bug 268135 has been marked as a duplicate of this bug. ***
Comment 14 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2007-01-06 09:13:49 PST
*** Bug 316248 has been marked as a duplicate of this bug. ***
Comment 15 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2007-01-06 09:45:30 PST
*** Bug 317270 has been marked as a duplicate of this bug. ***
Comment 16 jonathan chetwynd 2007-01-06 09:54:35 PST
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 17 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2007-01-06 10:07:24 PST
*** Bug 320724 has been marked as a duplicate of this bug. ***
Comment 18 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2007-01-06 12:43:01 PST
Created attachment 250708 [details] [diff] [review]
patch
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-01-07 18:36:27 PST
When checking in testcases, check in the ones from the duplicates too, please.
Comment 20 tor 2007-01-08 14:58:17 PST
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 2007-01-11 00:45:27 PST
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 2007-01-11 00:47:30 PST
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] (Away Jun. 27 - Jul. 13) 2007-01-11 03:02:19 PST
Created attachment 251164 [details] [diff] [review]
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) PTO Until July 5th 2007-01-26 13:44:18 PST
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] (Away Jun. 27 - Jul. 13) 2007-01-26 15:50:23 PST
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] (Away Jun. 27 - Jul. 13) 2007-01-26 15:51:41 PST
_debug_ build?? What's wrong with me today. I mean _nightly_ build of course.
Comment 27 jonathan chetwynd 2007-01-27 01:15:36 PST
#25 will do, cheers!
Comment 28 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-01-27 02:14:00 PST
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] (Away Jun. 27 - Jul. 13) 2007-01-27 03:20:03 PST
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 2007-02-01 06:04:40 PST
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 2007-02-01 06:05:42 PST
oops verified two ~:"

anyway well on the way...

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