Bug 108778 - Add support for mathvariants that cannot be emulated via CSS
: Add support for mathvariants that cannot be emulated via CSS
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: MathML
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Frédéric Wang (:fredw)
http://tests.mathml-association.org/m...
:
Depends on: 133845 155018
Blocks: 159623 mathvariant
  Show dependency treegraph
 
Reported: 2013-02-03 16:36 PST by Martin Robinson
Modified: 2016-07-11 11:39 PDT (History)
15 users (show)

See Also:


Attachments
Patch (26.58 KB, patch)
2013-02-03 16:48 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Try to fix the Mac build (26.59 KB, patch)
2013-02-03 17:21 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
font-style=italic in WebKit VS italic math alphanum in Gecko (Latin Modern Math) (16.10 KB, image/png)
2014-07-17 21:39 PDT, Frédéric Wang (:fredw)
no flags Details
Patch (205.77 KB, patch)
2016-03-17 07:57 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (207.50 KB, patch)
2016-04-26 09:02 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (207.46 KB, patch)
2016-05-17 06:40 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (207.45 KB, patch)
2016-06-25 00:38 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (207.42 KB, patch)
2016-07-08 00:24 PDT, Frédéric Wang (:fredw)
buildbot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (1.10 MB, application/zip)
2016-07-08 01:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.11 MB, application/zip)
2016-07-08 01:14 PDT, Build Bot
no flags Details
Patch (207.47 KB, patch)
2016-07-08 02:23 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (207.47 KB, patch)
2016-07-08 02:51 PDT, Frédéric Wang (:fredw)
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2013-02-03 16:36:15 PST
Some mathvariant choices cannot be emulated via CSS, so they must be implemented via character replacement. These include double-struck, Fraktur, bold Fraktur, script,  and bold script.
Comment 1 Martin Robinson 2013-02-03 16:48:21 PST
Created attachment 186277 [details]
Patch
Comment 2 Martin Robinson 2013-02-03 17:00:00 PST
A few notes to reviewers about this patch: First, this is all quite new to me, so I wouldn't be surprised if the patch is wildly wrong. Apologies in advance and thanks for any guidance you can give me!

The use of DOM replacement here isn't ideal, since it's a one way transformation. If JavaScript changes the mathvariant attribute, then the change won't be reflected properly. It seems that the way to handle this case properly is to create a RenderMathMLText and have it reflect the transformed text.

To be completely honest, I took this approach initially as it's a similar situation to CSS text transformations. The issue is that in the case of MathML mathvariant, the transformation is from characters in the BMP to out of the BMP. In practical terms this means that the UTF-16 encoded text value of the RenderText will be a different length than the DOM string (non-BMP characters can take two UTF-16 code units). This causes havoc with the Range and TextIterator classes, leading, for instance, to corrupted paste data.

The issues with Range and TextIterator are probably solvable issues (and probably should be solved), but as I was looking at it, I noticed a startling large amount of yak hair accumulating on the floor. I went instead with this simpler approach that was less correct. 

The other thing to notice about this patch is that I did not do mappings for transformations that are now handled via CSS. I hope to do that in a future patch. I see mapping even CSS-emulatable mathvariants as a good thing, because it means that you can copy and paste while maintaining some semantic meaning. The MathML spec stresses that mathvariant carries semantics.

Finally, judging by https://bugzilla.mozilla.org/show_bug.cgi?id=449396 it seems that the mathvariant  attribute shouldn't be inherited. This is reflected in my patch. It doesn't look like we support mstyle yet.
Comment 3 Build Bot 2013-02-03 17:19:25 PST
Comment on attachment 186277 [details]
Patch

Attachment 186277 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16355890
Comment 4 Martin Robinson 2013-02-03 17:21:48 PST
Created attachment 186282 [details]
Try to fix the Mac build
Comment 5 Martin Robinson 2013-03-04 11:52:03 PST
Comment on attachment 186282 [details]
Try to fix the Mac build

Clearing the review flag. Daniel and I talked about this patch and my approach is indeed problematic.
Comment 6 Frédéric Wang (:fredw) 2013-03-14 02:30:43 PDT
To follow-up yesterday discussion and for the record:

- I have the impression that the attribute is still used a lot. I asked to the LaTeXML folks to directly use the Unicode code point but MathJax still relies on mathvariant. So that's why this is blocking the MathJax tracking bug :)

