Bug 72828 - MathML operators not stretched horizontally
Summary: MathML operators not stretched horizontally
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 130322
Blocks: mathml-in-mathjax 99614 134021
  Show dependency treegraph
 
Reported: 2011-11-20 11:41 PST by Dave Barton
Modified: 2014-07-21 10:55 PDT (History)
15 users (show)

See Also:


Attachments
mover expr ~ doesn't stretch ~ (52 bytes, text/html)
2011-11-20 11:41 PST, Dave Barton
no flags Details
Another testcase for STIX Math (12.46 KB, text/html)
2014-04-15 06:30 PDT, Frédéric Wang (:fredw)
no flags Details
Patch (23.34 KB, patch)
2014-04-15 06:34 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Screenshot of the testcase (WebKitGTK+) (24.85 KB, image/png)
2014-04-15 06:35 PDT, Frédéric Wang (:fredw)
no flags Details
Patch (98.83 KB, patch)
2014-05-25 04:28 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (87.15 KB, patch)
2014-05-25 06:17 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (98.84 KB, patch)
2014-05-25 07:28 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (99.34 KB, patch)
2014-06-04 09:04 PDT, Frédéric Wang (:fredw)
cfleizach: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Barton 2011-11-20 11:41:18 PST
Created attachment 115987 [details]
mover expr ~ doesn't stretch ~

MathML operators are currently stretched vertically, but not horizontally.
Comment 1 Frédéric Wang (:fredw) 2014-04-15 06:30:23 PDT
Created attachment 229368 [details]
Another testcase for STIX Math
Comment 2 Frédéric Wang (:fredw) 2014-04-15 06:34:04 PDT
Created attachment 229369 [details]
Patch
Comment 3 Frédéric Wang (:fredw) 2014-04-15 06:35:23 PDT
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.
Comment 4 Brent Fulgham 2014-04-29 16:51:43 PDT
(In reply to comment #2)
> Created an attachment (id=229369) [details]
> Patch

Did you want this to be reviewed?
Comment 5 Frédéric Wang (:fredw) 2014-05-03 05:54:33 PDT
(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.
Comment 6 Frédéric Wang (:fredw) 2014-05-25 04:28:29 PDT
Created attachment 232034 [details]
Patch
Comment 7 Frédéric Wang (:fredw) 2014-05-25 06:17:16 PDT
Created attachment 232035 [details]
Patch
Comment 8 Frédéric Wang (:fredw) 2014-05-25 07:28:13 PDT
Created attachment 232036 [details]
Patch
Comment 9 chris fleizach 2014-05-27 09:48:31 PDT
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?
Comment 10 Frédéric Wang (:fredw) 2014-06-04 09:04:44 PDT
Created attachment 232483 [details]
Patch
Comment 11 Frédéric Wang (:fredw) 2014-06-04 09:13:38 PDT
(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.
Comment 12 chris fleizach 2014-06-04 18:19:49 PDT
(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
Comment 13 Frédéric Wang (:fredw) 2014-06-04 23:35:41 PDT
Committed r169607: <http://trac.webkit.org/changeset/169607>
Comment 14 Alexey Proskuryakov 2014-06-05 13:21:38 PDT
Tests added here have missing results. Can you please land the results?
Comment 15 Frédéric Wang (:fredw) 2014-06-05 13:24:17 PDT
(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?
Comment 16 Martin Robinson 2014-06-05 17:23:59 PDT
(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.
Comment 17 Frédéric Wang (:fredw) 2014-06-06 00:27:07 PDT
Committed r169643: <http://trac.webkit.org/changeset/169643>
Comment 18 Frédéric Wang (:fredw) 2014-06-06 00:37:55 PDT
Committed r169644: <http://trac.webkit.org/changeset/169644>
Comment 19 Alexey Proskuryakov 2014-07-21 10:43:37 PDT
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.
Comment 20 Frédéric Wang (:fredw) 2014-07-21 10:55:24 PDT
(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.