Closed Bug 129560 Opened 20 years ago Closed 20 years ago

Selection doesn't display on stretchy MathML characters

Categories

(Core :: MathML, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: rbs, Assigned: rbs)

Details

Attachments

(3 files, 3 obsolete files)

Characters that are handled by the nsMathMLChar class don't appear as selected.
Attached file testcase
Attachment #73060 - Attachment is patch: false
Attachment #73060 - Attachment mime type: text/plain → application/xhtml+xml
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...
Status: NEW → ASSIGNED
Attached patch partial patch for the <mo> case (obsolete) — Splinter Review
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.
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.
Attachment #75735 - Attachment is obsolete: true
Attachment #75738 - Attachment is obsolete: true
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.
Target Milestone: --- → mozilla1.0
Also includes various entities which select as <mi> but not as <mo>.
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.
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 on attachment 75929 [details] [diff] [review]
patch to enable the selection of stretchy chars in <mo>

next iteration will tidy things a bit more
Attachment #75929 - Attachment is obsolete: true
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.
>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.
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...)
RE: comment #13

Note that in certain areas of mathematics, symbols such as &Sum; are used as 
identifiers, not operators. MathML makes it easy to adjust the semantics 
(unfortunately?). With TeX it's hard, but do-able.
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.
So, why is the visibility no longer checked?

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

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

+  if (!visib->IsVisible())
+    return NS_OK;
Comment on attachment 76380 [details] [diff] [review]
updated patch - ready for r/sr

I knew that - just testing you ;)

sr=attinasi
Attachment #76380 - Flags: superreview+
Summary: Selection doesn't display on stretchy MathML characters → [fix, have sr, awaiting r/a] Selection doesn't display on stretchy MathML characters
Comment on attachment 76380 [details] [diff] [review]
updated patch - ready for r/sr

r=dbaron
Attachment #76380 - Flags: review+
Summary: [fix, have sr, awaiting r/a] Selection doesn't display on stretchy MathML characters → [fix, have r/sr, awaiting a] Selection doesn't display on stretchy MathML characters
Whiteboard: have r=dbaron, sr=attinasi
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
Attachment #76380 - Flags: approval+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Summary: [fix, have r/sr, awaiting a] Selection doesn't display on stretchy MathML characters → Selection doesn't display on stretchy MathML characters
Whiteboard: have r=dbaron, sr=attinasi
Checked that testcases in comment #1, comment #8 and comment #12 work as
expected on Win2k (with my font setup).
Status: RESOLVED → VERIFIED
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).
You need to log in before you can comment on or make changes to this bug.