- As Martin said above, mathvariant is available on <mstyle> too. Although I think most of mstyle should be removed (http://lists.w3.org/Archives/Public/www-math/2012Sep/0002.html) I believe mstyle@mathvariant is actually one of the attribute used in real content.

- Like Neil and David said, some characters are invariant to italic change for example U+210E (PLANCK CONSTANT) and the italic style should not be applied to them. Similarly, style="font-weight: bold;" should in theory not apply to e.g. fraktur letters.

- MathJax and Gecko "implements" this via CSS using different fonts. So instead of remapping the Unicode code point, they change the style (italic, bold, font-face). That could be a temporary solution. See http://mxr.mozilla.org/mozilla-central/source/layout/mathml/mathml.css#124

As Dave said, I plan to implement that in Gecko using a private -moz-mathvariant CSS property. Basically, the mathvariant attribute on mstyle and token elements will be mapped to this property and then the text rendering code will use that property to choose the right transform to perform. So I agree with you that this is related to the italic mi issue and could be handled the same way.
Comment 7 Frédéric Wang (:fredw) 2013-09-30 02:04:31 PDT
(In reply to comment #6)
> As Dave said, I plan to implement that in Gecko using a private -moz-mathvariant CSS property. Basically, the mathvariant attribute on mstyle and token elements will be mapped to this property and then the text rendering code will use that property to choose the right transform to perform. So I agree with you that this is related to the italic mi issue and could be handled the same way.

FYI, James Kitchener has taken over my work and is currently implementing that via -moz-mathvariant CSS property and text transformations:
https://bugzilla.mozilla.org/show_bug.cgi?id=114365
Comment 8 Frédéric Wang (:fredw) 2014-03-10 06:19:57 PDT
I was about to try to work on this bug. My plan was:

- add a CSS property mathvariant that is inherited and has initial value "auto".

- rules in mathml.css should be replaced by direct mapping in Source/WebCore/mathml/ for math/mstyle and token elements.

- when mathvariant is "auto", RenderMathMLToken::updateStyle should switch to "italic" for single char <mi> (at the moment it updates the font-style, which is not quite correct and does not work well with inherited mathvariant values)

- add a TextMathTransformer class that would be called in RenderText to do a similar job as applyTextTransform. The RenderStyle will be used to decide the mathvariant.

Now I noticed Martin's comment about the issue with change of string length, so that will need more work in Range and TextIterator...

BTW, Gecko's implementation has more transforms and mathvariant values, see https://hg.mozilla.org/mozilla-central/file/923f1411f42f/layout/generic/MathMLTextRunFactory.cpp. The case of italic/bold/bold-italic is not handled yet (https://bugzilla.mozilla.org/show_bug.cgi?id=930504).

I think this bug becomes important to migrate to MATH fonts, since they don't have separate font files for italic/bold style but only a single font with the mathvariants in the Plane 1.
Comment 9 Frédéric Wang (:fredw) 2014-07-17 21:39:01 PDT
Created attachment 235110 [details]
font-style=italic in WebKit VS italic math alphanum in Gecko (Latin Modern Math)
Comment 10 Frédéric Wang (:fredw) 2015-12-23 00:21:17 PST
Thinking again about how to fix this bug, I believe we could just add a special case in the RenderMathMLToken class to paint mathvariant-transformed characters with a non-standard code path (like what we do for RenderMathMLOperator to draw stretchy operators). For example, it seems we could rely on the TextPainter class for that purpose. Of course, just like RenderMathMLOperator this could cause other issues like (e.g. for selection) but will allow to get correct rendering.

In my opinion, the priority is to support the "automatic mathvariant italic" for single-char <mi> because the current text-style=italic CSS gives poor rendering (cf attachment 235110 [details]).
Comment 11 Frédéric Wang (:fredw) 2016-02-05 06:11:29 PST
(In reply to comment #10)
> In my opinion, the priority is to support the "automatic mathvariant italic"
> for single-char <mi> because the current text-style=italic CSS gives poor
> rendering (cf attachment 235110 [details]).

This one is now done in Alex's MathMLLayout branch.
Comment 12 Frédéric Wang (:fredw) 2016-03-17 07:57:41 PDT
Created attachment 274292 [details]
Patch
Comment 13 Frédéric Wang (:fredw) 2016-04-26 09:02:18 PDT
Created attachment 277380 [details]
Patch
Comment 14 Frédéric Wang (:fredw) 2016-05-17 06:40:15 PDT
Created attachment 279117 [details]
Patch

Updating test expectation for iOS.
Comment 15 Frédéric Wang (:fredw) 2016-06-25 00:38:58 PDT
Created attachment 282054 [details]
Patch
Comment 16 Frédéric Wang (:fredw) 2016-07-08 00:24:43 PDT
Created attachment 283115 [details]
Patch
Comment 17 Build Bot 2016-07-08 01:12:17 PDT
Comment on attachment 283115 [details]
Patch

Attachment 283115 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1645514

New failing tests:
imported/mathml-in-html5/mathml/relations/css-styling/mathvariant-transforms-1.html
Comment 18 Build Bot 2016-07-08 01:12:21 PDT
Created attachment 283120 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 19 Build Bot 2016-07-08 01:14:54 PDT
Comment on attachment 283115 [details]
Patch

Attachment 283115 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1645517

New failing tests:
imported/mathml-in-html5/mathml/relations/css-styling/mathvariant-transforms-1.html
Comment 20 Build Bot 2016-07-08 01:14:59 PDT
Created attachment 283121 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 21 Frédéric Wang (:fredw) 2016-07-08 02:23:49 PDT
Created attachment 283130 [details]
Patch

Fixing path to the woff font in the test...
Comment 22 Frédéric Wang (:fredw) 2016-07-08 02:51:58 PDT
Created attachment 283132 [details]
Patch

Fix error in debug build
Comment 23 Brent Fulgham 2016-07-11 09:45:04 PDT
Comment on attachment 283132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283132&action=review

I hd a few minor nits with the change, but overall looks good and does not seem to introduce any regressions. r=me.

> Source/WebCore/rendering/mathml/MathMLStyle.cpp:192
> +    // The mathvariant attribute on the <math>, <mstyle> or token elements override the default behavior.

// The mathvariant attribute .... OVERRIDES the default behavior.

<nitpick>

> Source/WebCore/rendering/mathml/MathMLStyle.h:46
> +    // The special value none means that no explicit mathvariant value has been specified.

// The special value 'None' ...

> Source/WebCore/rendering/mathml/MathMLStyle.h:47
> +    // Note that the values are important for the computation perform in the mathVariant function of RenderMathMLToken, do not change them!

// Note that the NUMERICAL values are important for the computation PERFORMED in the mathVariant function of RenderMathMLToken, do not change them!

> Source/WebCore/rendering/mathml/MathMLStyle.h:83
> +    MathVariant m_mathVariant = None;

These would be better as C++11 style:

bool m_displayStyle { false };
MathVariant m_mathVariant { None };

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:63
> +// replacement represents the mapped mathvariant Unicode character.

I suggest that 'key' and 'replacement' be quoted to make it clear that you are referring to variables in the struct below (not just generic concepts of key and replacement.)

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:68
> +static inline UChar32 ExtractKey(const MathVariantMapping* entry) { return entry->key; }

This crashes if 'entry' is null. If 'entry' is guaranteed to always have a value, could this be a const&?

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:70
> +MathVariantMappingSearch(uint32_t key, const MathVariantMapping* table, size_t tableLength)

Weird newline after UChar32 here.

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:72
> +    if (const MathVariantMapping* entry = tryBinarySearch<const MathVariantMapping, UChar32>(table, tableLength, key, ExtractKey))

This would read better as:

If (const auto* entry = ...

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:289
> +static UChar32 mathVariant(UChar32& codePoint, MathMLStyle::MathVariant mathvariant)

It doesn't appear that codePoint is ever mutated in this function. I suggest that it be made a const&.
Comment 24 Frédéric Wang (:fredw) 2016-07-11 11:37:01 PDT
(In reply to comment #23)
> > Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:68
> > +static inline UChar32 ExtractKey(const MathVariantMapping* entry) { return entry->key; }
> 
> This crashes if 'entry' is null. If 'entry' is guaranteed to always have a
> value, could this be a const&?

As I see in binarySearchImpl from WTF/wtf/StdLibExtras.h, the API assumes that the parameter is a pointer. It is also always guaranteed to be non-null.
Comment 25 Frédéric Wang (:fredw) 2016-07-11 11:39:28 PDT
Committed r203072: <http://trac.webkit.org/changeset/203072>