969906 - Implement maction@tooltip using XBL

Status: RESOLVED WONTFIX

(Core :: MathML, defect)

Description

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

Attachment: testcase-maction.html

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 1

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

Attachment: Patch V1

Comment 2

[Karl Tomlinson (:karlt)]

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.

Comment 3

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

(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.

Comment 4

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

Attachment: Patch V2

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

Comment 5

[Bill Gianopoulos [:WG9s]]

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

Comment 6

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

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

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

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.

Comment 8

[Aniket Deshpande]

Thanks :fredw, I'll do it.

Comment 9

[Aniket Deshpande]

Attachment: maction-tooltip.patch

Trailing space removed.

Comment 10

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

(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)

Comment 11

[Aniket Deshpande]

Attachment: trailing_spaces_969906.patch

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.

Comment 12

[Bill Gianopoulos [:WG9s]]

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.

Comment 13

[Bill Gianopoulos [:WG9s]]

Attachment: Patch V2 - updated

Updated patch to work with current mozilla-central.

Comment 14

[Bill Gianopoulos [:WG9s]]

(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">
                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Comment 15

[Bill Gianopoulos [:WG9s]]

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

Comment 16

[Bill Gianopoulos [:WG9s]]

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

Comment 17

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

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

Comment 18

[Bill Gianopoulos [:WG9s]]

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

Comment 19

[Aniket Deshpande]

Attachment: test_bug969906.html

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

Comment 20

[Aniket Deshpande]

(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.

Comment 21

[Aniket Deshpande]

Attachment: 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.)

Comment 22

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

(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 23

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

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?

Comment 24

[Aniket Deshpande]

(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.

Comment 25

[Bill Gianopoulos [:WG9s]]

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.

Comment 26

[Bill Gianopoulos [:WG9s]]

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

Comment 27

[Aniket Deshpande]

(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.

Comment 28

[Bill Gianopoulos [:WG9s]]

(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.

Comment 29

[Bill Gianopoulos [:WG9s]]

Attachment: test_bug969906.patch - corrected

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

Comment 30

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

(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.

Comment 31

[Aniket Deshpande]

(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.

Comment 32

[Aniket Deshpande]

(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.

Comment 33

[Aniket Deshpande]

Attachment: 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.

Comment 34

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

(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.

Comment 35

[Aniket Deshpande]

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.

Comment 36

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

(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?

Comment 37

[Aniket Deshpande]

(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?

Comment 38

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

(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?

Comment 39

[Aniket Deshpande]

(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.

Comment 40

[Bill Gianopoulos [:WG9s]]

(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.

Comment 41

[Bill Gianopoulos [:WG9s]]

(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.

Comment 42

[Bill Gianopoulos [:WG9s]]

Attachment: Patch V2 - updated

Same patch updated to work after landing of bug 1195162

Comment 43

[Bill Gianopoulos [:WG9s]]

Attachment: Patch V2 - updated

Just fixing a typo.

Comment 44

[Bill Gianopoulos [:WG9s]]

Attachment: Patch V2 - updated

Comment 45

[Boris Zbarsky [:bzbarsky]]

XBL is going away. Should this use webcomponents instead?

Comment 46

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

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #45)

XBL is going away. Should this use webcomponents instead?

In general the long term plan is to align on MathML Core (bug 1495813) and to implement legacy features as webcomponents/polyfills.
For maction, see https://github.com/mathml-refresh/mathml/issues/26

Comment 47

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

Mass WONTFIX for bugs related to legacy features that are not part of MathML Core (and with no plan to add them) or corresponding meta bugs. Please see bug 1495813 for the new meta bug.