129560 - Selection doesn't display on stretchy MathML characters

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[rbs]

Characters that are handled by the nsMathMLChar class don't appear as selected.

Comment 1

[rbs]

Attachment: testcase

Comment 2

[rbs]

weirdos include the fraction/belleved bar, the square root char, the generated 
chars from <mfenced>, the generated lquote/rquote of <ms>. Since the selection 
is meant for things that can be copied/edited, consumers such as Steve's JS 
editor need to make wise usage of the selection...

Comment 3

[rbs]

Attachment: partial patch for the <mo> case

Comment 4

[rbs]

Cc:ing mjudge because there is a subtle issue with nsTextFrame::SetSelected() 
that is causing troubles. The selection bit of the text frame is not kept 100% 
in sync.

Background: the idea in the stretchy code is that, given a markup <mo>...</mo>,
the stretchy code will ignore the inner child text frame that the frame 
construction code creates and do its stretching gymnastics using a separate 
nsMathMLChar class. This allowed me to leave the dreaded nsTextFrame alone... 
Unfortunately, since nsMathMLChar only does the rendering, it misses other 
operations such as caret, selection, etc. The caret was addressed in bug 130886. 
This bug is about making the selection appear in nsMathMLChar without hopefully 
having to mess too much with nsTextFrame, thereby limiting the possibilities of 
regressions there.

To display the selection in the helper MathMLChar for <mo>...</mo>, the aim is 
simply to fetch the selection bit that is maintained by the inner child text 
frame, and pass it on to the MathMLChar. This is essentially what is happening 
in @@ -80,17 +80,33 @@ in attachment 75735 [details] [diff] [review]. It follows therefore that if the 
frame state bit of the inner child text frame is not in sync, troubles arise.

Here is what I could gather after much hacking. When expanding/retracting a 
selection with the mouse, the selection code ends up calling 
nsSelection::Extend() and then nsSelection::Collapse(). If it happens that the 
textframe has no more selected content, then Extend() will call 
nsTextFrame::SetSelected() which will unset the text frame's selection bit -- as 
expected. But then, when Collapse() arrives, it calls nsTextFrame::SetSelected() 
again and this put back the text frame's selection bit again. Hence if somebody 
higher up (the MathML container frame in this case) queries the bit later on, it 
gets the value -- that is selected.

This doesn't cause trouble in other situations because the call to Invalidate() 
in nsTextFrame::SetSelected() at the time of Extend() makes things to be drawn 
with the proper text frame's selection [Collapse() hasn't been fired yet].

But in the case of the <mo> container which queries the bit later, it only gets 
the bad bit that Collapse() has left. Collapse() -> SetSelected() seem to have 
problems handling the situation where the mouse gradually goes out of the 
textframe.

Comment 5

[rbs]

Attachment: FYI - experimental patch in nsTextFrame

For information, I attached an experimental patch in nsTextFrame that makes the
selection of stretchy chars to work quite reliably. But this patch is a quick
hack because it makes nsTextFrame::SetSelected() to always use the more
expensive code path that looks up selection details themselves.

Comment 6

[rbs]

Attachment: patch to enable the selection of stretchy chars in <mo>

Comment 7

[rbs]

Cc:ing roc & attinasi for yet another cross-disciplinary MathML patch :-(
Please guys, take a look at attachment 75929 [details] [diff] [review] for r/sr, thanks.

Comment 8

[steve.swanson]

Attachment: example containing mroot, among other things

Also includes various entities which select as <mi> but not as <mo>.

Comment 9

[steve.swanson]

I've observed another selection problem, which Roger may or may not want to 
include with this bug.

Please see my attachment {id=75987}.

If an <mroot> starts a line and you select from right to left, the radicand 
doesn't get selected.

To reproduce this, load my sample and resize the window so that the cube root 
of x is on the left side of the window.  Select a word on the end of that line, 
then use SHIFT+HOME to select to the beginning of the line. You should notice 
that the 3 is selected, but the x is not.

Comment 10

[rbs]

The patch is purposely focussing on <mo>. I hoped to keep it small and less
scary. The other stuff that you observed + my comment #2 are for another time.
And BTW, I have no clue at all why that SHIFT+HOME to select the line doesn't
work :-)

Some other notes: making the sqrt symbol itself appear as selected doesn't seem
that much useful for copy-paste purposes since it won't work. Similarly, the
generated chars that are displayed via <mfenced> are specified as attributes in
the markup, and so they cannot be copied-pasted to begin with.

Comment 11

[rbs]

Comment on attachment 75929 [details] [diff] [review]
patch to enable the selection of stretchy chars in <mo>

next iteration will tidy things a bit more

Comment 12

[steve.swanson]

I applied the patch and it worked as expected. <mo>s work much better and I 
didn't see any change in the selection behavior of anything else.

