The default bug view has changed. See this FAQ.
Bug 427990 (xlink-mathml)

XLink href not working on MathML elements (including <mrow>)

RESOLVED FIXED in mozilla7

Status

()

Core
MathML
P3
normal
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: distler, Assigned: fredw)

Tracking

(Blocks: 1 bug, {regression, testcase})

Trunk
mozilla7
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound][passed try])

Attachments

(4 attachments, 13 obsolete attachments)

1.63 KB, application/xhtml+xml
Details
1.16 KB, application/xhtml+xml
Details
18.55 KB, patch
sicking
: review+
Details | Diff | Splinter Review
1.77 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

9 years ago
Created attachment 314596 [details]
testcase
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, testcase
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.)
OS: Mac OS X → All
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
Looks like bug 355548.
Blocks: 355548
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.
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.
Flags: wanted1.9.1?
Priority: -- → P2
Assignee: nobody → mozbugz

Comment 7

9 years ago
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
Duplicate of this bug: 448526
(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.

Summary: XLink not working on <mrow> elements → XLink href not working on <mrow> elements
Tweaking summary to avoid duplicate reports.
Summary: XLink href not working on <mrow> elements → XLink href not working on MathML elements (including <mrow>)
Flags: wanted1.9.1? → wanted1.9.1+
(Assignee)

Updated

8 years ago
Blocks: 525772
(Reporter)

Comment 11

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

Comment 12

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

Updated

7 years ago
Depends on: 510202

Comment 13

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

Updated

7 years ago
Alias: xlink-mathml
(Assignee)

Comment 14

6 years ago
Created attachment 490831 [details] [diff] [review]
Patch V1

Experimental patch, which restores the xlink features and partially add support for the MathML3 href.
(Assignee)

Comment 15

6 years ago
Created attachment 491565 [details] [diff] [review]
Patch V2
Attachment #490831 - Attachment is obsolete: true
(Assignee)

Comment 16

6 years ago
Created attachment 491566 [details]
testcase
Attachment #314596 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 534968
Hardware: PowerPC → All
Assignee: karlt → fred.wang
(Assignee)

Comment 17

6 years ago
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)
Priority: P2 → P3
(Assignee)

Comment 18

6 years ago
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)
Attachment #491565 - Attachment is obsolete: true
Attachment #500674 - Flags: review?(roc)
(Assignee)

Comment 19

6 years ago
Created attachment 500678 [details] [diff] [review]
reftests
(Assignee)

Updated

6 years ago
Attachment #500678 - Flags: review?(roc)
Please separate the chrome changes from the Gecko changes, I can only review the latter. Gavin Sharp can probably review the former.
(Assignee)

Comment 21

6 years ago
Created attachment 500796 [details] [diff] [review]
Patch V3 - gecko changes
Attachment #500674 - Attachment is obsolete: true
Attachment #500796 - Flags: review?(roc)
Attachment #500674 - Flags: review?(roc)
(Assignee)

Comment 22

6 years ago
Created attachment 500797 [details] [diff] [review]
Patch V3 - chrome changes
Attachment #500797 - Flags: review?(gavin.sharp)
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

6 years ago
XLink should only work for SVG and only the subset that SVG requires.

Comment 25

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

Comment 26

6 years ago
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
Attachment #500797 - Attachment is obsolete: true
Attachment #501984 - Flags: review?(dao)
Attachment #500797 - Flags: review?(gavin.sharp)
With XLink support being dropped, I don't think the MathML 2 spec referring to the XLink spec has relevance here.

Comment 28

