534968 - (mathml-href) [MathML3] Add support for href attribute

Status: RESOLVED INCOMPLETE

(Core :: MathML, defect, P5)

Description

[Frédéric Wang (:fredw)]

MathML 3 introduces the "href" attribute to make almost elements linkable.
See also bug 427990 for XLink in MathML.

Comment 1

[Frédéric Wang (:fredw)]

Attachment: Patch

Patch from bug 427990. See its last review in bug 427990 comment 81.

Comment 2

[Frédéric Wang (:fredw)]

Attachment: reftest

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 541471 [details] [diff] [review]
reftest

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

Comment 4

[Frédéric Wang (:fredw)]

Attachment: testcase

Since bug 427990 was fixed, MathML3 href is almost working. However if you right click on links in the testcase, you notice that the context menu is missing options for MathML3 href: bookmark link, copy link location etc This is what is implemented in the patch of attachment 541466 [details] [diff] [review].

See also bug 427990 comment 41, about adding warnings in the console.

Comment 5

[Florian Scholz (Open Web Docs)]

> Since bug 427990 was fixed, MathML3 href is almost working. However if you
> right click on links in the testcase, you notice that the context menu is
> missing options for MathML3 href: bookmark link, copy link location etc This
> is what is implemented in the patch of attachment 541466 [details] [diff] [review] [review].

So, with href working now, should we update the docs to mention that it's implemented in Gecko 7.0 now? (even if your patch and bug 668618 won't make it into 7.0)

Comment 6

[Frédéric Wang (:fredw)]

> So, with href working now, should we update the docs to mention that it's
> implemented in Gecko 7.0 now? (even if your patch and bug 668618 won't make
> it into 7.0)

Yes, I think we can say it is implemented in Gecko 7.0. The missing feature is at the Chrome level, in the context menu. I had already updated the Firefox_7_for_developers page.

Comment 8

[Marco Bonardo [:mak]]

Hi, I can provide you tests for contentAreaClick, I already had a couple for the duped bug!

Comment 9

[Frédéric Wang (:fredw)]

Attachment: Patch (V2)

Comment 10

[Marco Bonardo [:mak]]

I suspect it would be better to split the patch in the part managing the click and the part managing the context menu. they are pretty much unrelated and would help any reviewer evaluating the 2 separately.

Comment 11

[Frédéric Wang (:fredw)]

