Bug 153917

Summary: Refactor RenderMathMLFraction layout to avoid using flexbox
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: cfleizach, commit-queue, dbarton, esprehn+autocc, fred.wang, glenn, jdiggs, kondapallykalyan, mrobinson, rego, svillar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 153742    
Bug Blocks: 158937, 153918, 153991, 155639, 155714, 157943    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
svillar: review+
Patch final version
buildbot: commit-queue-
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch final version none

Alejandro G. Castro
Reported 2016-02-05 03:40:24 PST
This is the next patch in the series to remove the dependency with flexbox.
Attachments
Patch (8.62 KB, patch)
2016-02-05 03:46 PST, Alejandro G. Castro 		no flags
DetailsFormatted DiffDiff
Patch (64.57 KB, patch)
2016-03-03 08:24 PST, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (67.04 KB, patch)
2016-03-11 01:24 PST, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (66.97 KB, patch)
2016-03-31 08:17 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (67.04 KB, patch)
2016-04-01 06:10 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (67.24 KB, patch)
2016-04-07 06:24 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (67.41 KB, patch)
2016-04-08 03:59 PDT, Frédéric Wang (:fredw) 		svillar: review+
DetailsFormatted DiffDiff
Patch final version (67.36 KB, patch)
2016-04-11 05:27 PDT, Frédéric Wang (:fredw) 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (797.63 KB, application/zip)
2016-04-11 06:20 PDT, Build Bot 		no flags
Details
Patch final version (108.21 KB, patch)
2016-04-11 07:07 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Alejandro G. Castro
Comment 1 2016-02-05 03:46:57 PST
Created attachment 270744 [details] Patch
Frédéric Wang (:fredw)
Comment 2 2016-02-05 06:13:57 PST
Alex: I guess should be made "depends on" bug 153742, or at least bug 153208?
Alejandro G. Castro
Comment 3 2016-02-05 07:42:14 PST
(In reply to comment #2) > Alex: I guess should be made "depends on" bug 153742, or at least bug 153208? Yep, thanks, I forgot, we need the row bug 153208, it does not need the underover refactor.
Frédéric Wang (:fredw)
Comment 4 2016-03-03 08:24:24 PST
Created attachment 272758 [details] Patch Same as bug 153208 and bug 153742: I'm directly integrating the complete refactoring of fraction, so that we do not need to introduce flexbox functions that are unrelated to math layout, at the price of doing some test adjustments. The patch also fixes the parsing of absolute linethickness value and adds support for the AxisHeight/FractionRuleThickness MATH parameters. I think that keep the patch size relatively small, though. Cc'ing Joanie & Chris. The accessibility code uses a function to read the linethickness but the mac tests really expect the "relative value with respect to the default linethickness". Since the default linethickness depends on the font (now that the FractionRuleThickness is read from the MATH table), I've updated the function to read this relative value instead.
Frédéric Wang (:fredw)
Comment 5 2016-03-11 01:24:12 PST
Created attachment 273702 [details] Patch I'm uploading updated version of the MathML refactoring patches to solve conflicts after bug 154388 and bug 155005 ; and to do other minor changes.
Frédéric Wang (:fredw)
Comment 6 2016-03-31 08:17:52 PDT
Created attachment 275285 [details] Patch
Frédéric Wang (:fredw)
Comment 7 2016-04-01 06:10:15 PDT
Created attachment 275398 [details] Patch
Joanmarie Diggs
Comment 8 2016-04-01 07:45:53 PDT
(In reply to comment #4) > Cc'ing Joanie & Chris. The accessibility code uses a function to read the > linethickness but the mac tests really expect the "relative value with > respect to the default linethickness". Since the default linethickness > depends on the font (now that the FractionRuleThickness is read from the > MATH table), I've updated the function to read this relative value instead. I'll defer to Chris since it's his platform. But what you say sounds sane to me and, in terms of the accessibility changes, it's a very small modification. @Chris: This patch is part of a bigger set scattered throughout bugzilla. But if you have time to sanity check the one accessibility line Frédéric is modifying, that would be great. Thanks!
Joanmarie Diggs
Comment 9 2016-04-01 07:54:37 PDT
In terms of the accessibility changes (tests only, and a one-pixel change in size and position), LGTM.
Manuel Rego Casasnovas
Comment 10 2016-04-05 07:05:04 PDT
Comment on attachment 275398 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275398&action=review The patch LGTM, just a few comments inline. > LayoutTests/mathml/opentype/fraction-line-expected.html:1 > +<html> Please add <!DOCTYPE html> unless you want to explicitly test quirks mode. The same applies to the rest of the tests. > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:81 > +RenderBox& RenderMathMLFraction::getNumerator() const Couldn't we just call it "numerator()"? > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:87 > +RenderBox& RenderMathMLFraction::getDenominator() const And here "denominator()"? > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:212 > + IntPoint adjustedPaintOffset = roundedIntPoint(paintOffset + location() + LayoutPoint(0, getNumerator().offsetHeight() + gNumeratorGap * style().fontSize() + m_lineThickness / 2)); Why we use the numerator for the height but not for the width?
Frédéric Wang (:fredw)
Comment 11 2016-04-05 08:10:43 PDT
(In reply to comment #10) > Couldn't we just call it "numerator()"? > > And here "denominator()"? Yes, that seems better. Probably the same should be done in bug 153742 too. > > > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:212 > > + IntPoint adjustedPaintOffset = roundedIntPoint(paintOffset + location() + LayoutPoint(0, getNumerator().offsetHeight() + gNumeratorGap * style().fontSize() + m_lineThickness / 2)); > > Why we use the numerator for the height but not for the width? The choice taken here is to draw a fraction bar that takes the whole fraction width and it seems more natural to me to rely on the renderer's width rather than on the numerator or denominator's widths. In general the three values are equal anyway, but they may be different if we have margin etc The use of the numerator height is just to calculate the offset to place the bar below it. Probably, the current expression is not really readable... In bug 155639, it is written m_ascent - getMathAxisHeight() which looks better to me.
Frédéric Wang (:fredw)
Comment 12 2016-04-07 06:24:10 PDT
Created attachment 275889 [details] Patch
Sergio Villar Senin
Comment 13 2016-04-07 08:44:43 PDT
Comment on attachment 275889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275889&action=review Patch seems good at first sight. I've a couple of questions. I guess the bevelled attribute is still not supported right? Why do we have differences in the rendering in the tests? I mean, I do see how anonymous children are removed, but why do we have those pixel differences? > LayoutTests/mathml/opentype/fraction-line-expected.html:13 > + <body> No need for <html> <head> and <body> tags since you're already adding <!DOCTYPE html> > LayoutTests/mathml/opentype/fraction-line.html:17 > + <body> Ditto. > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:68 > +LayoutUnit RenderMathMLBlock::getMathAxisHeight() const Let's remove the get, style guides force us not to include it. > Source/WebCore/rendering/mathml/RenderMathMLBlock.h:67 > + LayoutUnit getMathAxisHeight() const; Ditto. > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:42 > +// FIXME: The "MathML in HTML5" implementation note suggests the values 50% for "thin" and 200% for "thick". Is there any open bug for this issue? > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:68 > +} Looks a bit scary to directly compare an arbitrary sized user added string with some string in our code. Are we sure that equalIgnoringASCIICase() is safe? Also what if there is some invalid value, is the center alignment the specified fallback? > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:165 > + Extra line > Source/WebCore/rendering/mathml/RenderMathMLFraction.h:68 > + }; I think we are no longer using these k prefixes. Seem preferable to use a more descriptive/verbose enum names like FractionAlignmentCenter etc...
Frédéric Wang (:fredw)
Comment 14 2016-04-08 01:14:26 PDT
Comment on attachment 275889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275889&action=review Thanks for the review. I'll upload a new patch soon. > I guess the bevelled attribute is still not supported right? Indeed. (not sure we want to support it) > Why do we have differences in the rendering in the tests? I mean, I do see how anonymous children are removed, but why do we have those pixel differences? A general comment on this (that also applies to other patches): Alex initially tried to stay very close to the current flexbox-based rendering in order to leave tests unchanged and make review easier. Unfortunately, that duplicated many flexbox functions and logic into the MathML class and actually make review harder. So Martin asked to just do the complete refactoring and update the tests to match the new rendering. The MathML3 spec is very vague about the rendering so that should not be an issue ; the plan is now to follow the "MathML in HTML5" implementation note to get more reliable rendering. Now on this specific patch, the math axis and fraction rule thickness are now read from the MATH table instead of the calculating them with arbitrary heuristic so this may change some positions/sizes. But I'm sure there are other changes given how different the render tree and layout functions are when we remove flexbox layout and anonymous wrappers... >> LayoutTests/mathml/opentype/fraction-line-expected.html:13 >> + <body> > > No need for <html> <head> and <body> tags since you're already adding <!DOCTYPE html> I'd prefer to keep them for now for consistency with other tests and because I find the explicit structure clearer. >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:68 >> +LayoutUnit RenderMathMLBlock::getMathAxisHeight() const > > Let's remove the get, style guides force us not to include it. oops, I removed the all these get* from my github branch but forgot that function. Done and patches rebased now. >> Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:42 >> +// FIXME: The "MathML in HTML5" implementation note suggests the values 50% for "thin" and 200% for "thick". > > Is there any open bug for this issue? Yes, it's bug 155639 and this will be handled in the second phase of the refactoring. I'll update that comment. >> Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:68 >> +} > > Looks a bit scary to directly compare an arbitrary sized user added string with some string in our code. Are we sure that equalIgnoringASCIICase() is safe? > > Also what if there is some invalid value, is the center alignment the specified fallback? I'm not sure I understand that, but for consistency with the current linethickness parsing I'll use equalLettersIgnoringASCIICase (I'm not sure who wrote this parseAlignmentAttribute btw). Probably, the attribute parsing should be moved to the MathMLElement classes in the future, but let's keep it that way for now...
Frédéric Wang (:fredw)
Comment 15 2016-04-08 01:18:54 PDT
> Also what if there is some invalid value, is the center alignment the specified fallback? Forgot that one: IIRC, For invalid values the MathML spec says to ignore them and use the default value (which is center here)
Frédéric Wang (:fredw)
Comment 16 2016-04-08 03:59:59 PDT
Created attachment 275991 [details] Patch
Sergio Villar Senin
Comment 17 2016-04-11 01:29:20 PDT
Comment on attachment 275991 [details] Patch r=me, provided EWS is happy with the changes
Frédéric Wang (:fredw)
Comment 18 2016-04-11 05:27:25 PDT
Created attachment 276140 [details] Patch final version
Build Bot
Comment 19 2016-04-11 06:20:13 PDT
Comment on attachment 276140 [details] Patch final version Attachment 276140 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1136418 New failing tests: mathml/presentation/roots.xhtml mathml/presentation/fractions-linethickness.html mathml/opentype/fraction-line.html
Build Bot
Comment 20 2016-04-11 06:20:15 PDT
Created attachment 276145 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Frédéric Wang (:fredw)
Comment 21 2016-04-11 07:07:13 PDT
Created attachment 276147 [details] Patch final version Rebasing tests for iOS simulator...
Frédéric Wang (:fredw)
Comment 22 2016-04-11 08:38:51 PDT
Committed r199295: <http://trac.webkit.org/changeset/199295>
Frédéric Wang (:fredw)
Comment 23 2016-04-18 07:44:55 PDT
Committed r199665: <http://trac.webkit.org/changeset/199665>
Manuel Rego Casasnovas
Comment 24 2016-04-18 12:32:46 PDT
(In reply to comment #23) > Committed r199665: <http://trac.webkit.org/changeset/199665> It seems there was an issue in the ChangeLog, as this change is related to bug #153918 and not this one. Just notifying about it for future reference.
Frédéric Wang (:fredw)
Comment 25 2016-07-18 06:21:22 PDT
It's https://trac.webkit.org/changeset/199295
Note You need to log in before you can comment on or make changes to this bug.