Last Comment Bug 427990 - (xlink-mathml) XLink href not working on MathML elements (including <mrow>)
(xlink-mathml)
: XLink href not working on MathML elements (including <mrow>)
Status: RESOLVED FIXED
[inbound][passed try]
: regression, testcase
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: P3 normal with 1 vote (vote)
: mozilla7
Assigned To: Frédéric Wang (:fredw)
:
Mentors:
: 448526 (view as bug list)
Depends on: 504324 510202
Blocks: mathml-2 355548 mathml-href
  Show dependency treegraph
 
Reported: 2008-04-09 07:30 PDT by distler
Modified: 2012-06-15 22:25 PDT (History)
19 users (show)
roc: wanted1.9.1+
hsivonen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (982 bytes, application/xhtml+xml)
2008-04-09 07:32 PDT, distler
no flags Details
Patch V1 (13.16 KB, patch)
2010-11-16 01:10 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V2 (22.18 KB, patch)
2010-11-18 10:15 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
testcase (1.63 KB, application/xhtml+xml)
2010-11-18 10:15 PST, Frédéric Wang (:fredw)
no flags Details
Patch V3 (22.75 KB, patch)
2011-01-02 07:45 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
reftests (2.15 KB, patch)
2011-01-02 07:53 PST, Frédéric Wang (:fredw)
roc: review+
Details | Diff | Splinter Review
Patch V3 - gecko changes (19.01 KB, patch)
2011-01-03 06:54 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V3 - chrome changes (3.94 KB, patch)
2011-01-03 06:55 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch - chrome changes (V4) (3.99 KB, patch)
2011-01-07 07:54 PST, Frédéric Wang (:fredw)
gavin.sharp: review-
Details | Diff | Splinter Review
testcase 2 (1.16 KB, application/xhtml+xml)
2011-01-10 03:12 PST, Frédéric Wang (:fredw)
no flags Details
Patch - gecko changes (V4) (19.00 KB, patch)
2011-01-10 03:14 PST, Frédéric Wang (:fredw)
roc: review+
jonas: review+
Details | Diff | Splinter Review
Patch - gecko changes (V5) (18.99 KB, patch)
2011-03-23 00:27 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
reftests (2.15 KB, patch)
2011-05-13 00:51 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch - gecko changes (V6) (18.90 KB, patch)
2011-05-13 00:52 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch - gecko changes (V7) (18.42 KB, patch)
2011-06-16 17:27 PDT, Bill Gianopoulos [:WG9s]
no flags Details | Diff | Splinter Review
Patch - gecko changes (V8) (18.55 KB, patch)
2011-06-18 16:18 PDT, Bill Gianopoulos [:WG9s]
jonas: review+
Details | Diff | Splinter Review
reftests - V2 (1.77 KB, patch)
2011-06-23 12:43 PDT, Frédéric Wang (:fredw)
roc: review+
Details | Diff | Splinter Review

Description distler 2008-04-09 07:30:56 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008033001 SeaMonkey/2.0a1pre
Build Identifier: 

The xlink:href attribute on an <mrow> element should produce a clickable hyperlink. It does, on release versions, but not in the current nightlies.

