1027354 - Deprecated MathML fontstyle and fontweight completely ignored

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Paul Masson]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

Create a vector using <mi fontstyle="normal" fontweight="bold">v</mi>


Actual results:

Renders like <mi>v</mi> without any styles applied


Expected results:

The specification says that when mathvariant is present it should override deprecated styles, but it is not present in this markup. Shouldn't deprecated styles still be implemented for the near future when ignoring them changes the meaning of the math? Also, this markup passes validation with http://validator.w3.org/nu/.

The release notes https://developer.mozilla.org/en-US/Firefox/Releases/28 indicate that support for mathvariant has been added but not that deprecated styles are no longer supported. Is there another forum in which significant changes of this sort are announced?

Comment 1

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

The attributes still seem to work with <mtext>. Also <mi style="font-family: normal">v</mi> is with normal style. I believe what's happening is that single-char <mi> have an implicit mathvariant="italic" value which overrides the style on it.

One question: where did you find the markup with these deprecated attributes and / or CSS style? The tools / pages should definitely be updated to use mathvariant or style instead, and I think Firefox has been sending warning to the console about these deprecated attributes. Note that fontstyle/fontweight or equivalent CSS style don't have semantics meaning in MathML, mathvariant is supposed to be used for that purpose.

Comment 2

[James Kitchener (:jkitch)]

I've found the code responsible.

Getting fontweight="bold", style="font-weight:bold" and fontstyle="normal" to work again on single char <mi> is easy.  

Getting style="font-style:normal" to work is non-trivial and will take some thought.

To maximise the probability of this being uplifted to aurora/beta, are you happy for this bug to neglect style="font-style:normal"?

Comment 3

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

That seems inconsistent. What about making only the fontstyle and fontweight attributes work again on single char <mi>?

Comment 4

[Paul Masson]

This markup didn't come from any Mozilla page. I started using MathML in early 2003 with the WebEQ applet for Internet Explorer. Their implementation of MathML 1 didn't include mathvariant, so I just got used to doing vectors this way. If I've gotten by without mathvariant this long then there are likely to be other legacy documents online without it, so if it's not too much trouble it makes sense to me to render fontstyle and fontweight when they appear without any mathvariant. FYI MathJax renders my example with styles applied.

Comment 5

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

I don't really know what the spec says with mixed implicit mathvariant on single-char mi and deprecaded fontstyle/fontweight although I think it makes sense to make the latter overrides the former. Note that in theory, the MathML spec says that

fontstyle/fontweight = style="..." = CSS style

while

mathvariant = Mathematical Alphanumeric Symbols Unicode Block = mathematical semantics

Until now, the difference was not necessarily obvious because we followed the comment of the spec that "it is acceptable to implement basic mathvariants via CSS styles". However, starting with Gecko 31, we will move to OpenType MATH fonts that have a single "Regular" font file and italic/bold/bold-italic chars in the Mathematical Alphanumeric Symbols block. This means that <mtext mathvariant="italic">x</mtext> and <mtext style="font-style: italic">x</mtext> might not render the same for these fonts. So I'd recommend to update your documents to remove the deprecated attributes and use mathvariant instead.

(Note that MathJax does really follow the MathML spec here and instead implements mathvariant using one font file for each mathvariant).

Comment 6

[James Kitchener (:jkitch)]

Attachment: Fix only fontweight and fontstyle attributes

This patch disables mathvariant styling for single char <mi> if the fontweight="bold" or fontstyle="normal" attributes are used.  CSS or style="x" modifications to these values are ignored.

I suppose the flags could be added to nsTextFrameUtils instead.  However the MathML stuff is self contained and I am also planning on adding some more flags in a future patch.

I also have another attempt which supports CSS styling by resurrecting some code removed by the initial mathvariant patch.
https://tbpl.mozilla.org/?tree=Try&rev=a53109507eab
However the side effects include subtle modifications to font metrics, so I am not comfortable with promoting that to aurora/beta.

Comment 7

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

Comment on attachment 8443924 [details] [diff] [review]
Fix only fontweight and fontstyle attributes

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

