Open the testcase and select the text. All the letters and symbols in the math correctly draw white, but the fraction line, radical signs and parentheses are red. I have a patch for this, but it's an identical change across 4 files so should probably be refactored to avoid code duplication.

Does the change make ::selection match all the parts of the maths?

Good question. Unfortunately, I haven't figured out how to put ::selection in my CSS. Why doesn't "math::selection {background-color: lime}" work? Debug Mozilla says: "CSS Error: Expected identifier for pseudo-class selector not found ':'. Selector expected Ruleset ignored due to bad selector." Can you point me to some CSS with ::selection?

Oh, you have to use :-moz-selection at the moment. (The parser doesn't have support for '::' yet.)

Um.... -moz-selection shouldn't work either, should it? ::selection will work once glazou checks in his patch. The parser handles :: OK (not perfect, but OK) last I checked.

Oh? I thought the '::' patch wasn't checked in, and the ':-moz-selection' patch was. Certainly the pseudo-elements in html.css don't have double '::' yet.

:-moz-selection works for me. Certain constructs in MathML don't respect it, however. I'll try to figure out why and improve my patch.

1. Fixes the bug. Also works correctly WRT CSS-specified selection colors. Also works correctly WRT disabled windows. 2. Now draws the line for <mfrac> last, which looks better when selected. 3. Doesn't work with Print Selection. But I don't think any MathML works with Print Selection. 4. Makes bug 175850 more noticeable. (since you tend to get white on white)

Any hope for this patch getting into the tree? I find the selection drawing bugs (bug 175845 and bug 175850 -- both addressed in the patch) annoying.

What happens when a table cell is selected? Seems to me that a better home for the added methods is nsMathMLFrame instead? I don't like very much the fact that nsMathMLContainerFrame is argument. + if (IsMathFrameSelected(aPresContext)) { Simply call this |IsMathMLFrameSelected| +PRBool +nsMathMLContainerFrame::CurrentBackGroundSelectionColor(nsIPresContext* aPresContext, My Personal pref would have been to use the names GetForeGroundSelectionColor, GetBackGroundSelectionColor. + if (IsFrameInSelection(aPresContext, firstChild)) { + isSelected = PR_TRUE; + } isSelected = IsFrameInSelection(aPresContext, firstChild);

I'm not sure what you mean by the table cell remark. If the MathML is inside a table cell, selecting the cell seems to do the right thing (the cell is outlined, but the content is not drawn as selected.) If you're talking about selecting cells inside an mtable, that's not really possible. Last I checked, the special behavior for selecting table cells only worked inside <table>, <tr>, <td> and not inside <mtable>, <mtr>, <mtd>. Of course, it would be nice to fix this eventually. As for your comments on the patch -- feel free to move/rename the code as you see fit. My point was just to figure out how to look up the style information to draw things right.

>As for your comments on the patch -- feel free to move/rename the code as you >see fit. Want to try out to move?

I tried. It didn't work. |IsMathMLFrameSelected| needs to call |nsFrame::GetSelected|. I guess you could put the function out on nsMathMLFrame, but it couldn't do anything meaningful until there was a concrete frame around.

I changed the names as suggested, though I used |GetForeGroundColor| instead of |GetForeGroundSelectionColor| since the color is returned even without selection.

Let me have a shot to see how it goes. If the move proves messy, we will go with your patch.

Comment on attachment 120094 [details] [diff] [review] cleaned up patch I applied your patch. I am afraid it makes things worse and inconsistent. I will attach a screenshot for illustration.

This is how I migrated the functions to nsMathMLFrame. But the earlier screenshot was with your original patch. Same troubles were carried forward here.

The inconsistency is not my fault. All my patch tries to do is correctly draws frames which are selected. If you select with the mouse linearly, only leaf text frames get selected. You can use Select All to test my drawing patch. I've noticed these drawing inconsistencies with the mouse even without the patch. Simply spin the selection around the page a few times. You'll likely find frames where the background is drawn selected while the foreground isn't. It's as though the selection state changes between drawing the two layers. Eventually, the poor selection behavior in MathML should be corrected. Fot those not familiar with the issue, MathML have some objects which are different from anything in HTML. For example an mfrac like {a+b}/{c+d}. It really doesn't make sense to select from the b to the c. And the mfrac occupies a frame which is larger than the union of it's children. But this knowledge lies out in the mfrac, not in the contained text. I haven't seen a simple way to get the Mozilla selection code to query parents about the validity of selections. There are comments in the Mozilla code that changing to "geometric" selection would fix this problem. I can write JavaScript which selects the various frames and can detect situations like the above fraction. Unfortunately, when I hooked this into the mouse handling event, it usually didn't work. It seemed like some other code was interfering and making sure the selection didn't extend beyond the logical caret. In any case, JavaScript is not the right place for selection logic. However, my patch still fixes a valid bug. If a MathML frame is selected (however that happens), it SHOULD draw correctly.

Your patch (id=120174) is better than mine. I was afraid to touch |GetSelected|, but that seems to be the cleanest thing to do. As for the nsMathMLFrame stuff, I don't see why you prefer a static function taking a frame argument over a member function on the frame itself. But it doesn't really matter.

I intended to separate the majors from the minors. The alternative was your earlier approach, which unfortunately had excessive C++ which ultimately meant passing the awkward container as argument later. Whereas I am passing the leaner interface class earlier. Different perspectives, I suppose. But there is a curious sense of safety/friendliness with those self-contained |static|. > Eventually, the poor selection behavior in MathML should be corrected. Sure. That's why you should iterate on your patch to resolve the review comments. It is customary to go through several iterations. So don't take it personally. > However, my patch still fixes a valid bug. Only partially. It fixes the rare case of selectAll, but it unfortunatley introduces other troubles for the most common cases of partial selection. E.g., unreadable dark text in a selected area; radical symbol selected here, but not there. Leaving the user confused as to why there are such inconsistencies, and what it means if they were to do something with the selection (e.g., they can ask what would happen to the missed symbols). So, just go ahead and iterate if you can. Also mention the limitations so that people don't build unexpected expectations when trying the patch :-) I sympathize with you that the default selection behavior isn't suitable for MathML, but won't be looking at it myself right now.

Roger, I'm sorry if it sounds like I'm taking this personally or getting all excited. I'd just like to resolve this issue. I do appreciate the time you're spending on it. In my mind, there are two separate bugs. 1. The drawing bug listed in the Subject, 2. The selection behavior of Mozilla in MathML. Because I can write JavaScript which correctly selects nodes, I find it annoying that bug #1 exists. I have no route though JS to correct this. Hence my patch. I have spent many days studying bug #2 and would love to find a fix. So far, I haven't seen a path to a fix which didn't go through nsTextFrame and the selection components. I'm afraid. Both that these are very tricky modules and that changes there are less likely to be accepted. Are you sure that the drawing of dark text on a selected background is due to this patch? I've seen this happen in non-patched versions. Sorry, but I find Mozilla's current selection behavior to be troubling anyway. If you select a radical, what happens? What if you select from b to c in $\sqrt{a+b}+\frac{c}{d}$? What if you select an mfenced? I don't think the patch makes that any worse. To wrap things up, I'll 1. Drop back to a non-patched Mozilla and verify the drawing problems I claimed above. Did your sample document involve any BiDi script? If so, is it publicly accessible? 2. Look at the selection issues again. I completely understand that you don't want to go there.

http://www.mozilla.org/projects/mathml/start.xml http://www.mozilla.org/projects/mathml/start-thai.xml > If you select a radical, what happens? What if you select from b to > c in $\sqrt{a+b}+\frac{c}{d}$? With my unpatched Nav7, only the _text_ gets selected at present, which admittedly remains intuitive/consistent -- although not ideal from your JS editor point of view.

Thank you for your patience. I agree there is a problem with the patch. In particular, the issues in the screenshot don't happen without the patch. My guess is that the patched code is drawing the true state of the frames in memory. That is, some of the parent frames remain selected, either though all their children (and all the other content on the line) are unselected. It's going to take a little while to prove this conclusively, but that's the only explanation I can think of. My feeling is that sometimes selection does a SelectAllChildren kind of thing, and sometimes it iterates over text nodes. The items which draw incorrectly are exactly those where the two selection methods yield different results. So, in the end, I agree. The patch does need another iteration.

FWIW. Your patch introduces an additional problem which mine doesn't seem to have. If I select an <mo>, I get a black rectangle with your patch. I'm guessing this is because of your override of |GetSelected|, though I don't see why the background ever comes out black.

On Firefox 1.0 this works as expected. All the math encapsulated 'code' renders red. Selecting the formulae doesn't do any weird colouring. The only thing that does not get selected are the root, divion lines, and braces. Is this bug still valid?

Yes, the bug is still valid. > The only thing that does not get selected are the root, divion lines, and braces. That's the point. They shouldn't draw with the foreground color when they're selected.

For the parts displayed with nsMathMLChar, we need to use the special drawing for selection. For other graphic components like those in https://hg.mozilla.org/mozilla-central/rev/ff353339df3 we can use LookAndFeel::eColorID_TextSelectForeground but then the drawing may become invisible. So it seems necessary to add the corresponding "draw background" routines for each graphic component and they should be called before any foreground component is drawn.