267657 - clicking on link causes new window to open

Status: RESOLVED FIXED

(Core :: SVG, defect)

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

Any link when clicked on causes a new window to launch.
http://www.peepo.co.uk/launch/love.svg

This is specific to the first window opened when the application launches

Reproducible: Always
Steps to Reproduce:
1.launch application
2.visit uri
3. click on graphic link

Actual Results:  
new window opened

Expected Results:  
location of current window should have changed

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is because the binding for <svg:a> was erroneously changed to use "content"
instead of "_content".

But I have to ask... why are we handling the link behavior via the binding
(poorly, I must add)?  If the issue is that the back end doesn't assume
type="simple", then maybe we need to make that assumption in the back end?  If
that's a bad assumption for non-svg, then we either special-case it for svg or
we change the binding to set type="simple" in the constructor (well, off a
timer, I suppose).

Comment 2

[Jonathan Watt [:jwatt]]

I think Alex would be the person most likely to know why we handle the link
behavior via a binding.

Comment 3

[[e-mail]]

(In reply to comment #1)
>(poorly, I must add)?
Why was it using _content? Also IMHO the event should be click, not mousedown.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Those are parts of "poorly", yes.  Plus bug 267664, the fact that it doesn't
check our new-window prefs like the XLink code in nsXMLElement does, the lack of
support for actuate="onload", the fact that it puts text in the status bar with
a different priority than other links... That sort of thing.

Comment 5

[Hixie (not reading bugmail)]

This should IMHO be a blocker before SVG is enabled in release builds.

Comment 6

[tor]

Attachment: basic repair of binding (content -> _content)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Could we at least fix this to use the "click" event while we're here?  And
perhaps address the other issues with this code?  I'd really prefer we just fix
it to work right instead of wallpapering over some issues and leaving others...

Comment 8

[tor]

Attachment: as above, using click

I'm just doing tweaks to the binding because I'm not sure of how to
do the full fix, but would like something better than our current
behavior.

Comment 9

[Alex Fritze]

Comment on attachment 172975 [details] [diff] [review]
as above, using click

I agree this should be implemented properly, but until someone volunteers do to
a proper implementation, I guess there's no harm in fixing the current
implementation.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible better fix

This would work great, I think, if bug 211916 (in its SVG variant) did not
exist.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Nasty hack

This makes use of an nsXMLElement, since xlink works on those... Doesn't fix
all the deps of this bug (not sure why it doesn't fix the mouseout thing), but
fixes this bug and bug 273197.

Comment 12

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

Why isn't this simply implemented in C++? The HandleDOMEvent code could be moved
up from nsGenericHTMLElement to nsGenericElement, or to some utility class. And
then called by <html:a>, <html:area>, <xml xlink=> and <svg:a>.

Comment 13

[Alex Fritze]

Comment on attachment 172987 [details] [diff] [review]
Nasty hack

You don't need the bogus namespace, since the svg content factory creates
generic xml elements for unknown tags.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I want the bogus namespace so I can restrict the styling to that element.

And yes, sicking, this should be implemented in C++.  And xlink should work on
HTML elements too.  All of which is on my "to do sometime" list, but is not 1.8
material at this point, imo....

Comment 15

[Anne (:annevk)]

*** Bug 287701 has been marked as a duplicate of this bug. ***

Comment 16

[Anne (:annevk)]

Could the patch that changes "content" to "_content" be checked in perhaps?

Comment 17

[[e-mail]]

Comment on attachment 172987 [details] [diff] [review]
Nasty hack

I'm wondering what the intent of the stylesheet is... if you just want a nested
element you could use
<content>
  <xlink:xlink xbl:inherits="xlink:href" xlink:type="simple">
    <svg:g xbl:inherits=" ... ">
      <children/>
    </svg:g>
  </xlink:xlink>
</content>
(OK, xlink:xlink is just as bogus)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Aren't kids inserted via <children> styled by the scoped sheet?  So what I was
aiming for was having something that really wouldn't appear in <children> there...

Comment 19

[[e-mail]]

I realized you needed the scoped stylesheet to attach the binding to the bogus
node, but as that binding only appeared to exist to provide a second anonymous
node I didn't understand why the original binding couldn't specify both nodes.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  That was because I'm not very good with XBL?  ;)

Comment 21

[[e-mail]]

Attachment: Simplified hack (untested)

Comment 22

[tor]

That approach doesn't work because the xlink:xlink causes an nsInlineFrame to be
constructed, which SVG skips over during painting because it isn't a SVG child
frame.

Comment 23

[[e-mail]]

Attachment: Less simplified hack

Comment 24

[tor]

(In reply to comment #23)
> Created an attachment (id=183410) [edit]
> Less simplified hack

This doesn't work because the frames aren't being constructed for the link children.

Comment 25

[tor]

More details on it not working.  First, svgBindings.xml is now in
chrome://global/content/svg (-moz-binding).  Second, when that is corrected the
following assert happens:

###!!! ASSERTION: Unable to locate an XBL binding.: 'protoBinding', file
/home/tor/src/moz-trunk/mozilla/content/xbl/src/nsXBLService.cpp, line 886
Break: at file /home/tor/src/moz-trunk/mozilla/content/xbl/src/nsXBLService.cpp,
line 886

Comment 26

[[e-mail]]

Comment on attachment 183410 [details] [diff] [review]
Less simplified hack

Duh, yeah, I meant #xlink instead of #svg :-[

Comment 27

[tor]

Ok, that makes it a bit happier, though it still doesn't work.  There's still a
Inline(xlink) frame in the heirarchy which cause the svg painting to ignore the
<a> content.  Heirarchy is now:

SVGGenericContainer(a)
  Inline(xlink)
    SVGG(g)
      SVGEllipse(ellipse)

Comment 28

[[e-mail]]

Comment on attachment 183410 [details] [diff] [review]
Less simplified hack

Would you mind trying extends="svg:generic" instead of display="svg:generic" ?

Comment 29

[tor]

Attachment: patched version of "less simplified hack"

Yes, that worked.  Attaching corrected version of your patch.

Comment 30

[[e-mail]]

Comment on attachment 183440 [details] [diff] [review]
patched version of "less simplified hack"

Don't forget that I've been attaching -w versions for easy review, so you don't
see the indent on the svg:g or the children.

Comment 31

[tor]

Attachment: version with indent

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 183526 [details] [diff] [review]
version with indent

If it works, r=bzbarsky

Comment 33

[Asa Dotzler [:asa]]

Comment on attachment 183526 [details] [diff] [review]
version with indent

a=asa

Comment 34

[tor]

Checked in.  Thanks for your help with this, Neil.