Summary:  MathML operators not stretched horizontally  

Product:  WebKit  Reporter:  Dave Barton <dbarton>  
Component:  MathML  Assignee:  Frédéric Wang (:fredw) <fred.wang>  
Status:  RESOLVED FIXED  
Severity:  Normal  CC:  ap, bfulgham, bunhere, cdumez, cfleizach, commitqueue, donggwan.kim, esprehn+autocc, fred.wang, glenn, gyuyoung.kim, innovimax, kondapallykalyan, mrobinson, sergio  
Priority:  P2  
Version:  528+ (Nightly build)  
Hardware:  All  
OS:  All  
Bug Depends on:  130322  
Bug Blocks:  84019, 99614, 134021  
Attachments: 

Created attachment 229368 [details]
Another testcase for STIX Math
Created attachment 229369 [details]
Patch
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. (In reply to comment #2) > Created an attachment (id=229369) [details] > Patch Did you want this to be reviewed? (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. Created attachment 232034 [details]
Patch
Created attachment 232035 [details]
Patch
Created attachment 232036 [details]
Patch
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 reuse 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? Created attachment 232483 [details]
Patch
(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 nonzero 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 reuse 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. (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 nonzero 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 reuse 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 Committed r169607: <http://trac.webkit.org/changeset/169607> Tests added here have missing results. Can you please land the results? (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? (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 webkitpatch rebaseline can get new results or not. Committed r169643: <http://trac.webkit.org/changeset/169643> Committed r169644: <http://trac.webkit.org/changeset/169644> What are the expected results for opentypestretchyhorizontal? 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. (In reply to comment #19) > What are the expected results for opentypestretchyhorizontal? 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/opentypestretchyhorizontalexpected.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. 
Created attachment 115987 [details] mover expr ~ doesn't stretch ~ MathML operators are currently stretched vertically, but not horizontally.