6 years ago
(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?
(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

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

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

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

Comment 34

6 years ago
>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.
(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

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

Comment 38

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

Comment 39

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

Updated

6 years ago
Attachment #501984 - Flags: review?(dao) → review?(gavin.sharp)

Comment 40

6 years ago
(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!
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

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

Comment 43

6 years ago
For rendering issues with underlined MathML, see bug 504324. Attachment 502339 [details] [diff] fixes the cases of MathML 3 href and XLink.
Attachment #500678 - Flags: review?(roc) → review+
Shouldn't the code in GetPopupLinkNode and GetLinkLocation just be calling nsIContent::GetHrefURI?
(Assignee)

Comment 45

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

Comment 46

6 years ago
Created attachment 502457 [details] [diff] [review]
Patch - gecko changes (V4)
Attachment #500796 - Attachment is obsolete: true
Attachment #502457 - Flags: review?(roc)
Attachment #500796 - Flags: review?(roc)
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.
Attachment #502457 - Flags: review?(roc)
Attachment #502457 - Flags: review?(jonas)
Attachment #502457 - Flags: review+
Comment on attachment 502457 [details] [diff] [review]
Patch - gecko changes (V4)

r=me on the content parts
Attachment #502457 - Flags: review?(jonas) → review+
(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.
(Assignee)

Comment 50

6 years ago
Created attachment 521126 [details] [diff] [review]
Patch - gecko changes (V5)

just resolving conflict as explained in comment 49...
Attachment #502457 - Attachment is obsolete: true
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?
(Assignee)

Comment 52

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

Updated

6 years ago
Whiteboard: [needs attachment 505534]
(Assignee)

Updated

6 years ago
Whiteboard: [needs attachment 505534] → [needs to land with attachment 505534][passed try]
Depends on: 504324
(Assignee)

Comment 53

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

Comment 55

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

Comment 57

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

Comment 58

6 years ago
Created attachment 532133 [details] [diff] [review]
reftests
Attachment #500678 - Attachment is obsolete: true
(Assignee)

Comment 59

6 years ago
Created attachment 532134 [details] [diff] [review]
Patch - gecko changes (V6)
Attachment #521126 - Attachment is obsolete: true
(Assignee)

Comment 60

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

Comment 62

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

Comment 65

6 years ago
@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?
The gecko part of this seems to, once again, not apply cleanly today.
(Assignee)

Comment 67

6 years ago
(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).
Do we absolutely have to have the browser code reviewed to land the Gecko code?
(Assignee)

Comment 69

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

Comment 71

6 years ago
> 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.
Created attachment 539948 [details] [diff] [review]
Patch - gecko changes (V7)

Updated to current tip.
Attachment #539948 - Attachment description: Patch - gecko changes (V6) → Patch - gecko changes (V7)
Attachment #532134 - Attachment is obsolete: true
Comment on attachment 539948 [details] [diff] [review]
Patch - gecko changes (V7)

Review of attachment 539948 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #539948 - Flags: review?(jonas)
Asking for new review from sicking just in case because the code it conflicted with was from bug 659053.
(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.
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.
(Assignee)

Comment 77

6 years ago
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.
(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.
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.
Attachment #539948 - Attachment is obsolete: true
Attachment #540276 - Flags: review?(jonas)
Attachment #539948 - Flags: review?(jonas)
(Assignee)

Comment 80

6 years ago
> 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 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.
Attachment #501984 - Flags: review?(gavin.sharp) → review-

Comment 82

6 years ago
(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 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.
Attachment #540276 - Flags: review?(jonas) → review+
(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.
(Assignee)

Comment 85

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

Comment 87

6 years ago
Created attachment 541465 [details] [diff] [review]
reftests - V2

Removing the MathML3 href reftest.
Attachment #501984 - Attachment is obsolete: true
Attachment #532133 - Attachment is obsolete: true
(Assignee)

Comment 88

6 years ago
The patches to checkin are:

attachment 505534 [details] [diff] [review]
attachment 540276 [details] [diff] [review]
attachment 541465 [details] [diff] [review]
(Assignee)

Comment 89

6 years ago
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.
(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.
Does r=roc still apply to attachment 541465 [details] [diff] [review]?
Attachment #541465 - Flags: review+
(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
Flags: in-testsuite+
Whiteboard: [needs to land with attachment 505534][passed try] → [inbound][passed try]
Target Milestone: --- → mozilla7
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 94

6 years ago
(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
You need to log in before you can comment on or make changes to this bug.