I've prepared some XBL bindings to implement the tooltip and highlight actiontypes. The MathML spec suggests to use namespaces for non-standard attributes, but I suspect this won't work well in non-XML document. So for the highlight actiontypes I've instead used the command names of itex2MML and introduced two non-standard attributes fghighlight and bghighlight (hopefully they could be standardized later).

Comment on attachment 8372873 [details] [diff] [review] Patch V1 The "highlight" action is not a predefined action but provided as an example of an extension, without any clear specified behavior. I'd like to avoid adding extensions unless there is a strong need. "highlight" looks like something that could be achieved by other means. "tooltip" is at least predefined, even if optional. Implementing "tooltip" as compatibility sugar for "title" sounds appealing, but the title text is only set in the constructor, remaining the same after there are changes to the child.

(In reply to Karl Tomlinson (:karlt) from comment #2) > Comment on attachment 8372873 [details] [diff] [review] > Patch V1 > > The "highlight" action is not a predefined action but provided as an example > of an extension, without any clear specified behavior. I'd like to avoid > adding extensions unless there is a strong need. "highlight" looks like > something that could be achieved by other means. > These were defined as itex2MML commands (http://golem.ph.utexas.edu/~distler/blog/itex2MMLcommands.html) and implemented with a small Javascript snippet. It's slightly a problem to me to generate MathML code that could not work independently (for copy and paste of equations etc). However, these itex2MML commands are marked "deprecated" so I'm fine with ignoring this. > "tooltip" is at least predefined, even if optional. Implementing "tooltip" > as > compatibility sugar for "title" sounds appealing, but the title text is only > set in the constructor, remaining the same after there are changes to the > child. OK, I thought it would be updated each time the content changes, but I'll have to check that.

Here is a new version that works with attachment 600000 [details].

This patch needs a test so that if another patch like the previous one for bug 687807 breaks it a test will fail.

(In reply to Bill Gianopoulos [:WG9s] from comment #5) > This patch needs a test so that if another patch like the previous one for > bug 687807 breaks it a test will fail. @Aniket: I saw in the IRC logs that you wanted a MathML bug to work on. Are you interested to try this one? Maybe with a mochitest like layout/mathml/tests/test_bug975681.html it will be possible to emulate a mouseover and test this tooltip feature...

Comment on attachment 8374022 [details] [diff] [review] Patch V2 Review of attachment 8374022 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/xbl-maction/xbl-maction.xml @@ +31,5 @@ > + tooltip.firstElementChild); > + } > + ]]> > + </body> > + </method> trailing space to remove here.

Thanks :fredw, I'll do it.

Trailing space removed.

(In reply to Aniket Deshpande from comment #9) > Created attachment 8473806 [details] [diff] [review] > maction-tooltip.patch > > Trailing space removed. How did you apply the patch? This one seems to have even more trailing whitespace and to only add an xbl-maction.xml file. See https://developer.mozilla.org/en-US/docs/Mercurial_Queues for how to manage patches with mercurial queue. (you can use hg qimport to import https://bug969906.bugzilla.mozilla.org/attachment.cgi?id=8374022)

Re-submitting the patch with trailing spaces removed. Fredw: I think i saw few other trailing spaces ::: layout/style/xbl-maction/xbl-maction.xml @@ +31,5 @@ > + tooltip.firstElementChild); > + } > + ]]> > + </body> > + </method> also at # file, You can obtain one at http://mozilla.org/MPL/2.0/. -DIRS += ['xbl-marquee'] +DIRS += ['xbl-marquee', 'xbl-maction'] TEST_TOOL_DIRS += ['test'] I removed those. Please review if the changes are correct.

Unfortunately, it appears that something has changed on mozilla-central in the last week or so that makes this patch both no longer work, but it also causes the math in the testcase to no longer display at all.

Updated patch to work with current mozilla-central.

