759462 - Some MathML elements ignore ::-moz-selection colors

Status: NEW

(Core :: MathML, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: test case

If you load the attached testcase and then select all text, you will see that the parentheses are highlighted differently than everything else.  This is because ::-moz-selection has been used to change the highlight colors, but some MathML elements are still being highlighted with the system theme's highlight colors.  In this case, it is <mo>(</mo> and <mo>)</mo>, but the bug is not simply "<mo> does not respect ::-moz-selection"; the contents of the element matter, and I have seen the phenomenon with other MathML constructs (though I'm not sure exactly which).

I will attach a screen shot.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: screen shot, Firefox 11, Linux

Comment 2

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

(In reply to Zack Weinberg (:zwol) from comment #0)
> Created attachment 628061 [details]
> test case
> 
the contents of
> the element matter, and I have seen the phenomenon with other MathML
> constructs (though I'm not sure exactly which).

Certainly the mo drawn with nsMathMLChar. You shouldn't have the problem if you replace <mo> by <mtext>, or "(" by "a".

Comment 3

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

Here are the possible places to modify:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLChar.cpp#1816
http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLChar.cpp#2033

Comment 4

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

(In reply to Zack Weinberg (:zwol) from comment #0)
> I have seen the phenomenon with other MathML
> constructs (though I'm not sure exactly which).

This commit should contain all the places to modify:
https://hg.mozilla.org/mozilla-central/rev/ff353339df3

CC'ing Boris, who did these changes in bug 667576.

Comment 5

[Zack Weinberg (:zwol)]

(In reply to Frédéric Wang (:fredw) from comment #2)
>
> Certainly the mo drawn with nsMathMLChar. You shouldn't have the problem if
> you replace <mo> by <mtext>, or "(" by "a".

Yes, I observed the problem go away in both those cases; but I don't trust that the problem is limited to nsMathMLChar.


(In reply to Frédéric Wang (:fredw) from comment #4)
> 
> This commit should contain all the places to modify:
> https://hg.mozilla.org/mozilla-central/rev/ff353339df3

That can't be right; background-color is affected as well as color.

Comment 6

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

(In reply to Zack Weinberg (:zwol) from comment #5)
> Yes, I observed the problem go away in both those cases; but I don't trust
> that the problem is limited to nsMathMLChar.
> 

I think the problem is limited to nsMathMLChar and all the elements that use specific graphic constructions. So everything in the commit mentioned in comment #4.

> 
> (In reply to Frédéric Wang (:fredw) from comment #4)
> > 
> > This commit should contain all the places to modify:
> > https://hg.mozilla.org/mozilla-central/rev/ff353339df3
> 
> That can't be right; background-color is affected as well as color.

I'm not sure what you mean here. But there are actually two problems here:

- there are MathML selection bugs like bug 487587, bug 175845 and bug 175850, which affect nsMathMLChar and graphic components. Currently only nsMathMLChar uses LookAndFeel::eColorID_TextSelectForeground and draw a background when selected (although the selection size may be incorrect).

- Even if the bugs are fixed, the colors used for the foreground/background would rely on what is given by LookAndFeel. Apparently these colors are not those specified by ::-moz-selection and that's what you report here.

Comment 7

[Zack Weinberg (:zwol)]

I don't really understand how all this stuff works, but I *think* what I meant was that the commit you pointed out appears to only contain code related to problem 1, not problem 2.

Comment 8

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

Just a quick update: attachment 106311 [details] [diff] [review] that was proposed 10 years ago to fix bug 175845 and bug 175850 seems to contain some code to get the pseudo style context from -moz-selection. So perhaps the three bugs could actually be fixed in one go.

Comment 9

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

@Anuj: The first step would be to fix the case of <mo> operators drawn with nsMathMLChar, which is enough for Zack's testcase. If you look for "LookAndFeel" in layout/mathml/nsMathMLChar.cpp, you should find two places (background and foreground). As you can see, the ::-moz-selection pseudo element is not taken into account. Compare with how nsTextPaintStyle::InitSelectionColorsAndShadow (in layout/generic/nsTextFrame.cpp) determines the background/foreground color.

Comment 10

[Thomas Wisniewski]

Attachment: 759462-use_selection_colors_for_more_mathml_elements.diff

Here's a patch that applies the foreground/background colors from -moz-selection to more MathML bits, falling back to the look&feel colors (and taking it into account whether the tab is inactive and such).

One thing I noticed while testing is that Windows and GTK seem to default to using white as the selection foreground color, effectively making notations and bars vanish when they're selected (if the background is white, like on MDN pages). I'm not sure what to do about this. I was considering just using the selection *background* color instead of the foreground one for those cases, but I'm not sure if that's a good idea. Thoughts?

There are are a few other things that I'm wondering:

1. Would it be best to also handle -moz-selection text-shadows in this patch as well, or file a follow-up bug to handle that later?

2. If we're going to paint all the MathML decorations/symbols as though they're "selected", shouldn't a copy+paste operation pick them up too? (If so, how should that work? If not, then should I only apply the fore/background selection colors to <mo> elements, rather than all the other places?)

3. Right now the separators and open/closing symbols of <mfence> aren't easy to select. How should we deal with that, given that they're not copy+pastable either? (bars are also not particularly easy to select, in case we'd like to handle that case as well).

Comment 11

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

(In reply to Thomas Wisniewski from comment #10)
> Created attachment 8818743 [details] [diff] [review]
> 759462-use_selection_colors_for_more_mathml_elements.diff
> 
> Here's a patch that applies the foreground/background colors from
> -moz-selection to more MathML bits, falling back to the look&feel colors
> (and taking it into account whether the tab is inactive and such).

Thanks for working on this! I'll try and take a look this week-end.
Note that you'll need to write tests for this. See https://developer.mozilla.org/en-US/docs/Mozilla/Creating_reftest-based_unit_tests and layout/reftests/mathml

> One thing I noticed while testing is that Windows and GTK seem to default to
> using white as the selection foreground color, effectively making notations
> and bars vanish when they're selected (if the background is white, like on
> MDN pages). I'm not sure what to do about this. I was considering just using
> the selection *background* color instead of the foreground one for those
> cases, but I'm not sure if that's a good idea. Thoughts?

Mmh, that seems to be a platform bug unrelated to MathML. Maybe ask to Boris Barsky or Karl Tomlinson (cc'ed).

> There are are a few other things that I'm wondering:
> 
> 1. Would it be best to also handle -moz-selection text-shadows in this patch
> as well, or file a follow-up bug to handle that later?

I guess better doing that in a separate patch. Actually, I think it's bug 827039.

> 2. If we're going to paint all the MathML decorations/symbols as though
> they're "selected", shouldn't a copy+paste operation pick them up too? (If
> so, how should that work? If not, then should I only apply the
> fore/background selection colors to <mo> elements, rather than all the other
> places?)

I guess we want all bars and operators (and everything that uses the CSS "color" property) to be painted with -moz-selection color when selected. "Copy" currently only copies text flavor. See bug 539506 for adding a MathML flavor (and also https://addons.mozilla.org/en-US/firefox/addon/mathml-copy/ for a workaround)
 
> 3. Right now the separators and open/closing symbols of <mfence> aren't easy
> to select. How should we deal with that, given that they're not
> copy+pastable either? (bars are also not particularly easy to select, in
> case we'd like to handle that case as well).

I think we can ignore issues with <mfenced>. Technically, this element can be expanded to an equivalent <mrow>+<mo> form and I believe should be removed from the MathML spec (or at least from web engines). It has other issues like open/close/separators text not being accessible to Accessibility Technologies, not copyable etc See also https://people.igalia.com/fwang/mfenced-polyfill/

Comment 12

[Boris Zbarsky [:bzbarsky]]

> effectively making notations and bars vanish when they're selected 
> (if the background is white, like on MDN pages).

Why are we painting the selection foreground color on top of something that is not the selection background color?

Comment 13

[Bill Gianopoulos [:WG9s]]

I have (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #12)
> > effectively making notations and bars vanish when they're selected 
> > (if the background is white, like on MDN pages).
> 
> Why are we painting the selection foreground color on top of something that
> is not the selection background color?

I have asked this same question before and never got a satisfying answer.

Comment 14

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #13)
> I have (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #12)
> > > effectively making notations and bars vanish when they're selected 
> > > (if the background is white, like on MDN pages).
> > 
> > Why are we painting the selection foreground color on top of something that
> > is not the selection background color?
> 
> I have asked this same question before and never got a satisfying answer.

This issue is not restricted to MathML code either.

Comment 15

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

Thomas: Can you please reply to Boris' question? Is it something introduced in your patch or is it not specific to MathML (as Bill said)? If it's the latter, is there a bug entry for that?

Comment 16

[Thomas Wisniewski]

Attachment: 759462-use_selection_colors_for_more_mathml_elements.diff

Sure, I'm certainly not against just drawing a selection-background rect for <menclose> notations and bars. In fact here's a patch which just uses a slightly-inflated version of bars as their background, and uses the rect mapped-out by notation arrows/lines/etc (which I'm guessing would suffice).

I'm just not sure if I should be adding a new constant for the new classes in these lines, or if it's safe to just re-use the TYPE_vars for the foreground values:
>NS_DISPLAY_DECL_NAME("MathMLBarSelectionBackground", TYPE_MATHML_BAR)
>NS_DISPLAY_DECL_NAME("MathMLMencloseNotationSelectionBackground", TYPE_MATHML_MENCLOSE_NOTATION)

Also, thanks for the other notes and bug-references. Once we decide how to bake this patch, I'll try to find time to work on the text-shadow case.

Comment 17

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

Thanks Thomas. I see that this bug is related to bug 175845 and that the issue with the missing background is bug 175850.

Comment 18

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

Attachment: Testcase

@Thomas: Here is a new test. Can you please check that it covers all the changes you've made in your patch? And can you also please convert it into a reftest and integrate it into your patch (see https://developer.mozilla.org/en-US/docs/Mozilla/Creating_reftest-based_unit_tests and layout/reftests/text-shadow/text-shadow-on-selection-1.html)? Thanks!

Comment 19

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

Comment on attachment 8819679 [details] [diff] [review]
759462-use_selection_colors_for_more_mathml_elements.diff

Review of attachment 8819679 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/mathml/nsMathMLChar.cpp
@@ +2051,5 @@
>      styleContext = parentContext;
>    }
>  
>    RefPtr<gfxContext> thebesContext = aRenderingContext.ThebesContext();
> +  nscolor fgColor = nsFrameSelection::GetForegroundColor(aFrame, aIsSelected);

With this change, the styleContext variable above becomes unused, causing a warning-treated-as-error for me.

Comment 20

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

Comment on attachment 8819679 [details] [diff] [review]
759462-use_selection_colors_for_more_mathml_elements.diff

Review of attachment 8819679 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling review after the previous feedback.

::: layout/mathml/nsMathMLFrame.cpp
@@ +383,4 @@
>    }
>  #ifdef NS_BUILD_REFCNT_LOGGING
>    virtual ~nsDisplayMathMLBar() {
> +    MOZ_COUNT_DTOR(nsDisplayMathMLBarSelectionBackground);

And this function should be named ~nsDisplayMathMLBarSelectionBackground)

Comment 21

[Thomas Wisniewski]

Thanks, fredw! I'll integrate the test you provided with my next patch, but I'm thinking that I might have a better approach to handling the color-detection than the one I came up with in this current patch, which I'll try to piece together this weekend.

In spirit it's a similar idea, but I'd rather re-use the nsTextPaintStyle class from nsTextFrame instead of having semi-duplicated code like this. It seems doable by just moving nsTextPaintStyle out of nsTextFrame and using it in the MathML classes as well, and would lay the groundwork for fixing not just this bug, but also bug 292563 (and should be extensible for text-shadows as well, so we can do a follow-up to add support for selection-shadows to these MathML objects as well).

Comment 22

[Bill Gianopoulos [:WG9s]]

(In reply to Thomas Wisniewski from comment #21)
> Thanks, fredw! I'll integrate the test you provided with my next patch, but
> I'm thinking that I might have a better approach to handling the
> color-detection than the one I came up with in this current patch, which
> I'll try to piece together this weekend.
> 
> In spirit it's a similar idea, but I'd rather re-use the nsTextPaintStyle
> class from nsTextFrame instead of having semi-duplicated code like this. It
> seems doable by just moving nsTextPaintStyle out of nsTextFrame and using it
> in the MathML classes as well, and would lay the groundwork for fixing not
> just this bug, but also bug 292563 (and should be extensible for
> text-shadows as well, so we can do a follow-up to add support for
> selection-shadows to these MathML objects as well).

That sounds like a great idea.  Consolidating code rather than duplicating it is always preferred.  that way we don't end up with future fixes only landing in one place leaving the copy buggy as well as just reducing the overall size and complexity of the codebase.

Comment 23

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

(In reply to Thomas Wisniewski from comment #21)
> Thanks, fredw! I'll integrate the test you provided with my next patch, but
> I'm thinking that I might have a better approach to handling the
> color-detection than the one I came up with in this current patch, which
> I'll try to piece together this weekend.
> 
> In spirit it's a similar idea, but I'd rather re-use the nsTextPaintStyle
> class from nsTextFrame instead of having semi-duplicated code like this. It
> seems doable by just moving nsTextPaintStyle out of nsTextFrame and using it
> in the MathML classes as well, and would lay the groundwork for fixing not
> just this bug, but also bug 292563 (and should be extensible for
> text-shadows as well, so we can do a follow-up to add support for
> selection-shadows to these MathML objects as well).

That sounds good. BTW, there was another error in the name of the destructor (class nsDisplayNotationSelectionBackground of nsMathMLmencloseFrame.cpp). Maybe you should try building in debug and with treat-warning-as-error.

I think drawing some background around bars and menclose notations seem OK and similar to what will be needed for text-shadow (although maybe the option option for bug 175850 was to draw the background of the whole mfrac/msqrt/menclose/mroot frames).

Also, in my test I use white as a background, maybe you should use a different color so that we are sure that the background is actually drawn. The idea of the reftest would then be to compare a page with moz::selection color/background & selected math against a page with normal color/background properties.