The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
SVG
P1
normal
RESOLVED FIXED
13 years ago
7 years ago

People

(Reporter: jonathan chetwynd, Assigned: jwatt)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P1], URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

13 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [Hixie-P1]
Created attachment 172982 [details]
Testcase.  Unhovering the green square should alert.
Attachment #172982 - Attachment mime type: image/xml+svg → image/svg+xml
(Assignee)

Comment 3

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

Comment 4

11 years ago
Created attachment 219096 [details] [diff] [review]
work in progress - works, but needs polished
(Assignee)

Comment 5

11 years ago
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 6

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

Could you just ditch the svgBindings.xml file?
(Assignee)

Comment 7

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

Comment 8

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

(Reporter)

Comment 9

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

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

Comment 11

10 years ago
This bug is not fixed (Jeff is right, it was bug 334587 that was fixed).
(Reporter)

Comment 12

10 years ago
oops sorry, slightly hectic here at the moment.

thanks!
(Assignee)

Updated

10 years ago
Duplicate of this bug: 268135
(Assignee)

Updated

10 years ago
Duplicate of this bug: 316248
(Assignee)

Updated

10 years ago
Duplicate of this bug: 317270
(Reporter)

Comment 16

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

Updated

10 years ago
Duplicate of this bug: 320724
(Assignee)

Comment 18

10 years ago
Created attachment 250708 [details] [diff] [review]
patch
Attachment #219096 - Attachment is obsolete: true
Attachment #250708 - Flags: superreview?
Attachment #250708 - Flags: review?(tor)
(Assignee)

Updated

10 years ago
Attachment #250708 - Flags: superreview? → superreview?(jst)
(Assignee)

Updated

10 years ago
Attachment #250708 - Flags: superreview?(jst) → superreview?(bugmail)
When checking in testcases, check in the ones from the duplicates too, please.
Flags: in-testsuite?

Comment 20

10 years ago
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.
(Reporter)

Comment 21

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

apologies if you already considered this.
(Reporter)

Comment 22

10 years ago
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 :-(
(Assignee)

Comment 23

10 years ago
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.
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)

Updated

10 years ago
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+
(Assignee)

Comment 25

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 26

10 years ago
_debug_ build?? What's wrong with me today. I mean _nightly_ build of course.
(Reporter)

Comment 27

10 years ago
#25 will do, cheers!

Comment 28

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

Comment 29

10 years ago
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.
(Reporter)

Comment 30

10 years ago
jwatt,

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

Comment 31

10 years ago
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.