WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
153917
Refactor RenderMathMLFraction layout to avoid using flexbox
https://bugs.webkit.org/show_bug.cgi?id=153917
Summary
Refactor RenderMathMLFraction layout to avoid using flexbox
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
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
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.
Top of Page
Format For Printing
XML
Clone This Bug