mmh, I don't really like that now each single char <mi> has to check parent ancestors for possible fontweight (we had perf/update issues with mstyle that used to do the same)... Perhaps we could restrict the support to only fontweight on the <mi> (I think that's what Paul used to do, but his web site now uses mathvariant anyway). Actually, I think we should probably drop support for obsolete attributes at some point and ask people to move to mathvariant, because as I said above they are now different with MATH fonts...

Karl?

::: layout/generic/MathMLTextRunFactory.cpp
@@ +598,5 @@
>      if (singleCharMI && mathVar == NS_MATHML_MATHVARIANT_NONE) {
> +      // If the user has explicitely set a non-default value for fontstyle or
> +      // font weight, the italic mathvariant behaviour of <mi> is disabled
> +      // This overrides the initial values specified in fontStyle, to avoid
> +      // inconsistencies in which vraiables allow CSS changes and which do not.

vraiables?

::: layout/generic/nsTextFrame.cpp
@@ +1925,5 @@
> +            }
> +            if (attrValue.EqualsLiteral("normal")) {
> +              break;
> +            }
> +            //Otherwise an invalid option.  Continue looking

missing space after //

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8443924 [details] [diff] [review]
Fix only fontweight and fontstyle attributes

(In reply to Frédéric Wang (:fredw) from comment #7)
> mmh, I don't really like that now each single char <mi> has to check parent
> ancestors for possible fontweight (we had perf/update issues with mstyle
> that used to do the same)... Perhaps we could restrict the support to only
> fontweight on the <mi> (I think that's what Paul used to do, but his web
> site now uses mathvariant anyway).

I hadn't noticed that fontweight was inherited, but I agree: I think/hope we can get away with only checking for fontweight on <mi>.  Having fontweight inherited while changing the fontstyle to italic or normal on <mi> was probably not so useful anyway.

Comment 9

[James Kitchener (:jkitch)]

Attachment: Fix only fontweight and fontstyle attributes

This restores functionality of fontweight="bold" or fontstyle="normal" when set on an <mi> element.  Inherited fontweight or style="" attributes are not supported.

Comment 10

[James Kitchener (:jkitch)]

https://tbpl.mozilla.org/?tree=Try&rev=cc1676975187

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72f554d3e1c8

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72f554d3e1c8

Comment 13

[James Kitchener (:jkitch)]

Comment on attachment 8445815 [details] [diff] [review]
Fix only fontweight and fontstyle attributes

Approval Request Comment
[Feature/regressing bug #]: Bug 114365
[User impact if declined]: Deprecated fontstyle and fontweight attributes will have no effect for certain <mi> elements
[Describe test coverage new/current, TBPL]: Patch includes appropriate tests, has been on MC for several days.
Try when applied to aurora:
https://tbpl.mozilla.org/?tree=Try&rev=2b8391db1e27
I presume the e10s and Marionette failures are symptoms of running on an older branch.
[Risks and why]: This patch has been specially designed to minimise risk.  The extra logic is only triggered by the single situation which it is meant to fix (<mi> with single character).  Everything which is dereferenced is null-checked.  There are no side effects.
[String/UUID change made/needed]: None

Comment 14

[James Kitchener (:jkitch)]

Attachment: rebased patch for beta

Approval Request Comment
[Feature/regressing bug #]: Bug 114365
[User impact if declined]: Deprecated fontstyle and fontweight attributes will have no effect for certain <mi> elements
[Describe test coverage new/current, TBPL]: Patch includes appropriate tests, has been on MC for several days.
Try when applied to beta:
https://tbpl.mozilla.org/?tree=Try&rev=0019d5605aa5
I presume the e10s and Marionette failures are symptoms of running on an older branch.
[Risks and why]: This patch has been specially designed to minimise risk.  The extra logic is only triggered by the single situation which it is meant to fix (<mi> with single character).  Everything which is dereferenced is null-checked.  There are no side effects.
[String/UUID change made/needed]: None

Comment 15

[Sylvestre Ledru [:Sylvestre]]

We only accept critical fixes in beta. I am not sure this bug fit into this category.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7d454f6db4d5