MathML operators not stretched horizontally

• Component: MathML
• Status: RESOLVED FIXED

[Dave Barton]

Created attachment 115987 [details]
mover expr ~ doesn't stretch ~

MathML operators are currently stretched vertically, but not horizontally.

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

Created attachment 229368 [details]
Another testcase for STIX Math

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

Created attachment 229369 [details]
Patch

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

Created attachment 229370 [details]
Screenshot of the testcase (WebKitGTK+)

Screenshot of the testcase with STIX Math fonts and the patch applied.

Note: the patch applies on top of bug 130322.

[Brent Fulgham]

(In reply to comment #2)
> Created an attachment (id=229369) [details]
> Patch

Did you want this to be reviewed?

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

(In reply to comment #4)
> (In reply to comment #2)
> > Created an attachment (id=229369) [details] [details]
> > Patch
> 
> Did you want this to be reviewed?

This is blocked by the MATH font support, so I'd prefer to get feedback on bug 130322 first.

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

Created attachment 232034 [details]
Patch

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

Created attachment 232035 [details]
Patch

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

Created attachment 232036 [details]
Patch

[chris fleizach]

Comment on attachment 232036 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Can you add some ChangeLog comments as to what needed to happen in general to fix this

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1129
> +static const UChar horizontalOperators[] = {

can you add a comment as to where this list originated from

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1346
> +        m_maxPreferredLogicalWidth = m_minPreferredLogicalWidth = m_leadingSpace + maximumGlyphWidth + m_trailingSpace;

i've been told WK style is to not put multiple assignments on the same line (but i couldn't find it in style guidelines)

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1655
> +        float baseSize = m_isVertical ? heightForGlyph(baseGlyph) : advanceForGlyph(baseGlyph);

should advanceForGlyph be renamed to widthForGlyph to match (heightForGlyph)?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1692
> +            glyphBounds = boundsForGlyph(m_stretchyData.right());

can you put some newlines between sections here, or refactor so you can use a method to re-use this code. It looks like there's some duplication here that could be collapsed

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:114
> +        GlyphData left() const { return m_data[2]; }

left and bottom are the same data?

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

Created attachment 232483 [details]
Patch

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

(In reply to comment #9)
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1655
> > +        float baseSize = m_isVertical ? heightForGlyph(baseGlyph) : advanceForGlyph(baseGlyph);
> 
> should advanceForGlyph be renamed to widthForGlyph to match (heightForGlyph)?
> 

I prefer to keep "advance" since that's really what widthForGlyph is returning. "width" would correspond to boundsForGlyph(data).width(). Concretely, we also take into account the space around the ink of the glyph, so a U+0020 SPACE has non-zero width. I don't know what's the best for math operators but that's what computePreferredLogicalWidths would return for normal text.

> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1692
> > +            glyphBounds = boundsForGlyph(m_stretchyData.right());
> 
> can you put some newlines between sections here, or refactor so you can use a method to re-use this code. It looks like there's some duplication here that could be collapsed
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:114
> > +        GlyphData left() const { return m_data[2]; }
> 
> left and bottom are the same data?

Yes, that's on purpose. The operator is either horizontal or vertical, so there is no overlap.

[chris fleizach]

(In reply to comment #11)
> (In reply to comment #9)
> > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1655
> > > +        float baseSize = m_isVertical ? heightForGlyph(baseGlyph) : advanceForGlyph(baseGlyph);
> > 
> > should advanceForGlyph be renamed to widthForGlyph to match (heightForGlyph)?
> > 
> 
> I prefer to keep "advance" since that's really what widthForGlyph is returning. "width" would correspond to boundsForGlyph(data).width(). Concretely, we also take into account the space around the ink of the glyph, so a U+0020 SPACE has non-zero width. I don't know what's the best for math operators but that's what computePreferredLogicalWidths would return for normal text.
> 
> > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1692
> > > +            glyphBounds = boundsForGlyph(m_stretchyData.right());
> > 
> > can you put some newlines between sections here, or refactor so you can use a method to re-use this code. It looks like there's some duplication here that could be collapsed
> > 
> > > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:114
> > > +        GlyphData left() const { return m_data[2]; }
> > 
> > left and bottom are the same data?
> 
> Yes, that's on purpose. The operator is either horizontal or vertical, so there is no overlap.

Looks like patch doesn't apply, otherwise seems ok

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

Committed r169607: <http://trac.webkit.org/changeset/169607>

[Alexey Proskuryakov]

Tests added here have missing results. Can you please land the results?

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

(In reply to comment #14)
> Tests added here have missing results. Can you please land the results?

I only had the references for GTK. I tried to mark the other platforms as failing, but it seems that the bots still want the results. Can someone generate the references for other platforms? Or should I just copy those from GTK?

[Martin Robinson]

(In reply to comment #15)
> (In reply to comment #14)
> > Tests added here have missing results. Can you please land the results?
> 
> I only had the references for GTK. I tried to mark the other platforms as failing, but it seems that the bots still want the results. Can someone generate the references for other platforms? Or should I just copy those from GTK?

You should be able to get them from the bots. I'm not sure if webkit-patch rebaseline can get new results or not.

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

Committed r169643: <http://trac.webkit.org/changeset/169643>

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

Committed r169644: <http://trac.webkit.org/changeset/169644>

[Alexey Proskuryakov]

What are the expected results for opentype-stretchy-horizontal? How can I tell whether the test fails of passes?

I'm getting different results on some OS X versions, and I don't know if that's an improvement or a regression.

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

(In reply to comment #19)
> What are the expected results for opentype-stretchy-horizontal? How can I tell whether the test fails of passes?
> 
> I'm getting different results on some OS X versions, and I don't know if that's an improvement or a regression.

The rendering should look like something like LayoutTests/platform/gtk/mathml/opentype/opentype-stretchy-horizontal-expected.png:

- one rectangle glyph (actually perhaps square).
- one rectangle glyph of larger width.
- a wider construction with left/middle/right pieces and connecting rectangle glyphs.

All the glyphs should be taken from stretchy.woff font.

This corresponds to stretching horizontally an operator using the size variants (first two rectangles) or construction provided in the OpenType MATH table.