175845 - drawing of <mfrac>, <msqrt>, <mroot> and <mfenced> is wrong color when selected

Status: NEW

(Core :: MathML, defect, P5)

Description

[steve.swanson]

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.

Comment 1

[steve.swanson]

Attachment: simple testcase

Comment 2

[Hixie (not reading bugmail)]

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

Comment 3

[steve.swanson]

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?

Comment 4

[Hixie (not reading bugmail)]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Hixie (not reading bugmail)]

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.

Comment 7

[steve.swanson]

:-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.

Comment 8

[steve.swanson]

Attachment: 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)

Comment 9

[steve.swanson]

Attachment: upgraded patch works in 1.4a

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.

Comment 10

[rbs]

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);

Comment 11

[steve.swanson]

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.

Comment 12

[rbs]

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

Want to try out to move?

Comment 13

[steve.swanson]

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.

Comment 14

[steve.swanson]

Attachment: cleaned up patch

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

Comment 15

[rbs]

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

Comment 16

[rbs]

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.

Comment 17

[rbs]

Attachment: screenshot with patch 120094

Comment 18

[rbs]

Attachment: FYI - patch to move the functions to nsMathMLFrame

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

Comment 19

[steve.swanson]

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.

Comment 20

[steve.swanson]

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.

Comment 21

[rbs]

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.

Comment 22

[steve.swanson]

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.

Comment 23

[rbs]

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.

Comment 24

[steve.swanson]

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.

Comment 25

[steve.swanson]

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.

Comment 26

[Jeroen Ruigrok van der Werven]

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?

Comment 27

[steve.swanson]

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.

Comment 28

[Frédéric Wang (:fredw)]

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.