See the testcase.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Comment 1 distler 2008-04-09 07:32:02 PDT
Created attachment 314596 [details]
testcase
Comment 2 Karl Tomlinson (:karlt) 2008-04-09 21:47:32 PDT
Something changed between Beta 2 and Beta 3 that caused the element with the href attribute not to highlight.  (The link didn't work in Beta 2 but that was likely due to MathML frames then not knowing how wide they are.)
Comment 4 Karl Tomlinson (:karlt) 2008-05-05 15:36:41 PDT
Looks like bug 355548.
Comment 5 Karl Tomlinson (:karlt) 2008-05-06 00:59:48 PDT
Before bug 355548 landed, MathML elements were nsXMLElements.
After, MathML elements are nsMappedAttributeElement (which are nsStyledElements, which are nsGenericElements) and nsIDOMElements.
xlink was implemented on the nsXMLElements (through IsLink() at least), and so got lost.

Bug 211916 and bug 332773 are related here.
The last notable comment seems to be bug 332773 comment 44.
Comment 6 Karl Tomlinson (:karlt) 2008-06-23 11:06:05 PDT
MathML 2.0 recommends XLink:

  MathML, as an XML application, defines links by the use of the mechanism
  described in the W3C Recommendation "XML Linking Language"

  http://www.w3.org/TR/2003/REC-MathML2-20031021/chapter7.html#id.7.1.4.1

so we should restore this functionality.
Comment 7 David Carlisle 2008-07-18 09:52:17 PDT
I was about to report a bug with xlink/mathml but I think this may be the same issue, so adding a comment here.

Our site relies heavily on xlink  links.
For example links to #LINED in the text at

http://www.nag.co.uk/numeric/FL/manual/xhtml/D06/d06baf.xml#COORUS

these work in FF2 (and earlier versions) but in FF 3.0.1 the links no longer
activate on left menu button. They do activate on right button, but only offer to open in another tab/window which is pretty inconvenient.

David
Comment 8 Karl Tomlinson (:karlt) 2008-07-31 15:30:18 PDT
*** Bug 448526 has been marked as a duplicate of this bug. ***
Comment 9 Karl Tomlinson (:karlt) 2008-07-31 15:32:42 PDT
(In reply to comment #5)
> Bug 211916 and bug 332773 are related here.
> The last notable comment seems to be ...

Bug 211916 comment 44, i mean.

Comment 10 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2008-08-01 02:53:16 PDT
Tweaking summary to avoid duplicate reports.
Comment 11 distler 2009-11-08 19:19:03 PST
Why does right-clicking on an element with an xlink:href bring up a context menu containing "Open Link in New Window"/"Open Link in New Tab"?

Clearly, some of the functionality that used to be present before the change noted in comment 5 (for some reason, I am not allowed to access bug 355548) is still present. Just not left-clickability.
Comment 12 distler 2009-11-19 22:19:35 PST
Given that this bug has been open for a year and a half, perhaps a more workable strategy would be simply to implement the MathML3 @href attribute. That is now the recommended way to turn MathML elements into hyperlinks, anyway ...
Comment 13 Robert Longson 2010-02-23 21:41:23 PST
tooltips will need fixing too now as there needs to be a way to determine that the mathml element is a link for xlink:title display. See bug 51202 for details.
Comment 14 Frédéric Wang (:fredw) 2010-11-16 01:10:00 PST
Created attachment 490831 [details] [diff] [review]
Patch V1

Experimental patch, which restores the xlink features and partially add support for the MathML3 href.
Comment 15 Frédéric Wang (:fredw) 2010-11-18 10:15:02 PST
Created attachment 491565 [details] [diff] [review]
Patch V2
Comment 16 Frédéric Wang (:fredw) 2010-11-18 10:15:23 PST
Created attachment 491566 [details]
testcase
Comment 17 Frédéric Wang (:fredw) 2010-12-26 06:48:40 PST
Decreasing bug priorities, to follow David's definition:
http://dbaron.org/log/20090120-bug-priorities
(i.e. make these MathML bugs not blocking any release)
Comment 18 Frédéric Wang (:fredw) 2011-01-02 07:45:23 PST
Created attachment 500674 [details] [diff] [review]
Patch V3

Most of the code added in content/mathml/content/src/nsMathMLElement is copied from SVGAElement.

(note: "Ctrl + left click" to open an xlink in a new tab seems to be broken on trunk)
Comment 19 Frédéric Wang (:fredw) 2011-01-02 07:53:02 PST
Created attachment 500678 [details] [diff] [review]
reftests
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-02 13:50:28 PST
Please separate the chrome changes from the Gecko changes, I can only review the latter. Gavin Sharp can probably review the former.
Comment 21 Frédéric Wang (:fredw) 2011-01-03 06:54:31 PST
Created attachment 500796 [details] [diff] [review]
Patch V3 - gecko changes
Comment 22 Frédéric Wang (:fredw) 2011-01-03 06:55:30 PST
Created attachment 500797 [details] [diff] [review]
Patch V3 - chrome changes
Comment 23 Dão Gottwald [:dao] 2011-01-07 07:13:35 PST
Comment on attachment 500797 [details] [diff] [review]
Patch V3 - chrome changes

>+    // MathML href
>+    href = this.link.getAttribute("href");
>+    if (!href) {
>+	// XLink href
>+	href = this.link.getAttributeNS("http://www.w3.org/1999/xlink",
>+					"href");
>+    }

You're indenting with tabs, please use spaces only.
Also, I thought XLink wasn't supported anymore.
Comment 24 Anne (:annevk) 2011-01-07 07:39:49 PST
XLink should only work for SVG and only the subset that SVG requires.
Comment 25 David Carlisle 2011-01-07 07:53:48 PST
(In reply to comment #24)
> XLink should only work for SVG and only the subset that SVG requires.

why?

there is a LOT of existing content using xlink on mathml

http://dlmf.nist.gov/2.6

for example has a dozen or so instances, and the rest of dlmf thousands more.

Mathml3 specifies href as an alternative to xlink:href, but obviously most existing content is mathml2.
Comment 26 Frédéric Wang (:fredw) 2011-01-07 07:54:34 PST
Created attachment 501984 [details] [diff] [review]
Patch - chrome changes (V4)

MathML XLink is also still needed for backward compatibility with MathML2:

"To support backward compatibility, user agents that implement XML Linking in compound documents containing MathML 2 should continue to support the use of the xlink:href attribute in addition to supporting the href attribute."

http://www.w3.org/TR/MathML3/chapter6.html#interf.link
Comment 27 Dão Gottwald [:dao] 2011-01-07 08:09:55 PST
With XLink support being dropped, I don't think the MathML 2 spec referring to the XLink spec has relevance here.
Comment 28 David Carlisle 2011-01-07 08:15:28 PST
(In reply to comment #27)
> With XLink support being dropped, I don't think the MathML 2 spec referring to
> the XLink spec has relevance here.

and 12 years worth of existing content ?

It seems strange that html5 specifies bizarre rules about html elements in mathml apparently to support a miniscule number of sites found which were using invalid uses of html elements in math on the grounds that existing content should be preserved, yet there are serious suggestions that entire sites using valid markup should be dropped without hesitation?
Comment 29 Dão Gottwald [:dao] 2011-01-07 08:23:23 PST
(In reply to comment #28)
> (In reply to comment #27)
> > With XLink support being dropped, I don't think the MathML 2 spec referring to
> > the XLink spec has relevance here.
> 
> and 12 years worth of existing content ?

How do other browsers handle it?

It seems strange to speak of backward compatibility when XLink in MathML stopped working with Firefox 3 three years ago.

> It seems strange that html5 specifies bizarre rules about html elements in
> mathml apparently to support a miniscule number of sites found which were using
> invalid uses of html elements in math on the grounds that existing content
> should be preserved

I don't know anything about this, but the way you describe it sounds like a wrong tradeoff has been made.
Comment 30 David Carlisle 2011-01-07 08:30:42 PST
(In reply to comment #29)

> How do other browsers handle it?

IE/mathplayer can do links, other browsers never supported mathml in xhtml anyway.

> 
> It seems strange to speak of backward compatibility when XLink in MathML
> stopped working with Firefox 3 three years ago.

exactly, hence the age of this bug, which has been open for that long and it's been repeatedly acknowledged as a bug, now finally it's being fixed and people suggest that it shouldn't be fixed?

David
Comment 31 Dão Gottwald [:dao] 2011-01-07 08:40:28 PST
(In reply to comment #30)
> (In reply to comment #29)
> > How do other browsers handle it?
> 
> IE/mathplayer can do links, other browsers never supported mathml in xhtml
> anyway.

Doesn't seem like a must-have to me then.

> exactly, hence the age of this bug, which has been open for that long and it's
> been repeatedly acknowledged as a bug

... when MathML 2 was the latest recommendation and before bug 332773.
Comment 32 David Carlisle 2011-01-07 08:42:34 PST
(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)

> 
> It seems strange to speak of backward compatibility when XLink in MathML
> stopped working with Firefox 3 three years ago.

mathematicians are just patient. The sites have been broken since firefox 3 
(unless temporary xsl or javascript workarounds were added) but it was repeatedly indicated that it would be restored at some point, eg

http://lists.w3.org/Archives/Public/public-html/2009Apr/0063.html
or
http://www.devcomments.com/XPointer-is-dead-What-about-XLink-at10243.htm

or other places, just found those two via google now.
Comment 33 David Carlisle 2011-01-07 08:48:40 PST
(In reply to comment #31)

> Doesn't seem like a must-have to me then.

what possible logic could lead to the result that xlink be preserved in svg but not in mathml? The situations are similar and mathml has been supported in firefox a lot longer than svg (since before it was firefox)
Comment 34 distler 2011-01-07 08:49:36 PST
>It seems strange to speak of backward compatibility when XLink in MathML
stopped working with Firefox 3 three years ago.

If a bug has been open long-enough, it stops being a bug, and starts being a feature?

Seriously?

There is (as far as I can tell) no intention to drop support for MathML2-in-XHTML. So that means (minimally) support for @xlink:href on MathML elements.

It will be nice to have support for MathML3 (which, among other things, adds an @href attribute). But that's logically-independent of fixing this bug.

(Implementation-wise, it's not independent, as  -- if I understand correctly -- Frederic's patch implements both @xlink:href and @href.)

Since David C. is here, I suppose it's worth asking what the browser behaviour should be when BOTH attributes are present.
Comment 35 Dão Gottwald [:dao] 2011-01-07 08:57:08 PST
(In reply to comment #33)
> what possible logic could lead to the result that xlink be preserved in svg but
> not in mathml?

There's no href attribute in the SVG namespace, is there?

(In reply to comment #34)
> >It seems strange to speak of backward compatibility when XLink in MathML
> stopped working with Firefox 3 three years ago.
> 
> If a bug has been open long-enough, it stops being a bug, and starts being a
> feature?

This is cutting it too short. This bug was filed when MathML 2 was state of the art and when XLink was supported on any XML node.
Comment 36 David Carlisle 2011-01-07 09:00:04 PST
(In reply to comment #34)

> Since David C. is here, I suppose it's worth asking what the browser behaviour
> should be when BOTH attributes are present.

as noted in a comment in the patch:

+  if (href) {
+    // MathML href
+    // The REC says: "When user agents encounter MathML elements with both href
+    // and xlink:href attributes, the href attribute should take precedence."
Comment 37 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-01-07 09:03:54 PST
I thought the plan was to support xlink:href on MathML and SVG elements but on no other elements.

HTML5's MathML copy-paste-ready authoring compatibility story assumes that browsers support the href attribute in the XLink namespace on elements that are in the MathML namespace, so I think we should support it. We already have the parser side code in place waiting for this bug to get fixed.
Comment 38 distler 2011-01-07 09:04:56 PST
>This bug was filed when MathML 2 was state of the
art ...

It's STILL the state of the art, as NO browser currently implements the MathML3 Spec.

And, as far as I can tell, there's no suggestion that support for MathML2 be DROPPED.
Comment 39 Frédéric Wang (:fredw) 2011-01-07 09:11:36 PST
Just to clarify, this bug is for "XLink href" (as indicated in the title) not the generic XLink support which was dropped in bug 332773 (where it was acknowledged that XLink href is still important for MathML). The patches add no new features with respect to SVG XLink and most of the code added is only to make MathML linkable (required to support MathML3 href attribute). Supporting xlink:href on MathML element in addition to MathML3 href costs almost nothing more and this feature is still wanted as explained in comments above.
Comment 40 Bruce Miller 2011-01-07 10:01:56 PST
(In reply to comment #39)
>  Supporting
> xlink:href on MathML element in addition to MathML3 href costs almost nothing
> more and this feature is still wanted as explained in comments above.

Thank you!
Comment 41 Jonas Sicking (:sicking) PTO Until July 5th 2011-01-07 18:06:50 PST
My perception too was that there was a large enough body of documents out there that use xlinks in MathML that it was worth keeping/adding support for it in Firefox.

It sounds great if we additionally add support for a plain href attribute as to reduce namespace crazyness.

I would even be happy to put a warning in the console every time we hit a xlink:href attribute in mathml and say that it's deprecated and people should just remove the xlink: prefix. That way we can migrate over to using href and one day we can remove support.
Comment 42 Michael Kohlhase 2011-01-07 21:16:22 PST
(In reply to comment #41)
> My perception too was that there was a large enough body of documents out there
> that use xlinks in MathML that it was worth keeping/adding support for it in
> Firefox.
Just to add more data, there is a body of about half a million documents at http://arxmliv.kwarc.info that also have xlink:href, e.g. go to http://arxmliv.kwarc.info/retval_detail.php?retval=no_problems and click you way through to *.xhtml. Not released as the DLMF or indeed as well-known, but also nice. 

> 
> It sounds great if we additionally add support for a plain href attribute as to
> reduce namespace crazyness.
> 
> I would even be happy to put a warning in the console every time we hit a
> xlink:href attribute in mathml and say that it's deprecated and people should
> just remove the xlink: prefix. That way we can migrate over to using href and
> one day we can remove support.
Comment 43 Frédéric Wang (:fredw) 2011-01-09 03:57:47 PST
For rendering issues with underlined MathML, see bug 504324. Attachment 502339 [details] [diff] fixes the cases of MathML 3 href and XLink.
Comment 44 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-09 18:47:29 PST
Shouldn't the code in GetPopupLinkNode and GetLinkLocation just be calling nsIContent::GetHrefURI?
Comment 45 Frédéric Wang (:fredw) 2011-01-10 03:12:09 PST
Created attachment 502456 [details]
testcase 2

I've already mentioned that Ctrl+tab seems to be broken for XLinks on trunk. I've also just realized that the items in the menu of a SVG <a> have changed (I have only "bookmark" and "copy location").
Comment 46 Frédéric Wang (:fredw) 2011-01-10 03:14:20 PST
Created attachment 502457 [details] [diff] [review]
Patch - gecko changes (V4)
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-10 04:00:36 PST
Comment on attachment 502457 [details] [diff] [review]
Patch - gecko changes (V4)

r=me on the layout changes.

Need a content peer for the content changes.
Comment 48 Jonas Sicking (:sicking) PTO Until July 5th 2011-01-12 16:28:23 PST
Comment on attachment 502457 [details] [diff] [review]
Patch - gecko changes (V4)

r=me on the content parts
Comment 49 Bill Gianopoulos [:WG9s] 2011-02-04 05:14:30 PST
(In reply to comment #46)
> Created attachment 502457 [details] [diff] [review]
> Patch - gecko changes (V4)

This patch needs to be updated because of the change in the definition of strippedChars from bug 377392.
Comment 50 Frédéric Wang (:fredw) 2011-03-23 00:27:08 PDT
Created attachment 521126 [details] [diff] [review]
Patch - gecko changes (V5)

just resolving conflict as explained in comment 49...
Comment 51 :Ms2ger 2011-03-26 10:09:05 PDT
Comment on attachment 521126 [details] [diff] [review]
Patch - gecko changes (V5)

>+void
>+nsMathMLElement::GetLinkTarget(nsAString& aTarget)
>+{
>+  const nsAttrValue* target = mAttrsAndChildren.GetAttr(nsGkAtoms::target,
>+                                                        kNameSpaceID_XLink);
>+  if (target) {
>+    nsAutoString targetStr;
>+    target->ToString(targetStr); 
>+    aTarget.Assign(targetStr);
>+  }

Doesn't |target->ToString(aTarget)| work?
Comment 52 Frédéric Wang (:fredw) 2011-03-27 01:37:11 PDT
> Doesn't |target->ToString(aTarget)| work?

It does, thanks for pointing that. I've updated the patch to include this change as well as to fix conflicts with trunk. I guess more conflicts may come before the chrome changes are approved. I'll keep the patches up-to-date in my mercurial queues:

https://github.com/fred-wang/MozillaCentralPatches
Comment 53 Frédéric Wang (:fredw) 2011-04-28 07:58:50 PDT
(In reply to comment #20)
> Please separate the chrome changes from the Gecko changes, I can only review
> the latter. Gavin Sharp can probably review the former.

I see that Gavin's review queue is quite large... Is someone else able to review this patch?
Comment 54 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-28 08:32:42 PDT
I can review it. Can you explain why the frontend code needs to change to fix this regression? What codepath was taken in nsContextMenu.js previously such that this worked? Or did the context menu stuff never work?
Comment 55 Frédéric Wang (:fredw) 2011-04-28 09:07:30 PDT
Thanks. The patches fix the regression for XLink href (xlink:href) and also implement the new MathML 3 attribute href. The former is already used for SVG so no changes on the frontend are supposed to be needed for it (I haven't test without it, though). Comment 11 also seems to indicate that the frontend was already operational. I think the changes are only for the MathML 3 attribute href.
Comment 56 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-28 09:15:25 PDT
(In reply to comment #55)
> I think the changes are only for the MathML 3 attribute href.

Why do we want to add this support? Is it commonly used on the Web? Do other browsers support this functionality?
Comment 57 Frédéric Wang (:fredw) 2011-04-28 09:22:14 PDT
(In reply to comment #56)
> (In reply to comment #55)
> > I think the changes are only for the MathML 3 attribute href.
> 
> Why do we want to add this support? Is it commonly used on the Web? Do other
> browsers support this functionality?

It's the new way to do link in MathML and was introduced in the MathML 3 REC (xlink is kept for backward compatibility but is now deprecated, see discussion above). Other browsers don't have native support for MathML, but I think IE+MathPlayer and MathJax have support for this attribute.
Comment 58 Frédéric Wang (:fredw) 2011-05-13 00:51:23 PDT
Created attachment 532133 [details] [diff] [review]
reftests
Comment 59 Frédéric Wang (:fredw) 2011-05-13 00:52:20 PDT
Created attachment 532134 [details] [diff] [review]
Patch - gecko changes (V6)
Comment 60 Frédéric Wang (:fredw) 2011-05-13 00:58:12 PDT
Gavin, I don't know if that can help you to review the chrome changes, but I've updated the reviewed patches to make them compatible with trunk.
Comment 61 Bill Gianopoulos [:WG9s] 2011-06-02 15:18:55 PDT
The gecko part of this appears to have bit-rotted.  Errors from build using current trunk:

In file included from /home/wag/mozilla/mozilla2/content/mathml/content/src/nsMathMLElement.cpp:41:0:
/home/wag/mozilla/mozilla2/content/mathml/content/src/nsMathMLElement.h: In constructor ‘nsMathMLElement::nsMathMLElement(already_AddRefed<nsINodeInfo>)’:
/home/wag/mozilla/mozilla2/content/mathml/content/src/nsMathMLElement.h:62:62: error: no matching function for call to ‘mozilla::dom::Link::Link()’
/home/wag/mozilla/mozilla2/content/mathml/content/src/../../../base/src/Link.h:67:3: note: candidates are: mozilla::dom::Link::Link(mozilla::dom::Element*)
/home/wag/mozilla/mozilla2/content/mathml/content/src/../../../base/src/Link.h:58:1: note:                 mozilla::dom::Link::Link(const mozilla::dom::Link&)
make[8]: *** [nsMathMLElement.o] Error 1
Comment 62 Frédéric Wang (:fredw) 2011-06-02 22:42:11 PDT
Thank you Bill. It seems to be caused by the following changes:

http://hg.mozilla.org/mozilla-central/rev/678ae0f96b82

Link::Link requires to be initialized with an Element now. So doing

  nsMathMLElement(already_AddRefed<nsINodeInfo> aNodeInfo)
    : nsMathMLElementBase(aNodeInfo), Link(this),
      mIncrementScriptLevel(PR_FALSE)

should solve the problem.
Comment 63 Bill Gianopoulos [:WG9s] 2011-06-03 08:42:56 PDT
(In reply to comment #62)
> Thank you Bill. It seems to be caused by the following changes:
> 
> http://hg.mozilla.org/mozilla-central/rev/678ae0f96b82
> 
> Link::Link requires to be initialized with an Element now. So doing
> 
>   nsMathMLElement(already_AddRefed<nsINodeInfo> aNodeInfo)
>     : nsMathMLElementBase(aNodeInfo), Link(this),
>       mIncrementScriptLevel(PR_FALSE)
> 
> should solve the problem.

I can verify that your suggestion fixes the issue.  Builds including your patch queue plus this should be available soon at http://www.wg9s.com/mozilla/firefox/  The Linux 32-bit build is currently available Linux 64-bit as well as 32-bit Windows will be available as soon as the builds complete.
Comment 64 Bill Gianopoulos [:WG9s] 2011-06-03 11:24:10 PDT
(In reply to comment #63)
> (In reply to comment #62)
> > Thank you Bill. It seems to be caused by the following changes:
> > 
> > http://hg.mozilla.org/mozilla-central/rev/678ae0f96b82
> > 
> > Link::Link requires to be initialized with an Element now. So doing
> > 
> >   nsMathMLElement(already_AddRefed<nsINodeInfo> aNodeInfo)
> >     : nsMathMLElementBase(aNodeInfo), Link(this),
> >       mIncrementScriptLevel(PR_FALSE)
> > 
> > should solve the problem.
> 
> I can verify that your suggestion fixes the issue.  Builds including your
> patch queue plus this should be available soon at
> http://www.wg9s.com/mozilla/firefox/  The Linux 32-bit build is currently
> available Linux 64-bit as well as 32-bit Windows will be available as soon
> as the builds complete.

The Windows and Linux builds are now available at http://www.wg9s.com/mozilla/firefox/
Comment 65 Frédéric Wang (:fredw) 2011-06-03 11:37:26 PDT
@Bill:

OK, thank you for the information. I just sent the update to my patch queue. I made a build with the updated patch and tested it. Now, the MathML links are no longer colored in blue and the mouse cursor does not change when passing over a MathML link. The link and contextual menu seem to work fine, though. I don't remember having noticed such a regression in previous builds. Do you have the same problem and did you see this before?
Comment 66 Bill Gianopoulos [:WG9s] 2011-06-15 13:34:33 PDT
The gecko part of this seems to, once again, not apply cleanly today.
Comment 67 Frédéric Wang (:fredw) 2011-06-15 22:37:08 PDT
(In reply to comment #66)
> The gecko part of this seems to, once again, not apply cleanly today.

OK. I wanted to verify more precisely which part of the code changed and caused the regression I mentioned in comment 65 (BTW, it happens with your builds too). However, I don't think I will have time to work on this bug during the summer, so fixing MathML linking is unfortunately likely to be delayed again (anyway, it does not seem that the chrome changes will be approved soon).
Comment 68 Karl Tomlinson (:karlt) 2011-06-16 03:35:45 PDT
Do we absolutely have to have the browser code reviewed to land the Gecko code?
Comment 69 Frédéric Wang (:fredw) 2011-06-16 12:15:11 PDT
(In reply to comment #68)
> Do we absolutely have to have the browser code reviewed to land the Gecko
> code?

I think the xlink:href can work without it, but in that case the code for MathML3 href will be unused. Anyway, the Gecko patch needs to be updated now.
Comment 70 Bill Gianopoulos [:WG9s] 2011-06-16 12:29:06 PDT
(In reply to comment #67)
> (In reply to comment #66)
> > The gecko part of this seems to, once again, not apply cleanly today.
> 
> OK. I wanted to verify more precisely which part of the code changed and
> caused the regression I mentioned in comment 65 (BTW, it happens with your
> builds too). However, I don't think I will have time to work on this bug
> during the summer, so fixing MathML linking is unfortunately likely to be
> delayed again (anyway, it does not seem that the chrome changes will be
> approved soon).

Sorry I somehow missed the point in comment 65.  I will try to come up with a new gecko patch and attach it here and then work on figuring out the comment 65 issue.  Depending on my success in sorting this I might end up taking the bug, but am leaving it assigned to you for now.
Comment 71 Frédéric Wang (:fredw) 2011-06-16 12:59:14 PDT
> Sorry I somehow missed the point in comment 65.  I will try to come up with
> a new gecko patch and attach it here and then work on figuring out the
> comment 65 issue.  Depending on my success in sorting this I might end up
> taking the bug, but am leaving it assigned to you for now.

OK. Thank you very much for your help, Bill.
Comment 72 Bill Gianopoulos [:WG9s] 2011-06-16 17:27:21 PDT
Created attachment 539948 [details] [diff] [review]
Patch - gecko changes (V7)

Updated to current tip.
Comment 73 Bill Gianopoulos [:WG9s] 2011-06-16 17:36:02 PDT
Comment on attachment 539948 [details] [diff] [review]
Patch - gecko changes (V7)

Review of attachment 539948 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 74 Bill Gianopoulos [:WG9s] 2011-06-16 17:37:02 PDT
Asking for new review from sicking just in case because the code it conflicted with was from bug 659053.
Comment 75 Bill Gianopoulos [:WG9s] 2011-06-17 04:33:00 PDT
(In reply to comment #65)
> @Bill:
> 
> OK, thank you for the information. I just sent the update to my patch queue.
> I made a build with the updated patch and tested it. Now, the MathML links
> are no longer colored in blue and the mouse cursor does not change when
> passing over a MathML link. The link and contextual menu seem to work fine,
> though. I don't remember having noticed such a regression in previous
> builds. Do you have the same problem and did you see this before?

What is the purpose of this:
 Link::ResetLinkState(!!aNotify);

Does this not end up being the same as:

 Link::ResetLinkState(aNotify);

?

Specifically, the compiler optimization flags have been changed, if it was not before and the !! is expected to really do something, my guess is that it is now being optimized out.
Comment 76 Bill Gianopoulos [:WG9s] 2011-06-17 06:12:06 PDT
OK did more research into this.  Evidently !!aNotify is a trick to use in C to normalize non-zero boolean values since C had no boolean type.

In C++ !!aNotfy and (bool)aNotify seem to do the same thing.

Also, I think now that it is unlikely this is an issue from the Linux optimization change because it is happening in Windows builds as well.
Comment 77 Frédéric Wang (:fredw) 2011-06-17 06:18:58 PDT
Maybe you can compare the changes of nsMathMLElement in this patch with the current version of SVGAElement. You can also verify what happened in bug 598833.
Comment 78 Bill Gianopoulos [:WG9s] 2011-06-17 08:12:27 PDT
(In reply to comment #77)
> Maybe you can compare the changes of nsMathMLElement in this patch with the
> current version of SVGAElement. You can also verify what happened in bug
> 598833.

Thanks for the tip.  Perhaps between the 2 of us we can sort this all out.
Comment 79 Bill Gianopoulos [:WG9s] 2011-06-18 16:18:38 PDT
Created attachment 540276 [details] [diff] [review]
Patch - gecko changes (V8)

This fixes the issue from comment 65.

It turns out the previous patch only took into account the part 9 patch for bug 598833.  It neglected to add the RequestLinkStateUpdate required by the part 8 patch for that bug.  Doing so fixed the issue.
Comment 80 Frédéric Wang (:fredw) 2011-06-19 02:12:36 PDT
> It turns out the previous patch only took into account the part 9 patch for
> bug 598833.  It neglected to add the RequestLinkStateUpdate required by the
> part 8 patch for that bug.  Doing so fixed the issue.

Glad to hear you were able to find where the issue came from. I'll try the last version of the patch the next time I make build the sources.
Comment 81 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-21 14:06:14 PDT
Comment on attachment 501984 [details] [diff] [review]
Patch - chrome changes (V4)

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>   getLinkURL: function() {

>+    // MathML href
>+    href = this.link.getAttribute("href");
>+    if (!href) {
>+      // XLink href
>+      href = this.link.getAttributeNS("http://www.w3.org/1999/xlink",

This seems like it will produce the wrong result for XLinks that also have a non-xlink-namespaced "href" attribute, won't it? It's unfortunate that this code is separated out from the code that actually checks for NS_MathML (or that we have this code mirrored in nsContextMenu and browser.js, or that we even need to special case this handling in the front end at all rather than having s single "get the link/href" API at the Gecko level).

It would be good to get some kind of confirmation that we want to fix this bug at all from one of the relevant DOM peers (i.e. that the gecko changes will be accepted). There seems to be an unfortunate mixing of "fix a regression" and "implement some new functionality" in these patches, and I haven't really seen any strong arguments that justify adding additional complexity to code that's already kind of gross.
Comment 82 David Carlisle 2011-06-21 14:44:18 PDT
(In reply to comment #81)

> This seems like it will produce the wrong result for XLinks that also have a
> non-xlink-namespaced "href" attribute, won't it?

If a mathml element has both an href and xlink:href element then the xlink:href should be ignored and the href one used. Which is what I believe that code was doing?

http://www.w3.org/TR/MathML3/chapter6.html#interf.link

.....When user agents encounter MathML elements with both href and xlink:href attributes, the href attribute should take precedence.
Comment 83 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-22 15:13:26 PDT
Comment on attachment 540276 [details] [diff] [review]
Patch - gecko changes (V8)

Review of attachment 540276 [details] [diff] [review]:
-----------------------------------------------------------------

To start: this is ok with me, the patch looks great.

However, do we really want to launch all the complexity that comes with xlink upon mathml developers? Couldn't we simply allow a simple, unprefixed href attribute create a link? That way people don't have to mess around with namespaces, with xlink:type, xlink:show or xlink:actuate.

That will make things easier both for page authors and for us. We should of course also pitch this to the mathml WG, but in the meantime I think it's fine for us to experiment with.

xlink really is the only namespaced attributes out there, it'd simplify the web platform a whole lot if people didn't have to worry about the concept of namespaced attributes.
Comment 84 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-06-22 23:12:30 PDT
(In reply to comment #83)
> However, do we really want to launch all the complexity that comes with
> xlink upon mathml developers? Couldn't we simply allow a simple, unprefixed
> href attribute create a link?

I think we do want to support the simplest xlink:href case for MathML (like we do for SVG) in order to support existing content and existing authoring tools. According to the comments on the patch, the patch doesn't try to support the crazier XLink stuff.

It sucks that both MathML and SVG jumped on the XLink bandwagon initially. Both WGs have realized the error in their ways and are trying to steer away, but that doesn't change what content and tools are already out there.

I think it would be really nice to get the Gecko part landed even ahead of the chrome part so that it doesn't rot again.
Comment 85 Frédéric Wang (:fredw) 2011-06-23 01:39:46 PDT
If everybody agree, I suggest to land the Gecko part now, to close this bug and follow-up the chrome part in bug 534968. Also I like Jonas' proposal (in comment 41) about adding a console warning. I think we can open a bug for that, which will depend on bug 534968. If the Chrome peers still think MathML linking is a minor feature that do not justify adding code in the nsContextMenu.js & browser.js mess, then we can also open another bug to refactor the linking code. And we would make this block bug 534968. But, IMO the priority is to implement the MathML3 href and encourage people to use the syntax without xlink prefix ; not to clean up this code.
Comment 86 Bill Gianopoulos [:WG9s] 2011-06-23 12:32:13 PDT
(In reply to comment #85)
> If everybody agree, I suggest to land the Gecko part now, to close this bug

Remembering that the gecko part can't land without landing the patch on bug 504324.
Comment 87 Frédéric Wang (:fredw) 2011-06-23 12:43:48 PDT
Created attachment 541465 [details] [diff] [review]
reftests - V2

Removing the MathML3 href reftest.
Comment 88 Frédéric Wang (:fredw) 2011-06-23 12:50:12 PDT
The patches to checkin are:

attachment 505534 [details] [diff] [review]
attachment 540276 [details] [diff] [review]
attachment 541465 [details] [diff] [review]
Comment 89 Frédéric Wang (:fredw) 2011-06-23 14:21:02 PDT
I just tested the patches again. The Gecko part is actually enough to make the MathML3 href works, except that when you right click on such a link, the context menu does not contain all the link options (bookmark link, copy link location etc).

I still have the issue mentioned in comment 18.
Comment 90 Bill Gianopoulos [:WG9s] 2011-06-24 03:22:17 PDT
(In reply to comment #89)
> I just tested the patches again. The Gecko part is actually enough to make
> the MathML3 href works, except that when you right click on such a link, the
> context menu does not contain all the link options (bookmark link, copy link
> location etc).
> 
> I still have the issue mentioned in comment 18.

Which applies to SVG xlinks as well.
Comment 91 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-06-24 05:04:52 PDT
Does r=roc still apply to attachment 541465 [details] [diff] [review]?
Comment 93 Marco Bonardo [::mak] 2011-06-25 03:14:08 PDT
http://hg.mozilla.org/mozilla-central/rev/12294b20e4d9
http://hg.mozilla.org/mozilla-central/rev/0ba8f12ec350

the third changeset was for bug 504324 and I'll mark it there.
Comment 94 Frédéric Wang (:fredw) 2011-06-25 08:01:35 PDT
(In reply to comment #90)
> (In reply to comment #89)
> > I just tested the patches again. The Gecko part is actually enough to make
> > the MathML3 href works, except that when you right click on such a link, the
> > context menu does not contain all the link options (bookmark link, copy link
> > location etc).
> > 
> > I still have the issue mentioned in comment 18.
> 
> Which applies to SVG xlinks as well.

https://bugzilla.mozilla.org/show_bug.cgi?id=667195

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