Bug 534968 (mathml-href)

[MathML3] Add support for href attribute

RESOLVED INCOMPLETE

Status

()

Core
MathML
P5
normal
RESOLVED INCOMPLETE
8 years ago
3 years ago

People

(Reporter: fredw, Unassigned)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

8 years ago
MathML 3 introduces the "href" attribute to make almost elements linkable.
See also bug 427990 for XLink in MathML.
Flags: wanted1.9.0.x?
(Reporter)

Updated

7 years ago
Flags: wanted1.9.0.x?
(Reporter)

Updated

7 years ago
Alias: mathml-href
(Reporter)

Updated

7 years ago
Depends on: 427990
Keywords: dev-doc-needed
(Reporter)

Updated

7 years ago
Blocks: 585141
(Reporter)

Comment 1

6 years ago
Created attachment 541466 [details] [diff] [review]
Patch

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

Comment 2

6 years ago
Created attachment 541471 [details] [diff] [review]
reftest
(Reporter)

Updated

6 years ago
Attachment #541471 - Flags: review?(roc)
Comment on attachment 541471 [details] [diff] [review]
reftest

Review of attachment 541471 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #541471 - Flags: review?(roc) → review+
(Reporter)

Comment 4

6 years ago
Created attachment 541931 [details]
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.
Depends on: 668618
> 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)
(Reporter)

Comment 6

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

Updated

6 years ago
Duplicate of this bug: 668618
Hi, I can provide you tests for contentAreaClick, I already had a couple for the duped bug!
(Reporter)

Comment 9

6 years ago
Created attachment 544016 [details] [diff] [review]
Patch (V2)
Attachment #541466 - Attachment is obsolete: true
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.
(Reporter)

Comment 11

6 years ago
(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.
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.
Keywords: dev-doc-needed → dev-doc-complete
Created attachment 544805 [details] [diff] [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.
(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.
(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.
(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 :)
(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.
(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.
Created attachment 546318 [details]
testcase 2

Second testcase from bug 427990.
(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.
(In reply to comment #13)
> Created attachment 544805 [details] [diff] [review] [review]
> wip

The code checked in for bug 564387 conflicts with this patch.
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.
(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.
Created attachment 550247 [details] [diff] [review]
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.
Assignee: nobody → bill
Status: NEW → ASSIGNED
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.
Attachment #550247 - Flags: feedback?
Attachment #550247 - Flags: feedback? → feedback?(gavin.sharp)
It would be helpful if you could attach a patch with 8 (or more) lines of diff context.
hmm i thought it did have 8 lines of context.  Sorry
(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.
Created attachment 550729 [details] [diff] [review]
Patch for feedback
Attachment #550247 - Attachment is obsolete: true
Attachment #550729 - Flags: feedback?(gavin.sharp)
Attachment #550247 - Flags: feedback?(gavin.sharp)
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.
Attachment #550729 - Flags: feedback?(gavin.sharp) → feedback+
Assignee: bill → nobody
Status: ASSIGNED → NEW
(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.
Blocks: 339725
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.
Attachment #550729 - Flags: feedback+ → feedback?(mak77)
Assignee: nobody → bill
Status: NEW → ASSIGNED
(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 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.
Attachment #550729 - Flags: feedback?(mak77)
(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;
(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.
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.
Attachment #550729 - Attachment is obsolete: true
Attachment #544016 - Attachment is obsolete: true
Attachment #544805 - Attachment is obsolete: true
(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.
(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.
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.
Created attachment 558098 [details] [diff] [review]
Add comment to address Gavin's early break feedback comment
Attachment #558095 - Attachment is obsolete: true
Attachment #558098 - Attachment description: Add coment to address Gavin's early break comment → Add comment to address Gavin's early break feedback comment
Created attachment 558135 [details] [diff] [review]
Fix my new comment to be more correct
Attachment #558098 - Attachment is obsolete: true
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.
Unassigning this bug to me.  Back to determining this bug is past my meager capability to fix.
Assignee: bill → nobody
Status: ASSIGNED → NEW
> >+      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.
(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.
(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.
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
(Reporter)

Comment 49

5 years ago
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.
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.
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.
please use needinfo flag for direct questions, it's basically impossible to track requests done through simple comments.
(Reporter)

Updated

5 years ago
Priority: -- → P5
(Reporter)

Comment 53

3 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Reporter)

Updated

3 years ago
No longer blocks: 339725
Resolution: FIXED → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.