Implement maction@tooltip using XBL

NEW
Unassigned

Status

()

Core
MathML
4 years ago
6 months ago

People

(Reporter: fredw, Unassigned)

Tracking

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

Trunk
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 10 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8372872 [details]
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).
(Reporter)

Comment 1

4 years ago
Created attachment 8372873 [details] [diff] [review]
Patch V1
Attachment #8372873 - Flags: review?(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.
Attachment #8372873 - Flags: review?(karlt) → review-
(Reporter)

Comment 3

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

Comment 4

4 years ago
Created attachment 8374022 [details] [diff] [review]
Patch V2

Here is a new version that works with attachment 600000 [details].
Attachment #8372872 - Attachment is obsolete: true
Attachment #8372873 - Attachment is obsolete: true
Attachment #8374022 - Flags: review?(karlt)
(Reporter)

Updated

4 years ago
Summary: Implement actiontypes tooltip and highlight using XBL bindings → Implement maction@tooltip using XBL
(Reporter)

Updated

4 years ago
Attachment #8374022 - Flags: review?(karlt) → feedback?(karlt)
Attachment #8374022 - Flags: feedback?(karlt) → feedback+
This patch needs a test so that if another patch like the previous one for bug 687807 breaks it a test will fail.
(Reporter)

Updated

3 years ago
Assignee: fred.wang → nobody
(Reporter)

Comment 6

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

Comment 7

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

3 years ago
Thanks :fredw, I'll do it.

Comment 9

3 years ago
Created attachment 8473806 [details] [diff] [review]
maction-tooltip.patch

Trailing space removed.
(Reporter)

Comment 10

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

Updated

3 years ago
Attachment #8473806 - Attachment is obsolete: true

Comment 11

3 years ago
Created attachment 8474097 [details] [diff] [review]
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.
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.
Created attachment 8474225 [details] [diff] [review]
Patch V2 - updated

Updated patch to work with current mozilla-central.
Attachment #8374022 - Attachment is obsolete: true
Attachment #8474097 - Attachment is obsolete: true
(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!
Status: ASSIGNED → NEW
Oh and I figured this out by looking at the hg log on xbl-marquee.
(Reporter)

Comment 17

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

Comment 19

3 years ago
Created attachment 8475258 [details]
test_bug969906.html

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

Comment 20

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

3 years ago
Attachment #8475258 - Attachment is obsolete: true

Comment 21

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

Comment 22

3 years ago
(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"?
(Reporter)

Comment 23

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

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

Comment 27

3 years ago
(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.
Created attachment 8477982 [details] [diff] [review]
test_bug969906.patch - corrected

I fixed the mochitest.ini part of the patch for you.
Attachment #8475336 - Attachment is obsolete: true
(Reporter)

Comment 30

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

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

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

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

Comment 34

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

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

Comment 36

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

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

Comment 38

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

3 years ago
(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.
Created attachment 8701868 [details] [diff] [review]
Patch V2 - updated

Same patch updated to work after landing of bug 1195162
Attachment #8474225 - Attachment is obsolete: true
Created attachment 8701998 [details] [diff] [review]
Patch V2 - updated

Just fixing a typo.
Attachment #8701868 - Attachment is obsolete: true
Created attachment 8833741 [details] [diff] [review]
Patch V2 - updated
Attachment #8701998 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.