Bug 427990 - (xlink-mathml) XLink href not working on MathML elements (including <mrow>)
 Status: RESOLVED FIXED [inbound][passed try] regression, testcase Core Components MathML (show other bugs) Trunk All All P3 normal with 1 vote (vote) mozilla7 Frédéric Wang (:fredw) Anthony Jones (:kentuckyfriedtakahe, :k17e) (view as bug list) 504324 510202 mathml-2 355548 mathml-href Show dependency tree / graph

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

 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 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. distler 2008-04-09 07:32:02 PDT Created attachment 314596 [details] testcase 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.) Karl Tomlinson (:karlt) 2008-05-05 15:06:05 PDT 2008-01-08-04 works. 2008-01-09-04 doesn't. http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-01-08+03%3A00%3A00&maxdate=2008-01-09+05%3A00%3A00&cvsroot=%2Fcvsroot Karl Tomlinson (:karlt) 2008-05-05 15:36:41 PDT Looks like bug 355548. 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. 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. 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  Karl Tomlinson (:karlt) 2008-07-31 15:30:18 PDT *** Bug 448526 has been marked as a duplicate of this bug. *** 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.  Henri Sivonen (:hsivonen) 2008-08-01 02:53:16 PDT Tweaking summary to avoid duplicate reports. 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. 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 ... 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. 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. Frédéric Wang (:fredw) 2010-11-18 10:15:02 PST Created attachment 491565 [details] [diff] [review] Patch V2 Frédéric Wang (:fredw) 2010-11-18 10:15:23 PST Created attachment 491566 [details] testcase 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) 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) Frédéric Wang (:fredw) 2011-01-02 07:53:02 PST Created attachment 500678 [details] [diff] [review] reftests Robert O'Callahan (:roc) (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. Frédéric Wang (:fredw) 2011-01-03 06:54:31 PST Created attachment 500796 [details] [diff] [review] Patch V3 - gecko changes Frédéric Wang (:fredw) 2011-01-03 06:55:30 PST Created attachment 500797 [details] [diff] [review] Patch V3 - chrome changes 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. Anne (:annevk) 2011-01-07 07:39:49 PST XLink should only work for SVG and only the subset that SVG requires. 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. 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 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. 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? 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. 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 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. 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. 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) 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. 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. 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." Henri Sivonen (:hsivonen) 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. 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. 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. 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! Jonas Sicking (:sicking) No longer reading bugmail consistently 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. 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. 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. Robert O'Callahan (:roc) (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? 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 have changed (I have only "bookmark" and "copy location"). Frédéric Wang (:fredw) 2011-01-10 03:14:20 PST Created attachment 502457 [details] [diff] [review] Patch - gecko changes (V4) Robert O'Callahan (:roc) (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. Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-01-12 16:28:23 PST Comment on attachment 502457 [details] [diff] [review] Patch - gecko changes (V4) r=me on the content parts 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. 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... :Ms2ger (⌚ UTC+1/+2) 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? 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 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? :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? 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. :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? 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. Frédéric Wang (:fredw) 2011-05-13 00:51:23 PDT Created attachment 532133 [details] [diff] [review] reftests Frédéric Wang (:fredw) 2011-05-13 00:52:20 PDT Created attachment 532134 [details] [diff] [review] Patch - gecko changes (V6) 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. 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)’: /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 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 aNodeInfo) : nsMathMLElementBase(aNodeInfo), Link(this), mIncrementScriptLevel(PR_FALSE) should solve the problem. 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 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. 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 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/ 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? Bill Gianopoulos [:WG9s] 2011-06-15 13:34:33 PDT The gecko part of this seems to, once again, not apply cleanly today. 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). 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? 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. 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. 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. Bill Gianopoulos [:WG9s] 2011-06-16 17:27:21 PDT Created attachment 539948 [details] [diff] [review] Patch - gecko changes (V7) Updated to current tip. 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]: ----------------------------------------------------------------- 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. 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. 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. 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. 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. 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. 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. :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. 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. Jonas Sicking (:sicking) No longer reading bugmail consistently 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. Henri Sivonen (:hsivonen) 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. 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. 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. 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. 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] 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. 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. Henri Sivonen (:hsivonen) 2011-06-24 05:04:52 PDT Does r=roc still apply to attachment 541465 [details] [diff] [review]? Henri Sivonen (:hsivonen) 2011-06-24 05:59:20 PDT (In reply to comment #88) > The patches to checkin are: > > attachment 505534 [details] [diff] [review] [review] > attachment 540276 [details] [diff] [review] [review] > attachment 541465 [details] [diff] [review] [review] Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/12294b20e4d9 http://hg.mozilla.org/integration/mozilla-inbound/rev/6b26a5b08522 http://hg.mozilla.org/integration/mozilla-inbound/rev/0ba8f12ec350 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. 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.