Bug 159513 - MathOperator: Add a mapping from combining to non-combining equivalents
Summary: MathOperator: Add a mapping from combining to non-combining equivalents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on:
Blocks: 161189
  Show dependency treegraph
 
Reported: 2016-07-07 09:07 PDT by Frédéric Wang (:fredw)
Modified: 2016-08-25 06:59 PDT (History)
9 users (show)

See Also:


Attachments
Patch (WIP) (5.38 KB, patch)
2016-07-22 07:53 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (10.94 KB, patch)
2016-07-25 06:32 PDT, Frédéric Wang (:fredw)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2016-07-07 09:07:19 PDT
Many math fonts provide stretch variants and assemblies for combining characters instead of their non-combining equivalent contrary to what MathML says (https://www.w3.org/TR/MathML3/chapter7.html#chars.comb-chars). For example ̲ U+0332 COMBINING LOW LINE instead of _ U+005F LOW LINE. We should have mapping from combining to non-combining forms in order to be able to use the corresponding variant and assemblies from these math fonts.
Comment 1 Frédéric Wang (:fredw) 2016-07-22 00:01:09 PDT
Jacques Distler reported the same issue for ¯ U+00AF MACRON which does not have stretchy variants/assembly in Latin Modern Math. That's also true for U+203E OVERLINE or U+0304 COMBINING MACRON. However, COMBINING OVERLINE U+0305 has the desired strechy data. I think all these overbars should try and find an equivalent constructions in the similar characters. Probably the issue does not happen with Gecko because it relies on its Unicode fallback.
Comment 2 Frédéric Wang (:fredw) 2016-07-22 07:53:00 PDT
Created attachment 284331 [details]
Patch (WIP)

Here is a WIP patch (missing a test).
Comment 3 Frédéric Wang (:fredw) 2016-07-25 06:32:13 PDT
Created attachment 284480 [details]
Patch
Comment 4 Darin Adler 2016-07-25 09:44:12 PDT
Comment on attachment 284480 [details]
Patch

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

> Source/WebCore/rendering/mathml/MathOperator.h:79
> +    bool getBaseGlyph(const RenderStyle& style, GlyphData& baseGlyph) const
> +    {
> +        return getGlyph(style, m_baseCharacter, baseGlyph);
> +    }

Including function bodies that make the function declaration take multiple lines in the class definition typically ends up messing it up over time.

I prefer the idiom where we put the inline function body outside the class definition once it’s going to take multiple lines like this.
Comment 5 Darin Adler 2016-07-25 09:44:36 PDT
Comment on attachment 284480 [details]
Patch

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

> Source/WebCore/rendering/mathml/MathOperator.cpp:180
> +    { 0x005E, 0x0302, 0 }, // CIRCUMFLEX ACCENT
> +    { 0x005F, 0x0332, 0 }, // LOW LINE
> +    { 0x007E, 0x0303, 0 }, // TILDE
> +    { 0x00AF, 0x0304, 0x0305 }, // MACRON
> +    { 0x02C6, 0x0302, 0 }, // MODIFIER LETTER CIRCUMFLEX ACCENT
> +    { 0x02C7, 0x030C, 0 } // CARON

test should cover all 7 characters, but seems to only cover one of them
Comment 6 Frédéric Wang (:fredw) 2016-07-25 09:59:25 PDT
(In reply to comment #5)
> Comment on attachment 284480 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284480&action=review
> 
> > Source/WebCore/rendering/mathml/MathOperator.cpp:180
> > +    { 0x005E, 0x0302, 0 }, // CIRCUMFLEX ACCENT
> > +    { 0x005F, 0x0332, 0 }, // LOW LINE
> > +    { 0x007E, 0x0303, 0 }, // TILDE
> > +    { 0x00AF, 0x0304, 0x0305 }, // MACRON
> > +    { 0x02C6, 0x0302, 0 }, // MODIFIER LETTER CIRCUMFLEX ACCENT
> > +    { 0x02C7, 0x030C, 0 } // CARON
> 
> test should cover all 7 characters, but seems to only cover one of them

There seems to be a bug with the width of horizontal stretchy operators that made writing tests based on box measuring or on dumping the render tree difficult and I did not want to have such mismatch test for each of the character. (additionally, I also currently have issues getting screenshots with the test runner).
Comment 7 Frédéric Wang (:fredw) 2016-07-26 00:48:10 PDT
Committed r203714: <http://trac.webkit.org/changeset/203714>
Comment 8 Frédéric Wang (:fredw) 2016-07-26 00:49:18 PDT
(In reply to comment #5)
> test should cover all 7 characters, but seems to only cover one of them

I converted the mismatch test to a pixel test, but the PNG expectations will have to be regenerated later as I can't do it myself at the moment.