Bug 153917 - Refactor RenderMathMLFraction layout to avoid using flexbox
Summary: Refactor RenderMathMLFraction layout to avoid using flexbox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 153742
Blocks: 158937 153918 153991 155639 155714 157943
  Show dependency treegraph
 
Reported: 2016-02-05 03:40 PST by Alejandro G. Castro
Modified: 2016-07-18 06:21 PDT (History)
11 users (show)

See Also:


Attachments
Patch (8.62 KB, patch)
2016-02-05 03:46 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (64.57 KB, patch)
2016-03-03 08:24 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (67.04 KB, patch)
2016-03-11 01:24 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (66.97 KB, patch)
2016-03-31 08:17 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (67.04 KB, patch)
2016-04-01 06:10 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (67.24 KB, patch)
2016-04-07 06:24 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (67.41 KB, patch)
2016-04-08 03:59 PDT, Frédéric Wang (:fredw)
svillar: review+
Details | Formatted Diff | Diff
Patch final version (67.36 KB, patch)
2016-04-11 05:27 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2016-02-05 03:40:24 PST
This is the next patch in the series to remove the dependency with flexbox.
Comment 1 Alejandro G. Castro 2016-02-05 03:46:57 PST
Created attachment 270744 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2016-02-05 06:13:57 PST
Alex: I guess should be made "depends on" bug 153742, or at least bug 153208?
Comment 3 Alejandro G. Castro 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.
Comment 4 Frédéric Wang (:fredw) 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.
Comment 5 Frédéric Wang (:fredw) 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.
Comment 6 Frédéric Wang (:fredw) 2016-03-31 08:17:52 PDT
Created attachment 275285 [details]
Patch
Comment 7 Frédéric Wang (:fredw) 2016-04-01 06:10:15 PDT
Created attachment 275398 [details]
Patch
Comment 8 Joanmarie Diggs (irc: joanie) 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!
Comment 9 Joanmarie Diggs (irc: joanie) 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.
Comment 10 Manuel Rego Casasnovas 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?
Comment 11 Frédéric Wang (:fredw) 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.
Comment 12 Frédéric Wang (:fredw) 2016-04-07 06:24:10 PDT
Created attachment 275889 [details]
Patch
Comment 13 Sergio Villar Senin 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...
Comment 14 Frédéric Wang (:fredw) 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...
Comment 15 Frédéric Wang (:fredw) 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)
Comment 16 Frédéric Wang (:fredw) 2016-04-08 03:59:59 PDT
Created attachment 275991 [details]
Patch
Comment 17 Sergio Villar Senin 2016-04-11 01:29:20 PDT
Comment on attachment 275991 [details]
Patch

r=me, provided EWS is happy with the changes
Comment 18 Frédéric Wang (:fredw) 2016-04-11 05:27:25 PDT
Created attachment 276140 [details]
Patch final version
Comment 19 Build Bot 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
Comment 20 Build Bot 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
Comment 21 Frédéric Wang (:fredw) 2016-04-11 07:07:13 PDT
Created attachment 276147 [details]
Patch final version

Rebasing tests for iOS simulator...
Comment 22 Frédéric Wang (:fredw) 2016-04-11 08:38:51 PDT
Committed r199295: <http://trac.webkit.org/changeset/199295>
Comment 23 Frédéric Wang (:fredw) 2016-04-18 07:44:55 PDT
Committed r199665: <http://trac.webkit.org/changeset/199665>
Comment 24 Manuel Rego Casasnovas 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.
Comment 25 Frédéric Wang (:fredw) 2016-07-18 06:21:22 PDT
It's https://trac.webkit.org/changeset/199295