| Summary: | Refactor RenderMathMLFraction layout to avoid using flexbox | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alejandro G. Castro <alex> | ||||||||||||||||||||||
| Component: | MathML | Assignee: | 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
Alejandro G. Castro
2016-02-05 03:40:24 PST
Created attachment 270744 [details]
Patch
Alex: I guess should be made "depends on" bug 153742, or at least bug 153208? (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. 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. 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. Created attachment 275285 [details]
Patch
Created attachment 275398 [details]
Patch
(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! In terms of the accessibility changes (tests only, and a one-pixel change in size and position), LGTM. 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? (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. Created attachment 275889 [details]
Patch
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... 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... > 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)
Created attachment 275991 [details]
Patch
Comment on attachment 275991 [details]
Patch
r=me, provided EWS is happy with the changes
Created attachment 276140 [details]
Patch final version
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 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
Created attachment 276147 [details]
Patch final version
Rebasing tests for iOS simulator...
Committed r199295: <http://trac.webkit.org/changeset/199295> Committed r199665: <http://trac.webkit.org/changeset/199665> (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. |