487587 - In mathml formulas the content of many <mo> elements is not highlighted when selected

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[alberthilbert]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.8) Gecko/2009033100 Ubuntu/9.04 (jaunty) Firefox/3.0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.8) Gecko/2009033100 Ubuntu/9.04 (jaunty) Firefox/3.0.8

In mathml formulas the content of some <mo> elements is selectable, the content of a lot of others is not. For example, if the <mo> content is "+", "-", "(" or "=", the element is not user-selectable, it is selectable if its content is "&pm;", "&oplus", "&otimes" etc. I think <mo> elements should be selectable in all cases. In particular I want to write a javascript wysiwyg mathml editor for firefox and I want to use the text caret to "navigate" the formula, but with the described behavior of the browser that is impossible.
Thank you...

Reproducible: Always

Comment 1

[Karl Tomlinson (:karlt)]

Thanks for the report.
It's actually possible to select the elements, but it's pretty difficult when there is no highlight to indicate that they are selected.
This happens with <mo> elements that draw using nsMathMLChar.
ISTR the highlight being visible with Firefox 2.

Comment 2

[Frédéric Wang Nélar]

Attachment: Patch

It seems that the problem is located in nsMathMLmoFrame::IsFrameInSelection, where we always read a state bit set to false for the frame. Removing the code checking this bit state seems to work, except that the selection is not always cleared as expected.

Also, the selection rectangle has not the appropriate size for all operators (particularly composite chars). Using the nsRect of mMathMLChar gives a better result.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ehsan, you're changing around how this works, right?

Comment 4

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Ehsan, you're changing around how this works, right?

If you mean storing the selection bits on the content nodes, Mats is working on it.  But I don't think that should affect this bug.

Comment 5

[Frédéric Wang Nélar]

I guess the changes to the selection bits have fix the bug with the aFrame->GetSelected(). So it remains only to apply the second change of this patch to use the right bounding box (especially for characters built by parts).

Comment 6

[Frédéric Wang Nélar]

Attachment: Patch V2

Updated patch that includes only the fix to the computation of the selected rectangle, since the detection of the selection itself seems to work now.

As a testcase, you can try

https://developer.mozilla.org/en-US/docs/Mozilla_MathML_Project/MathML_Torture_Test

For example "select all" and look at the minus/plus signs in 6 & 9, the summation symbols in 12 or the parentheses and vertical bars in 23 & 24.

Note: this small patch only addresses the case of mo elements, there are other bugs like bug 175845, bug 175850, bug 759462 for further improvements.

Comment 7

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

Comment on attachment 678107 [details] [diff] [review]
Patch V2

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

::: layout/mathml/nsMathMLmoFrame.cpp
@@ +101,5 @@
>      nsIFrame* firstChild = mFrames.FirstChild();
>      if (IsFrameInSelection(firstChild)) {
> +      mMathMLChar.GetRect(selectedRect);
> +      // add a one pixel border (it renders better for operators like minus)
> +      selectedRect.Inflate(nsPresContext::CSSPixelsToAppUnits(1));

Can you explain more about why we need this?

Comment 8

[Frédéric Wang Nélar]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Comment on attachment 678107 [details] [diff] [review]
> Patch V2
> 
> Review of attachment 678107 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/mathml/nsMathMLmoFrame.cpp
> @@ +101,5 @@
> >      nsIFrame* firstChild = mFrames.FirstChild();
> >      if (IsFrameInSelection(firstChild)) {
> > +      mMathMLChar.GetRect(selectedRect);
> > +      // add a one pixel border (it renders better for operators like minus)
> > +      selectedRect.Inflate(nsPresContext::CSSPixelsToAppUnits(1));
> 
> Can you explain more about why we need this?

For example for minus, the height of the box may be just 1px and the minus may have the same color as the page background. If it is not surrounded by the selection background, the operator may not be visible.

Comment 9

[Frédéric Wang Nélar]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Comment on attachment 678107 [details] [diff] [review]
> Patch V2
> 
> Review of attachment 678107 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/mathml/nsMathMLmoFrame.cpp
> @@ +101,5 @@
> >      nsIFrame* firstChild = mFrames.FirstChild();
> >      if (IsFrameInSelection(firstChild)) {
> > +      mMathMLChar.GetRect(selectedRect);
> > +      // add a one pixel border (it renders better for operators like minus)
> > +      selectedRect.Inflate(nsPresContext::CSSPixelsToAppUnits(1));
> 
> Can you explain more about why we need this?

Or perhaps you meant why changing the selectedRect is needed at all? Using the rect of the text frame is just wrong, what we need is the rect computed by the Stretch routine which is in general larger. You can see that with large operators like the summation symbol but it is even more clear with parentheses stretched vertically.

Comment 10

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

How about using the visual overflow area of the text frame, then?

Comment 11

[Frédéric Wang Nélar]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> How about using the visual overflow area of the text frame, then?

The point is that these operators are drawn and measured in a separate module nsMathMLChar. For example to draw a radical we need to vertically align several pieces. I don't think the text frame is touched at all so if we measure it, we get the rect of a radical character at normal size which is much smaller than the rect of the stretched radical.

What we do here is just to use the text frame to determine whether the operator is selected. Probably mouse selection relies on the rect or visual overflow area of the text frame which is wrong too. Perhaps the right thing would be to update the rect or visual overflow area of the text frame after the character has been stretched by nsMathMLChar. Is it possible to do so and how?

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> How about using the visual overflow area of the text frame, then?

For these |useMathMLChar| mo frames, the text frame is not used to draw any text.  It is quite the wrong size, which leads to the current problems with display of the selection background.  (It also seems to be poorly placed, towards the top left.)

The nsMathMLChar rect however differs from text frames in that its rect is based on tight ink bounds, and so the Inflate().

Using the child text frame for detecting selection may not be the best option, but that is quite a separate problem.

Comment 13

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

Comment on attachment 678107 [details] [diff] [review]
Patch V2

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

OK, I'm convinced :-)

Comment 14

[Frédéric Wang Nélar]

https://tbpl.mozilla.org/?tree=Try&rev=f3a0b8fe20c4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c0965b86033

Seems like this should have a test?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c0965b86033