(In reply to Bill Gianopoulos [:WG9s] from comment #12) > Unfortunately, it appears that something has changed on mozilla-central in > the last week or so that makes this patch both no longer work, but it also > causes the math in the testcase to no longer display at all. I should have added this to the previous comment. To prevent others form having to do interdiffs to see how I fixed this, the issue here is that recent enhancements require explicitly permitting access to XBL bindings from content. The fix was to add the following: <binding id="tooltipBinding" inheritstyle="true" bindToUntrustedContent="true"> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

BTW this bug has a status of ASSIGNED, although it is assigned to Nobody. This seems just wrong!

Oh and I figured this out by looking at the hg log on xbl-marquee.

Thanks Bill & Aniket. If any of you wants to try to make a test for this, see comment 6.

This would look much better with .5em horizontal padding on both sides of the tooltip text.

I wrote this test for tooltip. Please suggest changes/improvements.

(In reply to Aniket Deshpande from comment #19) > Created attachment 8475258 [details] > test_bug969906.html > > I wrote this test for tooltip. Please suggest changes/improvements. All tests are failing. I will re-write the test.

Updated the test(now passing!). submitting the patch. I have a problem in creating the patch. After updating the mochitest.ini, the patch still shows +[DEFAULT] + +[test_bug330964.html] +[test_bug553917.html] +[test_bug706406.html] +[test_bug827713-2.html] +[test_bug827713.html] +[test_bug975681.html] +[test_bug969906.html] +[test_opentype-axis-height.html] +[test_opentype-fraction.html] +[test_opentype-limits.html] Please help.(Sorry for taking this discussion off track.)

(In reply to Aniket Deshpande from comment #21) > Created attachment 8475336 [details] [diff] [review] > test_bug969906.patch > > Updated the test(now passing!). submitting the patch. I have a problem in > creating the patch. > After updating the mochitest.ini, the patch still shows Not sure I understand... Did you try an "hg qrefresh"?

Comment on attachment 8475336 [details] [diff] [review] test_bug969906.patch Review of attachment 8475336 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/mathml/tests/test_bug969906.html @@ +15,5 @@ > +<p> > + <math> > + <maction actiontype="tooltip" id="test_maction_tooltip"> > + <mspace width="100px" height="20px" mathbackground="red"></mspace>--> > + <!-- <mtext>x</mtext> --> Are the children commented out? @@ +60,5 @@ > + window.removeEventListener("mouseout", doStopPropagation, true); > + window.removeEventListener("mouseenter", doStopPropagation, true); > + window.removeEventListener("mouseleave", doStopPropagation, true); > + > + Can you explain what you are testing?

(In reply to Frédéric Wang (:fredw) from comment #22) > (In reply to Aniket Deshpande from comment #21) > > Created attachment 8475336 [details] [diff] [review] > > test_bug969906.patch > > > > Updated the test(now passing!). submitting the patch. I have a problem in > > creating the patch. > > After updating the mochitest.ini, the patch still shows > > Not sure I understand... Did you try an "hg qrefresh"? Yes. A couple of times actually.

What I think is missing from the test also is a test for the condition reported in comment 12. We also need to compare the rendering of the page with no mouseover with and without the <maction actiontype="tooltip" id="test_maction_tooltip"> and make sure the page renders identically in both cases, to cover the case where the binding is rejected and the math does not display at all.

Although I guess this test would fail in that case also, just might be nice to report a different error for the 2 cases.

(In reply to Frédéric Wang (:fredw) from comment #23) > Comment on attachment 8475336 [details] [diff] [review] > test_bug969906.patch > > Review of attachment 8475336 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/mathml/tests/test_bug969906.html > @@ +15,5 @@ > > +<p> > > + <math> > > + <maction actiontype="tooltip" id="test_maction_tooltip"> > > + <mspace width="100px" height="20px" mathbackground="red"></mspace>--> > > + <!-- <mtext>x</mtext> --> > > Are the children commented out? > I forgot to remove the comments. > @@ +60,5 @@ > > + window.removeEventListener("mouseout", doStopPropagation, true); > > + window.removeEventListener("mouseenter", doStopPropagation, true); > > + window.removeEventListener("mouseleave", doStopPropagation, true); > > + > > + > > Can you explain what you are testing? I took reference from layout/mathml/tests/test_bug706406.html (maction toggle) So, the tests are before adding the EventListener ,after adding the EventListener and finally removing the EventListener. The tooltip will be triggered on mouseover. The pop-up will be above or below the desired text/equation. It should not be eiether left or right.

(In reply to Aniket Deshpande from comment #21) > Created attachment 8475336 [details] [diff] [review] > test_bug969906.patch > > Updated the test(now passing!). submitting the patch. I have a problem in > creating the patch. > After updating the mochitest.ini, the patch still shows > +[DEFAULT] > + > +[test_bug330964.html] > +[test_bug553917.html] > +[test_bug706406.html] > +[test_bug827713-2.html] > +[test_bug827713.html] > +[test_bug975681.html] > +[test_bug969906.html] > +[test_opentype-axis-height.html] > +[test_opentype-fraction.html] > +[test_opentype-limits.html] > > Please help.(Sorry for taking this discussion off track.) It appears you created the patch file as if layout/mathml/tests/mochitest.ini were a newly created file instead of as a diff between the current version and your changed version.

I fixed the mochitest.ini part of the patch for you.

(In reply to Aniket Deshpande from comment #27) > > Are the children commented out? > > > I forgot to remove the comments. I still see them in the latest patch, so I'm not sure if the test is really working. > > @@ +60,5 @@ > > > + window.removeEventListener("mouseout", doStopPropagation, true); > > > + window.removeEventListener("mouseenter", doStopPropagation, true); > > > + window.removeEventListener("mouseleave", doStopPropagation, true); > > > + > > > + > > > > Can you explain what you are testing? > > I took reference from layout/mathml/tests/test_bug706406.html (maction > toggle) > So, the tests are before adding the EventListener ,after adding the > EventListener and finally removing the EventListener. The tooltip will be > triggered on mouseover. The pop-up will be above or below the desired > text/equation. It should not be eiether left or right. Some comments: 1) I think we can forget the doStopPropagation stuff or the relative position of the tooltip for now. What we really want is to test whether a) the tooltip is not shown by default b) it appears when you move the mouse over it c) it disappears when you move the mouse out. 2) Before writing a mochitest, I think you should try on a local HTML page to see if you can find some Javascript code to test the visibility on the tooltip and debug exactly what's happening in your Firefox build. I'm not sure testing the positions as you did will be enough, but I don't understand what you did anyway (see below). 3) Are you sure that the bottom/right/top/left attributes on the MathML elements really work? AFAIK, existing tests rely on https://developer.mozilla.org/en-US/docs/Web/API/element.getBoundingClientRect instead. 4) Even if they are available, testing e.g. maction_tooltip.top == maction_tooltip.top (the top side of the maction is the top side of the maction???) or maction_tooltip.left == maction_tooltip.right (the left side of the maction is the right side of the maction????) as you do does not make sense to me. I suspect they are just all undefined and thus the test is vacuous.

(In reply to Bill Gianopoulos [:WG9s] from comment #29) > Created attachment 8477982 [details] [diff] [review] > test_bug969906.patch - corrected > > I fixed the mochitest.ini part of the patch for you. Thank you :WG9s for updating the patch.

(In reply to Frédéric Wang (:fredw) from comment #30) > (In reply to Aniket Deshpande from comment #27) > > > Are the children commented out? > > > > > I forgot to remove the comments. > > I still see them in the latest patch, so I'm not sure if the test is really > working. The patch is updated for mochitest.ini only. Will update it. > > > > @@ +60,5 @@ > > > > + window.removeEventListener("mouseout", doStopPropagation, true); > > > > + window.removeEventListener("mouseenter", doStopPropagation, true); > > > > + window.removeEventListener("mouseleave", doStopPropagation, true); > > > > + > > > > + > > > > > > Can you explain what you are testing? > > > > I took reference from layout/mathml/tests/test_bug706406.html (maction > > toggle) > > So, the tests are before adding the EventListener ,after adding the > > EventListener and finally removing the EventListener. The tooltip will be > > triggered on mouseover. The pop-up will be above or below the desired > > text/equation. It should not be eiether left or right. > > Some comments: > > 1) I think we can forget the doStopPropagation stuff or the relative > position of the tooltip for now. What we really want is to test whether a) > the tooltip is not shown by default b) it appears when you move the mouse > over it c) it disappears when you move the mouse out. > > 2) Before writing a mochitest, I think you should try on a local HTML page > to see if you can find some Javascript code to test the visibility on the > tooltip and debug exactly what's happening in your Firefox build. I'm not > sure testing the positions as you did will be enough, but I don't understand > what you did anyway (see below). > > 3) Are you sure that the bottom/right/top/left attributes on the MathML > elements really work? AFAIK, existing tests rely on > https://developer.mozilla.org/en-US/docs/Web/API/element. > getBoundingClientRect instead. > > 4) Even if they are available, testing e.g. maction_tooltip.top == > maction_tooltip.top (the top side of the maction is the top side of the > maction???) or maction_tooltip.left == maction_tooltip.right (the left side > of the maction is the right side of the maction????) as you do does not make > sense to me. I suspect they are just all undefined and thus the test is > vacuous. Thank you for the comments. This is very helpful.

This test case is showing the the tooltip content by deafult(no effect on mouseover and mouseout). Also, content under the mpsace element is not visible.

(In reply to Aniket Deshpande from comment #33) > Created attachment 8478874 [details] > test_tooltip.html > > This test case is showing the the tooltip content by deafult(no effect on > mouseover and mouseout). Also, content under the mpsace element is not > visible. mmmh, this does not seem valid MathML. Did you try http://html5.validator.nu/ and check the MathML spec? The mspace should be empty and maction with tooltip can only have two children.

Thank you for bringing that to notice. I corrected the errors. But there is a issue here, 1)<maction actiontype="tooltip"> <mtext>y</mtext> <mspace width="100px" height="20px" mathbackground="red"></mspace> </maction> ignores mspace 2)<maction actiontype="tooltip"> <mspace width="100px" height="20px" mathbackground="red"></mspace> <mtext>y</mtext> </maction> does not display the text. and the problem with comment 33 still persists.

(In reply to Aniket Deshpande from comment #35) > Thank you for bringing that to notice. I corrected the errors. But there is > a issue here, This is the expected result since maction@tooltip is not supported... But do you mean that for a build with attachment 8474225 [details] [diff] [review] applied it does not work either?

(In reply to Frédéric Wang (:fredw) from comment #36) > (In reply to Aniket Deshpande from comment #35) > > Thank you for bringing that to notice. I corrected the errors. But there is > > a issue here, > > This is the expected result since maction@tooltip is not supported... But do > you mean that for a build with attachment 8474225 [details] [diff] [review] > applied it does not work either? I think so.. yes. But before that i used qpush to apply the patch. I just need to confirm i am not doing anything incorrect. Also, is there any other way?

(In reply to Aniket Deshpande from comment #37) > I think so.. yes. But before that i used qpush to apply the patch. I just > need to confirm i am not doing anything incorrect. Also, is there any other > way? I'm not sure what your problem is, but with Bill's build (http://www.wg9s.com/mozilla/firefox/) I see the space & Y tooltips on your test of comment 35 when I pass the mouse over the MathML. And https://bug544001.bugzilla.mozilla.org/attachment.cgi?id=600000 also works for me. Just a stupid question: did you rebuild Firefox after having applied the patch?

(In reply to Frédéric Wang (:fredw) from comment #38) > (In reply to Aniket Deshpande from comment #37) > > I think so.. yes. But before that i used qpush to apply the patch. I just > > need to confirm i am not doing anything incorrect. Also, is there any other > > way? > > I'm not sure what your problem is, but with Bill's build > (http://www.wg9s.com/mozilla/firefox/) I see the space & Y tooltips on your > test of comment 35 when I pass the mouse over the MathML. And > https://bug544001.bugzilla.mozilla.org/attachment.cgi?id=600000 also works > for me. > It's just appears to be the problem with my build.I'll retry with another build. > Just a stupid question: did you rebuild Firefox after having applied the > patch? Yes, I did.

(In reply to Aniket Deshpande from comment #37) > (In reply to Frédéric Wang (:fredw) from comment #36) > > (In reply to Aniket Deshpande from comment #35) > > > Thank you for bringing that to notice. I corrected the errors. But there is > > > a issue here, > > > > This is the expected result since maction@tooltip is not supported... But do > > you mean that for a build with attachment 8474225 [details] [diff] [review] > > applied it does not work either? > > I think so.. yes. But before that i used qpush to apply the patch. I just > need to confirm i am not doing anything incorrect. Also, is there any other > way? I don't usually work on more than one patch at a time, so I have never used patch queues. I normally just clone the repository and then user hg import to apply the patch.

(In reply to Frédéric Wang (:fredw) from comment #36) > (In reply to Aniket Deshpande from comment #35) > > Thank you for bringing that to notice. I corrected the errors. But there is > > a issue here, > > This is the expected result since maction@tooltip is not supported... But do > you mean that for a build with attachment 8474225 [details] [diff] [review] > applied it does not work either? On my builds, running the test in the bug attachment, the red field shifts one character width to the right upon mouseover and does not shift back on mouseout. I am not sure if this is an issue with the test or with the patch being tested.

Same patch updated to work after landing of bug 1195162

Just fixing a typo.