I did notice instances where the selection extends well below the bottom of the 
content. See for example http://www.mozilla.org/projects/mathml/demo/mo.xhtml 
and select the first row of the operator table.  I'm guessing this is font-
dependent (TeX fonts?) and I've noticed elsewhere that it can happen to <mi>s 
as well, eg <mi>&infin;</mi>, so the issue is probably independent of this 
particular bug.

Still, I far prefer the behavior existing in the patch to the old behavior.

Comment 13

[rbs]

>I did notice instances where the selection extends well below the bottom of the 
>content.

Fixed in the next iteration.

>I'm guessing this is font-dependent (TeX fonts?) and I've noticed elsewhere
>that it can happen to <mi>s as well, eg <mi>&infin;</mi>

Yes, it is font-dependent. I have been mostly tuning <mo> (see bug 126619). It 
is not a good practice to use <mi>--identifier-- for symbols. Not only it isn't 
good semantically, but symbols often come from weird fonts, and tunings done for 
<mo> such as in this bug help to make them look better. Maybe the tuning logic 
could be factored later as a helper function to be used by all token elements.

Comment 14

[rbs]

Attachment: updated patch - ready for r/sr

The patch is an iteration over the previous one to tidy things a bit more and
be more conservative in the change in nsTextFrame to limit its impact to the
strict mimimum needed. Since it is mainline, I will expound about that a little
more.

The primary intention of the change in nsTextFrame was to improve the movement
of the caret for operators that stretchy horizontally, by making the text frame
retain the width that it was given -- rather than letting it re-measure the
char in a standard way and bypass that value. For example, it allows the caret
to jump from the beginning to the end of a stretchy arrow. Without this change,
the caret would undershot given that GetWidth() can only measure the witdh of
the default arrow glyph of base size.

I used a rather conservative test to retain the previous behavior as much as
most a possible, but a side-benefit is a general optimization where typing at
the end of a text skips through the measurement. In the process of testing for
code coverage and regressions, I corrected something that was checked by kin in
 r 1.189 for bug 16176
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsTextFrame.cpp&root=/cvsroot&subdir=mozilla/layout/html/base/src&command=DIFF_FRAMESET&rev1=1.188&rev2=1.189


-    if (inOffset > textLength && (TEXT_TRIMMED_WS & mState)){
+    if ((hitLength == textLength) && (TEXT_TRIMMED_WS & mState)) {

|textLength| is the clean length of the compressed text, whereas |inOffset|
represents the _raw_ relative offset (i.e., it points straight at the limit of
the text data -- with non-collapsed whitespace/tabs at the begining/between
words...). Hence that part of the test was always almost true. But the
statements inside the |if| often result in a no-op and so no problems is
usually observed.

(Got more understanding as I was wading through that code but no time to lookup
bugzilla and check for fall-through fixes of other caret/selection bugs...)

Comment 15

[steve.swanson]

RE: comment #13

Note that in certain areas of mathematics, symbols such as ∑ are used as 
identifiers, not operators. MathML makes it easy to adjust the semantics 
(unfortunately?). With TeX it's hard, but do-able.

Comment 16

[rbs]

Yep, I know. That's why I said "good practice" -- and implementation wise, it 
will allow people using your editor to reap the benefits of the tunings right 
now. [I could try to factor this fixup of the fonts to other tags, but it will 
take more time, touch many files, make r=/sr=/a= more scary, and might miss 
m1.0... Do you want me to try that :-) Just joking, generalizations can come 
later.]

Note to reviewers: I did spend far more time testing/debugging the few changes I 
made in nsTextFrame to limit regressions in the editor/caret/etc, than in 
writing the rest of the patch.

Comment 17

[Marc Attinasi]

So, why is the visibility no longer checked?

-  if (visib->IsVisible() && NS_FRAME_PAINT_LAYER_FOREGROUND == aWhichLayer) {
+  if (NS_FRAME_PAINT_LAYER_FOREGROUND == aWhichLayer) {

Comment 18

[rbs]

It is still checked -- for an early exit higher up:

+  if (!visib->IsVisible())
+    return NS_OK;

Comment 19

[Marc Attinasi]

Comment on attachment 76380 [details] [diff] [review]
updated patch - ready for r/sr

I knew that - just testing you ;)

sr=attinasi

Comment 20

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 76380 [details] [diff] [review]
updated patch - ready for r/sr

r=dbaron

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 76380 [details] [diff] [review]
updated patch - ready for r/sr

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 22

[rbs]

Checked in.

Comment 23

[steve.swanson]

Checked that testcases in comment #1, comment #8 and comment #12 work as
expected on Win2k (with my font setup).

Comment 24

[Uri Bernstein]

Comment on attachment 76380 [details] [diff] [review]
updated patch - ready for r/sr

>+      if ((hitLength == textLength) && (inOffset = mContentLength) &&
...................................................^

This was meant to be '==', right?
(came accross this while testing something else).