Bug 72828 - MathML operators not stretched horizontally
 Summary: MathML operators not stretched horizontally
 Product: Classification: Status: RESOLVED FIXED WebKit Unclassified MathML Version: 528+ (Nightly build) Platform: All All Importance: P2 Normal Assigned To: Frédéric Wang (:fredw) Keywords: 130322 mathml-in-mathjax 99614 134021 Show dependency tree / graph

 See Also: Reported: 2011-11-20 11:41 PST by Dave Barton Modified: 2014-07-21 10:55 PDT (History) CC List: 15 users (show) ap bfulgham bunhere cdumez cfleizach commit-queue donggwan.kim esprehn+autocc fred.wang glenn gyuyoung.kim innovimax kondapallykalyan mrobinson sergio

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.
 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. Frédéric Wang (:fredw) 2014-04-15 06:30:23 PDT Created attachment 229368 [details] Another testcase for STIX Math Frédéric Wang (:fredw) 2014-04-15 06:34:04 PDT Created attachment 229369 [details] Patch 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. 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? 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. Frédéric Wang (:fredw) 2014-05-25 04:28:29 PDT Created attachment 232034 [details] Patch Frédéric Wang (:fredw) 2014-05-25 06:17:16 PDT Created attachment 232035 [details] Patch Frédéric Wang (:fredw) 2014-05-25 07:28:13 PDT Created attachment 232036 [details] Patch 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? Frédéric Wang (:fredw) 2014-06-04 09:04:44 PDT Created attachment 232483 [details] Patch 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. 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 Frédéric Wang (:fredw) 2014-06-04 23:35:41 PDT Committed r169607:  Alexey Proskuryakov 2014-06-05 13:21:38 PDT Tests added here have missing results. Can you please land the results? 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? 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. Frédéric Wang (:fredw) 2014-06-06 00:27:07 PDT Committed r169643:  Frédéric Wang (:fredw) 2014-06-06 00:37:55 PDT Committed r169644:  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. 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.