Overflow of formulas is hidden for display mathematics

• Component: MathML
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

Created attachment 285314 [details]
Testcase

Jacques Distler reported the following issue: 'The "big fraction" of https://golem.ph.utexas.edu/wiki/instiki/show/Sandbox is summarily truncated'.

It seems that the overflow is hidden for MathML formula in display="block": no scroll bars appear which may explain that the formula looks "truncated". See the attached testcase comparing a simple formula in display and inline style.

[Frédéric Wang (:fredw)]

Created attachment 285315 [details]
Testcase

A better test case using a horizontal gradient from red to green. You should be able to scroll to the green part but that does not seem possible in nightly.

[Frédéric Wang (:fredw)]

Created attachment 285326 [details]
Experimental Patch

Some first thoughts: In general, because the preferred width of operators is overestimated (see https://bug-130326-attachments.webkit.org/attachment.cgi?id=283599). Hence all the MathML layoutBlock functions must set the correct logical width rather than relying on the preferred width. Then the RenderBox code use this logical width rather than filling the all available width.

There is an exception for <math display="block">: this must be rendered as a formula centered in the available space. Hence RenderMathMLRow::layoutBlock does not set the its logicalWidth to the width of the math content so that the centering done is this function is correct (http://tests.mathml-association.org/mathml/relations/html5-tree/display-1.html). I guess something in RenderBox also make it also occupy the whole available width. But when the available width is less than the width of the math content, the <math> tag is too narrow.

I'm attaching a hack that just injects the preferredLogicalWidth to force a minimal width but the ideal would be for RenderMathMLRow to expose the width of the math content and let RenderBox calculate the logical width by taking the maximum of the available width and of the width of the math content. Maybe rego or jfernandez have suggestions about how to do that.

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) [back 03/01/2018] from comment #2)
> I'm attaching a hack that just injects the preferredLogicalWidth to force a
> minimal width but the ideal would be for RenderMathMLRow to expose the width
> of the math content and let RenderBox calculate the logical width by taking
> the maximum of the available width and of the width of the math content.
> Maybe rego or jfernandez have suggestions about how to do that.

After bug 180348, I think it will be easier to get the actual width of the content of the <math display="block"> element using the new RenderMathMLRow::getContentBoundingBox function. This should give better result than using the preferred width or the logical width.

[Minsheng Liu]

Created attachment 332481 [details]
patch

[Minsheng Liu]

Oops, it should be "review?".

[Minsheng Liu]

I would like to close this before move layout code for <math> to RenderMathMLMath itself. As a result, this patch becomes surprisingly simple.

[Frédéric Wang (:fredw)]

Comment on attachment 332481 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332481&action=review

> LayoutTests/mathml/presentation/display-math-horizontal-overflow.html:14
> +        assert_equals(inline.clientWidth, block.clientWidth);

I think this test is a bit weak (it assumes inline math already have the correct width). I would instead try the 4 combinations between inline/block math and 100px/200px mspace content and check that the math has the expected width: 100px (for inline/100px), 200px (for block/100px) or 400px (for inline/400px and block/400px). You could even test centering inside the 200px container in the case block/100px, by checking the position of the mspace.

> Source/WebCore/ChangeLog:8
> +        Previously, <math> with display="block" uses its preferred width as logical width.

Is that true? I don't remember but I believe it takes the *container* width at the end? I think at some point it would be nice to check what RenderBox is doing and whether we can better handle MathML (what I had tried with the experimental patch). But I guess that would be more complex and I'm not sure I can review the RenderBox code.

> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:185
> +        if (width > logicalWidth())

I would do

if (width < logicalWidth) {
  // DO THE CENTERING
} else
   setLogicalWidth(width);

I think we can then just do centerBlockOffset = (logicalWidth() - width) / 2;

We also need to update the comment to explain the two cases.

[Minsheng Liu]

(In reply to Frédéric Wang (:fredw) from comment #7)
> Comment on attachment 332481 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332481&action=review
> 
> > LayoutTests/mathml/presentation/display-math-horizontal-overflow.html:14
> > +        assert_equals(inline.clientWidth, block.clientWidth);
> 
> I think this test is a bit weak (it assumes inline math already have the
> correct width). I would instead try the 4 combinations between inline/block
> math and 100px/200px mspace content and check that the math has the expected
> width: 100px (for inline/100px), 200px (for block/100px) or 400px (for
> inline/400px and block/400px). You could even test centering inside the
> 200px container in the case block/100px, by checking the position of the
> mspace.

I will do two tests, one for width when overflowed, and one for centering.

> 
> > Source/WebCore/ChangeLog:8
> > +        Previously, <math> with display="block" uses its preferred width as logical width.
> 
> Is that true? I don't remember but I believe it takes the *container* width
> at the end? I think at some point it would be nice to check what RenderBox
> is doing and whether we can better handle MathML (what I had tried with the
> experimental patch). But I guess that would be more complex and I'm not sure
> I can review the RenderBox code.

Oops! I got the concept wrong. I thought preferred width is just the container’s width. So, may I ask what is preferred width and when is used?

> 
> > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:185
> > +        if (width > logicalWidth())
> 
> I would do
> 
> if (width < logicalWidth) {
>   // DO THE CENTERING
> } else
>    setLogicalWidth(width);
> 
> I think we can then just do centerBlockOffset = (logicalWidth() - width) / 2;
> 
> We also need to update the comment to explain the two cases.

I think it is complicated that it is worthy having a layoutBlock() in RenderMathMLMath.

[Frédéric Wang (:fredw)]

(In reply to Minsheng Liu from comment #8)
> (In reply to Frédéric Wang (:fredw) from comment #7)
> > Comment on attachment 332481 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=332481&action=review
> > 
> > > LayoutTests/mathml/presentation/display-math-horizontal-overflow.html:14
> > > +        assert_equals(inline.clientWidth, block.clientWidth);
> > 
> > I think this test is a bit weak (it assumes inline math already have the
> > correct width). I would instead try the 4 combinations between inline/block
> > math and 100px/200px mspace content and check that the math has the expected
> > width: 100px (for inline/100px), 200px (for block/100px) or 400px (for
> > inline/400px and block/400px). You could even test centering inside the
> > 200px container in the case block/100px, by checking the position of the
> > mspace.
> 
> I will do two tests, one for width when overflowed, and one for centering.

Note that I actually meant testing the situation of a mspace of width 100px < 200px = container width and of width 400px > 200px = container width (you are only testing the latter). Also, there is already a reftest LayoutTests/mathml/presentation/attributes-display.html for centering, but it's good to test it using javascript too. 

I think it would be nicer if you could do all the metrics checks in one test because WPT tests are currently slow and some WebKit people complain about having too many tests.

> > > Source/WebCore/ChangeLog:8
> > > +        Previously, <math> with display="block" uses its preferred width as logical width.
> > 
> > Is that true? I don't remember but I believe it takes the *container* width
> > at the end? I think at some point it would be nice to check what RenderBox
> > is doing and whether we can better handle MathML (what I had tried with the
> > experimental patch). But I guess that would be more complex and I'm not sure
> > I can review the RenderBox code.
> 
> Oops! I got the concept wrong. I thought preferred width is just the
> container’s width. So, may I ask what is preferred width and when is used?

The min/max widths that would take the renderer if linebreaking happens or not. It is calculated before the layout in order to implement linebreaking (see computePreferredLogicalWidths functions). WebKit does not implement MathML linebreaking yet, so min preferred width == max preferred width is essentially the same as the logical width after layout modulo the approximations caused by stretchy operators (for which we only know the final width only after layout).

[Minsheng Liu]

Created attachment 332493 [details]
patch

Note that I use "overflow: auto" rather than "scroll". The latter will bring up the scroll bar unnecessarily in some situations.

[Frédéric Wang (:fredw)]

Comment on attachment 332493 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332493&action=review

LGTM, but please be sure to address the review comments. Thanks!

> LayoutTests/mathml/presentation/math-inline-display-width.html:25
> +        }, "dnline math, 100px content, 200px container");

s/dnline/display/

> LayoutTests/mathml/presentation/math-inline-display-width.html:43
> +        overflow: auto;

maybe you want "overflow: hidden" so that you don't have any potential problem with scroll bars.

> LayoutTests/mathml/presentation/math-inline-display-width.html:45
> +        line-height: 0;

why this line-height property?

> LayoutTests/mathml/presentation/math-inline-display-width.html:49
> +  <body onload="setTimeout(run, 2000)">

why do you need this 2 seconds delay?

> Source/WebCore/rendering/mathml/RenderMathMLMath.cpp:48
> +    LayoutUnit centerBlockOffset = std::max<LayoutUnit>(0, logicalWidth() - contentWidth) / 2;

As I said in previous review, I think this can just be centerBlockOffset = (logicalWidth() - contentWidth) / 2; since contentWidth < logicalWidth().

[Minsheng Liu]

Created attachment 332496 [details]
patch

[Minsheng Liu]

All resolved. line-height is set because inline math with line height = 1 will make div to have an extra 4px vertically. I have added a comment for that. 2 seconds delay is a leftover during my debug process and has been removed.

(In reply to Frédéric Wang (:fredw) from comment #11)
> Comment on attachment 332493 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332493&action=review
> 
> LGTM, but please be sure to address the review comments. Thanks!
> 
> > LayoutTests/mathml/presentation/math-inline-display-width.html:25
> > +        }, "dnline math, 100px content, 200px container");
> 
> s/dnline/display/
> 
> > LayoutTests/mathml/presentation/math-inline-display-width.html:43
> > +        overflow: auto;
> 
> maybe you want "overflow: hidden" so that you don't have any potential
> problem with scroll bars.
> 
> > LayoutTests/mathml/presentation/math-inline-display-width.html:45
> > +        line-height: 0;
> 
> why this line-height property?
> 
> > LayoutTests/mathml/presentation/math-inline-display-width.html:49
> > +  <body onload="setTimeout(run, 2000)">
> 
> why do you need this 2 seconds delay?
> 
> > Source/WebCore/rendering/mathml/RenderMathMLMath.cpp:48
> > +    LayoutUnit centerBlockOffset = std::max<LayoutUnit>(0, logicalWidth() - contentWidth) / 2;
> 
> As I said in previous review, I think this can just be centerBlockOffset =
> (logicalWidth() - contentWidth) / 2; since contentWidth < logicalWidth().

[EWS Watchlist]

Comment on attachment 332496 [details]
patch

Rejecting attachment 332496 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Minsheng Liu]

Guess I need another review+ flag…

(In reply to Build Bot from comment #14)
> Comment on attachment 332496 [details]
> patch
> 
> Rejecting attachment 332496 [details] from commit-queue.
> 
> [e-mail] does not have committer permissions according to
> https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/
> contributors.json.
> 
> - If you do not have committer rights please read
> http://webkit.org/coding/contributing.html for instructions on how to use
> bugzilla flags.
> 
> - If you have committer rights please correct the error in
> Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to
> the file (no review needed).  The commit-queue restarts itself every 2
> hours.  After restart the commit-queue will correctly respect your committer
> rights.

[WebKit Commit Bot]

Comment on attachment 332496 [details]
patch

Clearing flags on attachment: 332496

Committed r227722: <https://trac.webkit.org/changeset/227722>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36974748>