(In reply to comment #8)
> Hi, I can provide you tests for contentAreaClick, I already had a couple for
> the duped bug!

(In reply to comment #10)
> I suspect it would be better to split the patch in the part managing the
> click and the part managing the context menu. they are pretty much unrelated
> and would help any reviewer evaluating the 2 separately.

(In reply to comment #8)
> Hi, I can provide you tests for contentAreaClick, I already had a couple for
> the duped bug!

Thank you very much. Please attach a patch for them on this bug.

The problem here, is that attachment 541466 [details] [diff] [review] is essentially the same patch as in bug 427990 (modified to integrate your changes on bug 667195) which was unfortunately rejected because MathML3 href was not thought important enough to justify adding more complexity to the Chrome code. I don't want to continue the work on this, so feel free to modify the patch as you feel. But it may be a good idea to ask the Chrome peers what kind of changes they are likely to accept before.

Comment 12

[Marco Bonardo [:mak]]

Well, the comment there seems to be based on the fact there was no acknowledge on the basic support in Gecko and contentAreaClick was really sucky, plus was unclear if href had priority on xlink:href.
Now href in MathML (and maybe SVG soon?) is a standard, is implemented in Gecko, contentAreaClick is a bit better (far from perfect), there are tests, and priority is clear, so I feel like the click part doesn't really add bloat.

The fact contextMenu and click have both to figure out href still sucks, but maybe the context menu path may be better unified.

Comment 13

[Marco Bonardo [:mak]]

Attachment: wip

As a side note, we just published on the hacks.m.o blog this regarding Aurora7:

> MathML
> XLink href has been restored and the MathML3 href attribute is now supported.
> Developers are encouraged to move to the latter syntax.

encouraging using the new syntax, means increasing importance of this bug.

This is a slightly modified patch (starting from the good patch from Frederic) I tried to put together to evaluate if we may unify the context menu and click code paths. It's feasible, but it changes a couple facts, that imo are ignorable, so not worth complicating things just to support them:

- change hrefAndLinkNodeForClickEvent to hrefAndLinkNodeForElement. While this is a public exposed method, I searches mxr and didn't find any addon using it.
- drop check for xlink type simple. I still have to go through the reference to figure out if this is really needed (there are only 2 types allowing href, so searching href may be enough), but if it is we should rather move it into hrefAndLinkNodeForElement.
- don't throw if a link is invalid. on common links If I try to save-as a broken link, it just saves the current doc, I don't see why this was special-cased for xlinks.

I still have to double-check everything (this is not a top-priority atm), but if you have feedback it will be useful.

Comment 14

[Bill Gianopoulos [:WG9s]]

(In reply to comment #13)
> Created attachment 544805 [details] [diff] [review] [review]
> wip
> This is a slightly modified patch (starting from the good patch from
> Frederic) I tried to put together to evaluate if we may unify the context
> menu and click code paths. It's feasible, but it changes a couple facts,
> that imo are ignorable, so not worth complicating things just to support
> them:That was my whole point in pointing the 2 of you to each other's patches.  Seemed to me that you had more of a handle on how to better integrate this into the design going forward of click methods in browser.js which would alleviate Gavin's concerns of making code that was kind pof spaghetti-like, even more so.  But, I also thought that starting from code that was actually proven to work would be helpful.

Comment 15

[Bill Gianopoulos [:WG9s]]

(In reply to comment #14)
> (In reply to comment #13)
> > Created attachment 544805 [details] [diff] [review] [review] [review]
> > wip
> > This is a slightly modified patch (starting from the good patch from
> > Frederic) I tried to put together to evaluate if we may unify the context
> > menu and click code paths. It's feasible, but it changes a couple facts,
> > that imo are ignorable, so not worth complicating things just to support
> > them:

Hmm I screwed up and garbled the previous comment.

That was my whole point in pointing the 2 of you to each other's patches.  Seemed to me that you had more of a handle on how to better integrate this into the design going forward of click methods in browser.js which would alleviate Gavin's concerns of making code that was kind pof spaghetti-like, even more so.  But, I also thought that starting from code that was actually proven to work would be helpful.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #11)
> The problem here, is that attachment 541466 [details] [diff] [review] [review] is
> essentially the same patch as in bug 427990 (modified to integrate your
> changes on bug 667195) which was unfortunately rejected [...]

Those changes weren't "rejected" - I expressed reservations about taking such changes, but circumstances can change and arguments can be made. I am able (and willing) to be convinced - you shouldn't give up so easily :)

Comment 17

[Bill Gianopoulos [:WG9s]]

(In reply to comment #16)
> Those changes weren't "rejected" - I expressed reservations about taking
> such changes, but circumstances can change and arguments can be made. I am
> able (and willing) to be convinced - you shouldn't give up so easily :)

I expect to have a new patch here with 2 weeks that might be  more accepatble, if I understand what your origninal objectiosn were.  i thought they were:

1.  not sure if the gecko patches wre going to land.

    Well they did so that kind of negates that issue.

2.  The existing code in browser for all of this is kind of kludgey and hard to support and and this patch just makes it worse.  My intention is to create a patch that at least makes it easier, not sure it is the final solution, but at least adds support for more features while actually improving the on-going support issue.

I should ahve something posted here within a couple of weeks.

Comment 18

[Bill Gianopoulos [:WG9s]]

(In reply to comment #13)
> Created attachment 544805 [details] [diff] [review] [review]
> wip
> 
> As a side note, we just published on the hacks.m.o blog this regarding
> Aurora7:
> 
> > MathML
> > XLink href has been restored and the MathML3 href attribute is now supported.
> > Developers are encouraged to move to the latter syntax.
> 
> encouraging using the new syntax, means increasing importance of this bug.
> 
> This is a slightly modified patch (starting from the good patch from
> Frederic) I tried to put together to evaluate if we may unify the context
> menu and click code paths. It's feasible, but it changes a couple facts,
> that imo are ignorable, so not worth complicating things just to support
> them:
> 
> - change hrefAndLinkNodeForClickEvent to hrefAndLinkNodeForElement. While
> this is a public exposed method, I searches mxr and didn't find any addon
> using it.
> - drop check for xlink type simple. I still have to go through the reference
> to figure out if this is really needed (there are only 2 types allowing
> href, so searching href may be enough), but if it is we should rather move
> it into hrefAndLinkNodeForElement.
> - don't throw if a link is invalid. on common links If I try to save-as a
> broken link, it just saves the current doc, I don't see why this was
> special-cased for xlinks.
> 
> I still have to double-check everything (this is not a top-priority atm),
> but if you have feedback it will be useful.

I looked at this further and decided you are on the right track here.  Unifying the 2 paths would make this all much easier to maintain.  I will concentrate on testing your wip patch, and perhaps suggest improvements to that.

Comment 19

[Bill Gianopoulos [:WG9s]]

Attachment: testcase 2

Second testcase from bug 427990.

Comment 20

[Bill Gianopoulos [:WG9s]]

(In reply to comment #13)
> Created attachment 544805 [details] [diff] [review] [review]
> wip

I attached the second testcase from bug 427990 to show that your wip patch, as a consequence of unifying the code,  also fixes this issue for SVG xlinks, which the previous patch did not.  This highlights the advantages of unifying the code path.

I am now including this patch in my nightly builds at http://www.wg9s.com/mozilla/firefox/ to facilitate testing.

Comment 21

[Bill Gianopoulos [:WG9s]]

(In reply to comment #13)
> Created attachment 544805 [details] [diff] [review] [review]
> wip

The code checked in for bug 564387 conflicts with this patch.

Comment 22

[Marco Bonardo [:mak]]

feel free to unbitrot and take ownership of my patch if you wish, I have no problems with that and no time this week to fix it.

Comment 23

[Bill Gianopoulos [:WG9s]]

(In reply to comment #22)
> feel free to unbitrot and take ownership of my patch if you wish, I have no
> problems with that and no time this week to fix it.

Were it as simple as that, I would have done so already.  The issue is not just a merge conflict, it is a logic conflict.  Both patches change the initial definition of channel in nsContextMenu.prototype in kind of an orthogonal manner.  My guess form looking at this was that it might be that your change to this definition was no longer required.  So, I just removed that part of your patch and have been running builds with that included for a couple fo weeks and have not noticed any issues.  However, I kind of think that I need to have you look at that area to make sure that is really the correct approach.

Comment 24

[Bill Gianopoulos [:WG9s]]

Attachment: Wip updated to tip

This is what I am building with, but like I said it is just the previous patch with the conflicting part removed.

Comment 25

[Bill Gianopoulos [:WG9s]]

Comment on attachment 550247 [details] [diff] [review]
Wip updated to tip

Looking for feedback from Gavin as to if this approach of unifying the codepaths so that the code for determining the link for a click and the context menu to display is no longer in 2 places alleviates the concerns that the previous patch just took a bad situation and makes it worse.  This patch, besides supporting the new MathML3 features, also consolidates the code to find the link for a click or in one place.

Also, the entire question of it not being clear if the gecko part was going to be approved is now moot since that code is now in mozilla-central.

SO, what I am looking for is just a Mak's rework of the code in browser.js so that each time we come up with some new thing that is a link instead of having 2 palaces to change we only have one now is what you were looking for as far as making the code more supportable.

If i get positive feedback, then I will ask for reviews from Mak to make sure what i did to resolve the merge conflict is the right thing and then from gavin to do a real code review.

The feedback request is just to make sure this is not all just a waste of everyone's time.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

It would be helpful if you could attach a patch with 8 (or more) lines of diff context.

Comment 27

[Bill Gianopoulos [:WG9s]]

hmm i thought it did have 8 lines of context.  Sorry

Comment 28

[Bill Gianopoulos [:WG9s]]

(In reply to comment #27)
> hmm i thought it did have 8 lines of context.  Sorry

OIC it does not now i need to figure out what system i used to create that my .hgrc is supposed to give 8 lines of context.

Comment 29

[Bill Gianopoulos [:WG9s]]

Attachment: Patch for feedback

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 550729 [details] [diff] [review]
Patch for feedback

This looks good in general.

What's the test coverage like for the getters/properties you're changing in nsContextMenu? I.e., if you break them all individually, how many tests fail? :)

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

>     // Second, bubble out, looking for items of interest that can have childen.
>     // Always pick the innermost link, background image, etc.
>     const XMLNS = "http://www.w3.org/XML/1998/namespace";
>     var elem = this.target;
>     while (elem) {
>       if (elem.nodeType == Node.ELEMENT_NODE) {
>+        if (!this.onLink) {
>+          let [href, linkNode] = hrefAndLinkNodeForElement(elem);
>+          if (href) {
>+            // Target is a link or a descendant of a link.
>+            this.link = elem;
>+            this.linkURL = href;
>+            this.linkURI = makeURI(href);
>+            this.linkProtocol = this.linkURI.scheme;
>+          }
>         }

You probably want to break out of the loop at this point... this logic will need to be carefully reviewed.

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

> function wrapperMethod(aInvokedMethods, aMethodName) {
>   return function () {
>     aInvokedMethods.push(aMethodName);
>+
>     // At least getShortcutOrURI requires to return url that is the first param.
>-    return arguments[0];
>+    let maybeURL = arguments[0];
>+    if (/^http/.test(maybeURL)) {
>+      is(maybeURL, "http://mochi.test/moz/", "The correct URL was requested");
>+    }

This looks fragile, and I'm not sure it's that useful of a test.

Comment 31

[Bill Gianopoulos [:WG9s]]

(In reply to Gavin Sharp from comment #30)
> Comment on attachment 550729 [details] [diff] [review] [diff] [details] [review]
> Patch for feedback
> 
> This looks good in general.
> 
> What's the test coverage like for the getters/properties you're changing in
> nsContextMenu? I.e., if you break them all individually, how many tests
> fail? :)
> 
> >diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
> 
> >     // Second, bubble out, looking for items of interest that can have childen.
> >     // Always pick the innermost link, background image, etc.
> >     const XMLNS = "http://www.w3.org/XML/1998/namespace";
> >     var elem = this.target;
> >     while (elem) {
> >       if (elem.nodeType == Node.ELEMENT_NODE) {
> >+        if (!this.onLink) {
> >+          let [href, linkNode] = hrefAndLinkNodeForElement(elem);
> >+          if (href) {
> >+            // Target is a link or a descendant of a link.
> >+            this.link = elem;
> >+            this.linkURL = href;
> >+            this.linkURI = makeURI(href);
> >+            this.linkProtocol = this.linkURI.scheme;
> >+          }
> >         }
> 
> You probably want to break out of the loop at this point... this logic will
> need to be carefully reviewed.
> 
> >diff --git a/browser/base/content/test/browser_contentAreaClick.js b/browser/base/content/test/browser_contentAreaClick.js
> 
> > function wrapperMethod(aInvokedMethods, aMethodName) {
> >   return function () {
> >     aInvokedMethods.push(aMethodName);
> >+
> >     // At least getShortcutOrURI requires to return url that is the first param.
> >-    return arguments[0];
> >+    let maybeURL = arguments[0];
> >+    if (/^http/.test(maybeURL)) {
> >+      is(maybeURL, "http://mochi.test/moz/", "The correct URL was requested");
> >+    }
> 
> This looks fragile, and I'm not sure it's that useful of a test.

I appreciate the feedback.

What I was really looking for was your original "This looks good in general" comment.

The rest of your comments make it obvious to me that this needs to be turned back over to the original patch author.

It seems that my feeling that perhaps I could expedite this was too ambitious based on my time availability to learn new things right now because of work commitments.

Comment 32

[Bill Gianopoulos [:WG9s]]

Comment on attachment 550729 [details] [diff] [review]
Patch for feedback

OK feedback I am looking fvor here is:

1.  Is what I did to resolve the merge conflict the correct thing?

2.  I need to have an answer to Gavin's question here:

> >diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
> 
> >     // Second, bubble out, looking for items of interest that can have childen.
> >     // Always pick the innermost link, background image, etc.
> >     const XMLNS = "http://www.w3.org/XML/1998/namespace";
> >     var elem = this.target;
> >     while (elem) {
> >       if (elem.nodeType == Node.ELEMENT_NODE) {
> >+        if (!this.onLink) {
> >+          let [href, linkNode] = hrefAndLinkNodeForElement(elem);
> >+          if (href) {
> >+            // Target is a link or a descendant of a link.
> >+            this.link = elem;
> >+            this.linkURL = href;
> >+            this.linkURI = makeURI(href);
> >+            this.linkProtocol = this.linkURI.scheme;
> >+          }
> >         }
> 
> You probably want to break out of the loop at this point... this logic will
> need to be carefully reviewed.

should a break out of the loop here be required (meaning we select the fist condition matching these conditions) or should it keep going to find the last one?  I don;t understand what you intention here was well enough to answer this.

As far as the questions about tests, I will need help form someone doing any test coding as I am very wary of adding more tests that produce random oranges, I would really like to have help from someone with more experience with the test harness.  

But in any event i have more time now and no one else stepped up in the interirm, so I am re-taking the bug.

Comment 33

[Bill Gianopoulos [:WG9s]]

(In reply to Gavin Sharp from comment #30)
> >diff --git a/browser/base/content/test/browser_contentAreaClick.js b/browser/base/content/test/browser_contentAreaClick.js
> 
> > function wrapperMethod(aInvokedMethods, aMethodName) {
> >   return function () {
> >     aInvokedMethods.push(aMethodName);
> >+
> >     // At least getShortcutOrURI requires to return url that is the first param.
> >-    return arguments[0];
> >+    let maybeURL = arguments[0];
> >+    if (/^http/.test(maybeURL)) {
> >+      is(maybeURL, "http://mochi.test/moz/", "The correct URL was requested");
> >+    }
> 
> This looks fragile, and I'm not sure it's that useful of a test.

I am not sure what you mean here.  If you are saying this test looks like it may intermittently fail and it does not seem like that useful a thing to be testing anyway, I can certainly easily remove it.

However, if you think it needs to be replaced with something that tests something more useful, pleas elaborate.

Comment 34

[Marco Bonardo [:mak]]

Comment on attachment 550729 [details] [diff] [review]
Patch for feedback

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

notice this patch is probably going to conflict with bug 516753, possible ways out are coordinate with Mano and have him include the mathML fixes into his patch or take this patch asap and have mano work on the new code with the test checked-in

::: browser/base/content/nsContextMenu.js
@@ +548,5 @@
> +            this.link = elem;
> +            this.linkURL = href;
> +            this.linkURI = makeURI(href);
> +            this.linkProtocol = this.linkURI.scheme;
> +          }

gavin is right this should exit at the innermost link (see also the comment above), though, being this.onLink a getter it does already exit as soon as we set this.link. I guess may want a comment or an explicit break though, since implicit/hidden flow is bad.

::: browser/base/content/test/browser_contentAreaClick.js
@@ +271,5 @@
> +    let maybeURL = arguments[0];
> +    if (/^http/.test(maybeURL)) {
> +      is(maybeURL, "http://mochi.test/moz/", "The correct URL was requested");
> +    }
> +    return maybeURL;

this indeed does not look such a great test at first glance, but I added it to check that in case we have both a xlink and a plain href, the latter is preferred. I guess it may be changed to directly check which attributes are on the node and compare, but didn't look too deep into that. Just wanted to clarify what this is supposed to test.

Comment 35

[Bill Gianopoulos [:WG9s]]

(In reply to Marco Bonardo [:mak] (Away 7-17 Sept.) from comment #34)

> ::: browser/base/content/nsContextMenu.js
> @@ +548,5 @@
> > +            this.link = elem;
> > +            this.linkURL = href;
> > +            this.linkURI = makeURI(href);
> > +            this.linkProtocol = this.linkURI.scheme;
> > +          }
> 
> gavin is right this should exit at the innermost link (see also the comment
> above), though, being this.onLink a getter it does already exit as soon as
> we set this.link. I guess may want a comment or an explicit break though,
> since implicit/hidden flow is bad.

Looking at the code more closely, adding a break here would be wrong.  It would be oOK for the link case, but the same loop is doing links and background images.  Therefore, exiting the loop early might make it not find a background image that it should.

It would seem that perhaps the correct thing to do would be to set this.onLink = true;

Comment 36

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos from comment #35)
> (In reply to Marco Bonardo [:mak] (Away 7-17 Sept.) from comment #34)
> 
> > ::: browser/base/content/nsContextMenu.js
> > @@ +548,5 @@
> > > +            this.link = elem;
> > > +            this.linkURL = href;
> > > +            this.linkURI = makeURI(href);
> > > +            this.linkProtocol = this.linkURI.scheme;
> > > +          }
> > 
> > gavin is right this should exit at the innermost link (see also the comment
> > above), though, being this.onLink a getter it does already exit as soon as
> > we set this.link. I guess may want a comment or an explicit break though,
> > since implicit/hidden flow is bad.
> 
> Looking at the code more closely, adding a break here would be wrong.  It
> would be oOK for the link case, but the same loop is doing links and
> background images.  Therefore, exiting the loop early might make it not find
> a background image that it should.
> 
> It would seem that perhaps the correct thing to do would be to set
> this.onLink = true;

Looking at this more closely it seems that is how the code resolved this issue before this change.  I will post a new patch that does that and just leave it on this bug and get with Mano in bug 516753 to see how he wants to proceed.

Comment 37

[Bill Gianopoulos [:WG9s]]

Attachment: With early break out of loop

This addresses only Gavin's feedback about breaking out of the loop early.  It does not address any of his issues with tests.

Comment 38

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos from comment #37)
> Created attachment 558095 [details] [diff] [review]
> With early break out of loop
> 
> This addresses only Gavin's feedback about breaking out of the loop early. 
> It does not address any of his issues with tests.

I should have made it clearer from the patch title that this does not break out of the loop early, just ensures it will stop looking for links on subsequent iterations.

Comment 39

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos from comment #38)
> (In reply to Bill Gianopoulos from comment #37)
> > Created attachment 558095 [details] [diff] [review]
> > With early break out of loop
> > 
> > This addresses only Gavin's feedback about breaking out of the loop early. 
> > It does not address any of his issues with tests.
> 
> I should have made it clearer from the patch title that this does not break
> out of the loop early, just ensures it will stop looking for links on
> subsequent iterations.

That code still looks wrong.  The original patch removed the code to initialize this.onLink to false and the code to set it to true, but left a test to conditionally execute code based on the false case.

I think I need to restore the initialization also to make it correct.

Comment 40

[Bill Gianopoulos [:WG9s]]

OIC the get onLink() !!this.link, in the context menu prototype takes care of this for us, so I should really just add a comment to that effect here.

Comment 41

[Bill Gianopoulos [:WG9s]]

Attachment: Add comment to address Gavin's early break feedback comment

Comment 42

[Bill Gianopoulos [:WG9s]]

Attachment: Fix my new comment to be more correct

Comment 43

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 558135 [details] [diff] [review]
Fix my new comment to be more correct

>   while (node && !href) {
>     if (node.nodeType == Node.ELEMENT_NODE) {
>-      href = node.getAttributeNS("http://www.w3.org/1999/xlink", "href");
>+      // MathML3 allows to define a href attribute, that has priority on XLinks.
>+      if (node.namespaceURI == "http://www.w3.org/1998/Math/MathML" &&
>+          node.hasAttribute("href")) {
>+        href = node.getAttribute("href");

I think this should use hasAttributeNS/getAttributeNS.

Comment 44

[Bill Gianopoulos [:WG9s]]

Unassigning this bug to me.  Back to determining this bug is past my meager capability to fix.

Comment 45

[Marco Bonardo [:mak]]

> >+      if (node.namespaceURI == "http://www.w3.org/1998/Math/MathML" &&
> >+          node.hasAttribute("href")) {
> >+        href = node.getAttribute("href");
> 
> I think this should use hasAttributeNS/getAttributeNS.

fwiw, the spec says it is just href attribute, it should not be namespaced.

Comment 46

[Bill Gianopoulos [:WG9s]]

(In reply to Marco Bonardo [:mak] from comment #34)
> Comment on attachment 550729 [details] [diff] [review]
> Patch for feedback
> 
> Review of attachment 550729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> notice this patch is probably going to conflict with bug 516753, possible

It seems that bug 516753 has been abandoned.  So now what is the past path forward here?

I really think that this is a very good patch even without the MathML left click issue because it unifies the 2 codepaths that deal with links in Firefox to a single codepath so that changes don't continue to have to be made in 2 locations.

Comment 47

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #46)
> (In reply to Marco Bonardo [:mak] from comment #34)
> > Comment on attachment 550729 [details] [diff] [review]
> > Patch for feedback
> > 
> > Review of attachment 550729 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > notice this patch is probably going to conflict with bug 516753, possible
> 
> It seems that bug 516753 has been abandoned.  So now what is the past path
> forward here?
> 
> I really think that this is a very good patch even without the MathML left
> click issue because it unifies the 2 codepaths that deal with links in
> Firefox to a single codepath so that changes don't continue to have to be
> made in 2 locations.

Right click left click ... you know what I meant.

Comment 48

[Marco Bonardo [:mak]]

I suppose someone may review the changes, I can't, since I wrote part of them. Though the remaining concern from gavin's previous feedback are tests on the getters in nsContextMenu.js

Comment 49

[Frédéric Wang (:fredw)]

FYI, the case of Seamonkey's UI is handled in bug 717587. I'm not sure it can help to fix the Firefox's UI though, because the code seems a bit different.

Comment 50

[Bill Gianopoulos [:WG9s]]

Too emphasize the fact that I am not actively working on this.  And that the outstanding issues are writing tests which are written in languages I am not really comfortable in programming in.  I am dropping this patch from those I include in the builds I provide on my website.

Comment 51

[Bill Gianopoulos [:WG9s]]

Oh I should have said, I asked for help in writing the required tests, both form the patch author and from the reviewer who requested the tests and got no response.

Comment 52

[Marco Bonardo [:mak]]

please use needinfo flag for direct questions, it's basically impossible to track requests done through simple comments.

Comment 53

[Frédéric Wang (:fredw)]

I've opened bug 958957 to clarify that it's neither a Gecko bug nor a Seamonkey bug. The remaining issue is just in the Firefox UI and must be fixed by the appropriate people.