Closed
Bug 129560
Opened 22 years ago
Closed 22 years ago
Selection doesn't display on stretchy MathML characters
Categories
(Core :: MathML, defect)
Core
MathML
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: rbs, Assigned: rbs)
Details
Attachments
(3 files, 3 obsolete files)
1.93 KB,
application/xhtml+xml
|
Details | |
2.14 KB,
application/xhtml+xml
|
Details | |
18.67 KB,
patch
|
dbaron
:
review+
attinasi
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
Characters that are handled by the nsMathMLChar class don't appear as selected.
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
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
Comment 8•22 years ago
|
||
Also includes various entities which select as <mi> but not as <mo>.
Comment 9•22 years ago
|
||
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.
Assignee | ||
Comment 10•22 years ago
|
||
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.
Assignee | ||
Comment 11•22 years ago
|
||
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
Comment 12•22 years ago
|
||
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>∞</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.
Assignee | ||
Comment 13•22 years ago
|
||
>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>∞</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.
Assignee | ||
Comment 14•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 16•22 years ago
|
||
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•22 years ago
|
||
So, why is the visibility no longer checked? - if (visib->IsVisible() && NS_FRAME_PAINT_LAYER_FOREGROUND == aWhichLayer) { + if (NS_FRAME_PAINT_LAYER_FOREGROUND == aWhichLayer) {
Assignee | ||
Comment 18•22 years ago
|
||
It is still checked -- for an early exit higher up: + if (!visib->IsVisible()) + return NS_OK;
Comment 19•22 years ago
|
||
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 21•22 years ago
|
||
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+
Assignee | ||
Comment 22•22 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 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
Comment 23•22 years ago
|
||
Checked that testcases in comment #1, comment #8 and comment #12 work as expected on Win2k (with my font setup).
Status: RESOLVED → VERIFIED
Comment 24•19 years ago
|
||
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.
Description
•