Bug 170272 – Add a test to ensure <mo> paints its leading and trailing space as specified in its attributes.

• Component: MathML
• Status: RESOLVED FIXED

[Minsheng Liu]

Created attachment 305839 [details]
An arrow operator with unnecessary left space

I note that there is a large, observable space before an arrow character inside a <mo> element, as in the attached document (basically $X \to Y$). More clearly, one can inspect the <mo> element in the Inspector to find the extra space to the left of an arrow. I am not sure if this is the result of ruling the <mo> as a prefix or postfix, but according to the specification for MathML 3, an operator in the middle of a <mrow> should be treated as an infix, given that all other lookup information is absent (https://www.w3.org/TR/MathML/chapter3.html#presm.formdefval).

The issue is tested against both Safari and Safari Technology Preview. I have installed STIX 2, but since Safari uses the old STIX fonts, this seems not a font-specific issue. I also tested the same code in Firefox, and the spacing is normal.

I have tested it against multiple arrow characters, from normal "RIGHTWARDS ARROW" to long right arrow to the $\mapsto$ arrow. The issue is of importance as people type morphisms everywhere ("Let $\phi: A \to B$ be ...").

[Minsheng Liu]

I checked the Operator Dictionary, in which arrows are indeed treated as infix operators. I have no idea where the issue could be, then.

Is there any easy way to get started with WebKit’s MathML code base? I have some experience with C++ in general and tried to comprehend some part of WebKit but failed.

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

Hi,

You can play with lspace, rspace and form attributes on the <mo> element.

Indeed, in Epiphany the default spacing seems incorrect, although I have not checked the operator dictionary.

I'd suggest to compare

https://trac.webkit.org/browser/trunk/Source/WebCore/mathml/MathMLOperatorDictionary.cpp

and

https://www.w3.org/TR/MathML3/appendixc.html

to see if there is an error. If not, you'll have to debug the parsing in

https://trac.webkit.org/browser/trunk/Source/WebCore/mathml/MathMLOperatorElement.cpp

to see what's happening.

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

OK checking the dictionary, the default spacing is actually correct (lspace = rspace = 5). If you don't want the space, you can use <mo lspace="0em" rspace="0em">.

The issue seems to be how the stretched operator is drawn. Compare <mo stretchy="true> VS <mo stretchy="true>. In that case, you'll have to look into WebCore/rendering/mathml/ and more precisely RenderMathMLOperator.cpp and MathOperator.cpp

[Minsheng Liu]

I believe I have found the issue, but since I am not familiar with the code, I am not sure what is the right way to fix it. In RenderMathMLOpeartor.cpp, line 290 - 311:

> void RenderMathMLOperator::paint(PaintInfo& info, const LayoutPoint& paintOffset)
> {
>     RenderMathMLToken::paint(info, paintOffset);
>     if (!useMathOperator())
>         return;
> 
>     LayoutPoint operatorTopLeft = paintOffset + location();
>     operatorTopLeft.move(style().isLeftToRightDirection() ? leadingSpace() : trailingSpace(), 0);
> 
>     // Center horizontal operators.
>     if (!isVertical())
>         operatorTopLeft.move(-(m_mathOperator.width() - width()) / 2, 0);
>
>     m_mathOperator.paint(style(), info, operatorTopLeft);
> }

For the un-stretchy operator, useMathOperator() returns false and the code ends at the fifth line. The paintOffset is { x = 512, y = 512 } (which depends on the specific HTML I am using). Later in RenderBlock::paint(), it will be adjusted to { x = 796, y = 512 }, where 796 - 512 = 284 is the leading space, I believe (there is some space around the operator).

However, for the stretchy operator, the code continues. location() returns nothing. leadingSpace() gives 284. m_mathOperator.width() gives 790, width() gives 1538. Note that (1538 - 790) / 2 = 284. In other words, by adding the leadingSpace() the operator is already centered. Centering horizontal operators again actually moves the operator towards right by 284. I believe the “center horizontal operators” part is unnecessary, or should be grounded by something like

> auto contentWidth = width() - leadingSpace - trailingSpace();
> operatorTopLeft.move(-(m_mathOperator.width() - contentWidth) / 2, 0);

I am not sure, however, this gives the correct semantics, as I am truly unfamiliar with the code.

Moreover, it seems that the vertical positioning for stretchy operator is also wrong. I will keep inspecting the code.

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

(In reply to Minsheng Liu from comment #4)
> > auto contentWidth = width() - leadingSpace - trailingSpace();
> > operatorTopLeft.move(-(m_mathOperator.width() - contentWidth) / 2, 0);

Right, I think this makes more sense than the current code (although I don't exactly remember what is the reference point for strechy operator painting). At least, this will be compatible with the current code when lspace=rspace=0 which is the case for accents (see e.g. https://bug-72828-attachments.webkit.org/attachment.cgi?id=229368).

BTW, we have various tests in LayoutTests/mathml/ ; I think you should check whether they won't be broken by your changes (or will need a small rebaseline) and add a new one for the present use case.

[Alejandro G. Castro]

(In reply to Minsheng Liu from comment #4)
> I believe I have found the issue, but since I am not familiar with the code,
> I am not sure what is the right way to fix it. In RenderMathMLOpeartor.cpp,
> line 290 - 311:
> 
> > void RenderMathMLOperator::paint(PaintInfo& info, const LayoutPoint& paintOffset)
> > {
> >     RenderMathMLToken::paint(info, paintOffset);
> >     if (!useMathOperator())
> >         return;
> > 
> >     LayoutPoint operatorTopLeft = paintOffset + location();
> >     operatorTopLeft.move(style().isLeftToRightDirection() ? leadingSpace() : trailingSpace(), 0);
> > 
> >     // Center horizontal operators.
> >     if (!isVertical())
> >         operatorTopLeft.move(-(m_mathOperator.width() - width()) / 2, 0);
> >

I also wonder why are we doing this part of the layout in the paint method, probably we should try to move to the layout if we don't find any real limitation. This could be also the problem, as you found leadingSpace is a calculation already centered and we are centering again in the paint. Maybe just try to move that to the layout and see if that is already done. As Fred said run the tests because there are a lot of cases to consider in this code.

[Minsheng Liu]

Thanks for the feedback. I will look at it when I have more time later this week. 

However, I do have some question regarding the tests, as I am completely new to WebKit hacking. Hours ago, I run a WebKit test on my modified build. While there were many issues, none of them were related with MathML. I expect at least some diffs in MathML tests. Are MathML tests in LayoutTests/ automated or manual? What is the right way to see whether I have broken existing *MathML* code or not?

(In reply to Alejandro G. Castro from comment #6)
> (In reply to Minsheng Liu from comment #4)
> > I believe I have found the issue, but since I am not familiar with the code,
> > I am not sure what is the right way to fix it. In RenderMathMLOpeartor.cpp,
> > line 290 - 311:
> > 
> > > void RenderMathMLOperator::paint(PaintInfo& info, const LayoutPoint& paintOffset)
> > > {
> > >     RenderMathMLToken::paint(info, paintOffset);
> > >     if (!useMathOperator())
> > >         return;
> > > 
> > >     LayoutPoint operatorTopLeft = paintOffset + location();
> > >     operatorTopLeft.move(style().isLeftToRightDirection() ? leadingSpace() : trailingSpace(), 0);
> > > 
> > >     // Center horizontal operators.
> > >     if (!isVertical())
> > >         operatorTopLeft.move(-(m_mathOperator.width() - width()) / 2, 0);
> > >
> 
> I also wonder why are we doing this part of the layout in the paint method,
> probably we should try to move to the layout if we don't find any real
> limitation. This could be also the problem, as you found leadingSpace is a
> calculation already centered and we are centering again in the paint. Maybe
> just try to move that to the layout and see if that is already done. As Fred
> said run the tests because there are a lot of cases to consider in this code.

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

(In reply to Alejandro G. Castro from comment #6)
> I also wonder why are we doing this part of the layout in the paint method,
> probably we should try to move to the layout if we don't find any real
> limitation. This could be also the problem, as you found leadingSpace is a
> calculation already centered and we are centering again in the paint. Maybe
> just try to move that to the layout and see if that is already done.

IIRC, the layout part calculates the preferred width and width (left space + right space + content width). However, for stretchy operators we paint them ourselves, so we have to do some special positioning of glyphs, mirroring (RTL) etc. It seems the reference point for the painting is the center of the operator (which probably makes thing easier to applies a scale transform in RTL). Probably things can be simplified as many things are inherited for the old implementations, but I don't remember exactly.

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

(In reply to Minsheng Liu from comment #7)
> Thanks for the feedback. I will look at it when I have more time later this
> week. 
> 
> However, I do have some question regarding the tests, as I am completely new
> to WebKit hacking. Hours ago, I run a WebKit test on my modified build.
> While there were many issues, none of them were related with MathML. I
> expect at least some diffs in MathML tests. Are MathML tests in LayoutTests/
> automated or manual? What is the right way to see whether I have broken
> existing *MathML* code or not?

On which platform do you work?

Tests are executed automatically with the run-webkit-tests (you can restrict execution to Layout/mathml/). Two things to note:

1) It is possible that our test coverage is limited and hence does not detect your changes, that's why we always add new tests when we fix a bug.

2) There are some tests (pixel tests) which compare against an expected render tree (txt file) and visual rendering (PNG image). However, if your code changes visual rendering but to the render tree, the test runner will still consider the test to pass. That's very likely to be the case if you only modify RenderMathMLToken::paint here. That's why it's often better to use reftests or javascript tests.

[Minsheng Liu]

I am sorry that I have been inactive for the last several months. Now that I finally get some free time, I plan to re-investigate the matter. All past MathML layout tests are passed, and I am about to insert a new one. As for Alejandro’s advice, I am learning the rendering process of WebKit. Hopefully this bug could be resolved soon.

[Minsheng Liu]

In fact, I think the centering process should be removed entirely. In RenderMathMLOperator::layoutBlock, we have

    if (useMathOperator()) {
        for (auto child = firstChildBox(); child; child = child->nextSiblingBox())
            child->layoutIfNeeded();
        setLogicalWidth(leadingSpaceValue + m_mathOperator.width() + trailingSpaceValue);
        setLogicalHeight(m_mathOperator.ascent() + m_mathOperator.descent());
    } else {
        // irrelevant, as the buggy code only concerns cases where `useMathOperator() = true`.
    }

So, logicalWidth, which is just width when we are in horizontal writing mode, is just the sum of operator width, leading space, and trailing space. Hence, it makes no sense to do centering. It is already centered!

    // Center horizontal operators.
    if (!isVertical())
        operatorTopLeft.move(-(m_mathOperator.width() - width()) / 2, 0);

I will just remove the code, which does not lead to any regression.

[Minsheng Liu]

Forget my last post. I did not run the pixel level tests. Sorry but I am very unfamiliar with the WebKit infrastructure.

[Minsheng Liu]

Created attachment 326734 [details]
a proposed patch

Truly stretched operator requires the special centering code I proposed to delete. The reason is that the stretchTo function does not update the element’s width (but does change that of m_mathOperator). My proposed patch changes that, and everything works fine.

Regarding to pixel test regression, I got six failed, but five of them failed without changing any code, so I am not sure if expected images need to be updated. The only case *I think* that is relevant is mathml/opentype/opentype-stretchy-horizontal. My patch passed the pixel test but has different text output (render tree). Nonetheless, I compared patched image output with unmatched output with my eyes and I think the patch gives the correct behavior.

I need the following help:
1. I am unsure about how to create a pixel test. I create one, and run the pixel test. It seems that WebKit automatically included it into the test case. Is that all I need to do? How about other platforms where expected images have not been created?

2. How to fix the five failed pixel tests. I am pretty sure that my code does not change the behavior of WebCore (I compared the result with the unmatched build). Do those tests’ expected images need to be updated?

3. What else do I need to do/be careful about before we submit the patch?

[Minsheng Liu]

Created attachment 326737 [details]
comparison on opentype-stretchy-horizontal case

Here is a comparison for the opentype-stretchy-horizontal case. With the patch the output are aligned, which according to the test file, should be the correct behavior.

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

Comment on attachment 326734 [details]
a proposed patch

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

This looks good to me and correspond to what Alejandro suggested. But I guess we need more testing.

> b/Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:306
> +    

You should not do this whitespace change.

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

(In reply to Minsheng Liu from comment #13)
> Created attachment 326734 [details]
> a proposed patch
> 
> Truly stretched operator requires the special centering code I proposed to
> delete. The reason is that the stretchTo function does not update the
> element’s width (but does change that of m_mathOperator). My proposed patch
> changes that, and everything works fine.
> 
> Regarding to pixel test regression, I got six failed, but five of them
> failed without changing any code, so I am not sure if expected images need
> to be updated. The only case *I think* that is relevant is
> mathml/opentype/opentype-stretchy-horizontal. My patch passed the pixel test
> but has different text output (render tree). Nonetheless, I compared patched
> image output with unmatched output with my eyes and I think the patch gives
> the correct behavior.
> 
> I need the following help:
> 1. I am unsure about how to create a pixel test. I create one, and run the
> pixel test. It seems that WebKit automatically included it into the test
> case. Is that all I need to do? How about other platforms where expected
> images have not been created?
> 
> 2. How to fix the five failed pixel tests. I am pretty sure that my code
> does not change the behavior of WebCore (I compared the result with the
> unmatched build). Do those tests’ expected images need to be updated?
> 
> 3. What else do I need to do/be careful about before we submit the patch?

@Minsheng Liu: Thanks a lot for working on this.

You did not reply, but are you developing on Linux (e.g. WebKitGTK), macOS, or something else? Sometimes the pixel tests require some specific fonts to be installed. Sometimes they have not been updated and thus just marked "failing". I guess you can ignore existing failures, though.

opentype-stretchy-horizontal is supposed to test various ways of stretching operators (base size, variants and assembly). The spacing is irrelevant here and the screenshot you gave still looks good to me. You can check the help of run-webkit-tests to know how re-generate the test expectations for that test (text and png).

You can submit your patch to EWS testing. This will build it on various platforms and run tests on macOS & iOS (with some results and test expectations). You can also take a look at https://mdn.mozillademos.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test$samples/MathML_Torture_Test to see if you don't see anything abnormal or if you patch improves things.

I think we need to add at least one new test for this bug (i.e. that fails before your patch and passes after). The easiest way is to create a pixel test (again the help fo run-webkit-tests explains how to generate test expectations). However, I would prefer javascript tests or reftests, if possible as they are more reliable and easier to debug later. Here are some random ideas:

* A javascript tests that measures sizes & position of some boxes with getBoundingClientRect() to check that the operator is correctly sized/placed. See for example LayoutTests/mathml/presentation/stretchy-depth-height.html for an example.

* A reftest with a rectangle that hides the location where you expect the operator to be. See LayoutTests/mathml/presentation/mroot-transform.html for an example.

* A mismatch reftest for <mo lspace="0px" rspace="50px">→</mo> VS <mo lspace="50px" rspace="0px">→</mo>.

Note: In general try to avoid text in your tests as that may give inaccurate sizes. Use mspace/mpadded with pixel lengths instead. If needed, use the "Ahem" font or some special WOFF fonts to guarantee the expected size.

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

(In reply to Frédéric Wang (:fredw) from comment #16)
> You can also take a look at
> https://mdn.mozillademos.org/en-US/docs/Mozilla/MathML_Project/
> MathML_Torture_Test$samples/MathML_Torture_Test to see if you don't see
> anything abnormal or if you patch improves things.

Look at tests 19 and 22. Your patch seriously changes the position of horizontal braces. I guess the real issue is that the munderover render class uses the unstretch sizes to position its children and I suspect this "centering" hack is supposed to workaround that. However, probably the positions of children should really be updated after the stretch.

[Minsheng Liu]

Aha thanks! I did play around with the Torture Test but did not look at it carefully. The reason I included the OpenType stretchy horizontal case is that a similar issue was found there with <mover>. I will look at it, and the JavaScript test you just mentioned.

[Minsheng Liu]

Here comes the bad news. If we want to move layout code out of paint(), some non-trivial change is required.

For the cases you have mentioned, the DOM involves more than one layer of <mover>. It looks like:
\mover{x + ... + x}{\mover{horizontal brace}{k times}}
The current logic layouts the structure as follows:
1. layout {x + ... + x}.
2. layout {\mover{horizontal brace}{k times}}. (Note the width is incorrect!}
3. stretch the horizontal brace without updating its location.

And fixing everything in operator’s paint().

Despite being ad-hoc—it is unclear to me whether the original fix in paint() is or can be made correct or not—one consequence of this algorithm is that the width of the stretched part of a <mover> element is its original content width. I just checked myself that MathML 3 does not specify the width in such scenario (width is not mentioned at all in the section regarding <munder>/<mover>), but this felt just wrong and would make Inspector very hard to use in inspecting the bounds of those elements. I checked that in Firefox, the width of \mover{horizontal brace}{k times} is the same as that of {x + ... + x}.

So,
1. I think we should change the current behavior and use the more natural width, which involve a non-trivial change to the layout algorithm.
2. Since you are more familiar with MathML standard, does the width of a stretched operator affect its parents? If the standard is unclear in this area, should we send something to the committee?

— Minsheng

[Minsheng Liu]

Created attachment 326859 [details]
fix the algorithm of layout <mover>/<munder> when stretchy operators are present

The fix is simple: I force the layout of all blocks containing stretchy operators. The cost is not that high, as children of those blocks do not need layout and hence all the extra computation is to recalculate the location. Moreover, Inspector now gives the correct bounds of those elements.

I have tested the code manually against the torture test and it does not break the original bug, the arrow centering issue.

Could you look at my changes and check if I satisfy the coding guidelines? I am not very familiar with C++ and its OOP capabilities, and while I have read the coding style guidelines, I might forget some. But I think the reviewer should handle that anyway.

---

About tests. I work on macOS High Sierra. I would like to continue my work but I have to sleep right now. Hopefully I could finish tests before the Friday. I am a third year undergraduate (major in not CS but math) so the course load is not light.

[Build Bot]

Attachment 326859 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:39:  Streams are highly discouraged.  [readability/streams] [3]
ERROR: Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 1 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Minsheng Liu]

Created attachment 326860 [details]
same patch, ignore the <iostream>

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

(In reply to Minsheng Liu from comment #19)
> So,
> 1. I think we should change the current behavior and use the more natural
> width, which involve a non-trivial change to the layout algorithm.
> 2. Since you are more familiar with MathML standard, does the width of a
> stretched operator affect its parents? If the standard is unclear in this
> area, should we send something to the committee?

I believe it indeed *does* affect the width of its parent (otherwise you would paint stretchy operators out of the bounds of munderover) but you can ask confirmation on https://lists.w3.org/Archives/Public/www-math/

BTW, the unofficial draft http://mathml-association.org/MathMLinHTML5/ gives more details in general regarding layout rules.

(In reply to Minsheng Liu from comment #20)
> Created attachment 326859 [details]
> fix the algorithm of layout <mover>/<munder> when stretchy operators are
> present

Thanks for continuing the effort on this, it's very helpful.     

So this patch fixes your initial test case, right? What about the centering, do we still need it? I actually wonder whether it was because the actual size of the stretched operator does not necessarily match the target size (so it would still be more beautiful to do a centering... but ignoring lspace/rspace to avoid this bug)?

I forgot to mention that Tools/Script/check-webkit-style will do the style check of EWS, so you can run it locally. Also, you can use "Tools/Scripts/prepare-ChangeLog" to format a patch for review ("Tools/Scripts/webkit-patch upload" will do that). I guess it is explained in details in https://webkit.org/contributing-code/ 

Checking the code, it seems that RenderMathMLUnderOver::computeOperatorsHorizontalStretch is actually only called in RenderMathMLUnderOver::layoutBlock, just before some calls to layoutIfNeeded() on the munderover children... but I believe these layoutIfNeeded calls are useless as we already do the layout in computeOperatorsHorizontalStretch. Also, computeOperatorsHorizontalStretch really does operator stretching and child layout, so the name is not good (I guess this was inherited from old implementation). So I propose you open a preliminary cleanup bug to rename "computeOperatorsHorizontalStretch" to "stretchHorizontalOperatorsAndLayoutChildren" and remove the useless layoutIfNeeded calls, so that things are a bit clearer. That won't require new tests and you would be able to experience the contribution process.

I quickly try your patch on the torture test and your change makes WebKit crash on an ASSERT for me:

ASSERTION FAILED: !needsLayout()
/Users/fred/WebKit/Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp(479) : virtual std::optional<int> WebCore::RenderMathMLScripts::firstLineBaseline() const
1   0x10eeed24d WTFCrash
2   0x11639fbac WebCore::RenderMathMLScripts::firstLineBaseline() const
3   0x11638b0d8 WebCore::RenderMathMLBlock::ascentForChild(WebCore::RenderBox const&)
4   0x11639b909 WebCore::RenderMathMLRow::firstLineBaseline() const
5   0x11638b0d8 WebCore::RenderMathMLBlock::ascentForChild(WebCore::RenderBox const&)
6   0x11639b909 WebCore::RenderMathMLRow::firstLineBaseline() const
7   0x115ff77a4 WebCore::RenderBlock::firstLineBaseline() const
8   0x116019045 WebCore::RenderBlockFlow::firstLineBaseline() const
9   0x11629a8e6 WebCore::RenderTableCell::cellBaselinePosition() const
10  0x11629ab1b WebCore::RenderTableCell::layout()
11  0x1162b8dcd WebCore::RenderTableRow::layout()
12  0x115f8cdfc WebCore::RenderElement::layoutIfNeeded()
13  0x1162bd020 WebCore::RenderTableSection::layout()
14  0x115f8cdfc WebCore::RenderElement::layoutIfNeeded()
15  0x1162913ba WebCore::RenderTable::layout()

Are you building WebKit in debug mode?  (see https://webkit.org/building-webkit/).

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

(In reply to Frédéric Wang (:fredw) from comment #23)
> I quickly try your patch on the torture test and your change makes WebKit
> crash on an ASSERT for me:
> 
> ASSERTION FAILED: !needsLayout()

OK, I stand corrected. It's already reported in bug 174131

[Minsheng Liu]

> So this patch fixes your initial test case, right? What about the centering, do we still need it? I actually wonder whether it was because the actual size of the stretched operator does not necessarily match the target size (so it would still be more beautiful to do a centering... but ignoring lspace/rspace to avoid this bug)?

The updated computeOperatorHorizontalStretch(), plus the fix in stretchTo() that updates the logical width, should match the actual size of the stretched operator with the target size, rendering centering unnecessary. I think our general aim should to match these two sizes as they give better bounds in the Inspector. Therefore, removing the center code once and for all should be fine, and if we find any further issue on those, we could fix them independently.

By the way, I think the patch, if accepted, would not get into mainstream Safari until next September, so we have plenty of time to catch them…I am unsure about other platforms. 😂


> So I propose you open a preliminary cleanup bug to rename "computeOperatorsHorizontalStretch" to "stretchHorizontalOperatorsAndLayoutChildren" and remove the useless layoutIfNeeded calls, so that things are a bit clearer. That won't require new tests and you would be able to experience the contribution process.

Ok, my plan will be handle them in three steps:

1. Cleanup layout code in RenderMathMLUnderOver.cpp.
2. Rework stretchHorizontalOperatorsAndLayoutChildren() so that it gives correct bounds. This one requires some test using getBoundingClientRect().
3. Remove the center code and closes this issue.

By the way, is there any other environment where an operator might be stretched? If so, I could look at them manually.

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

(In reply to Minsheng Liu from comment #25)
> > So this patch fixes your initial test case, right? What about the centering, do we still need it? I actually wonder whether it was because the actual size of the stretched operator does not necessarily match the target size (so it would still be more beautiful to do a centering... but ignoring lspace/rspace to avoid this bug)?
> 
> The updated computeOperatorHorizontalStretch(), plus the fix in stretchTo()
> that updates the logical width, should match the actual size of the
> stretched operator with the target size, rendering centering unnecessary. I
> think our general aim should to match these two sizes as they give better
> bounds in the Inspector. Therefore, removing the center code once and for
> all should be fine, and if we find any further issue on those, we could fix
> them independently.

So my point is that we have non-streching elements providing a target size, then we stretch the operator to be *at least* that target size. We use the base size, size variants or glyph assembly but we can not necessarily match the target size. I think the logical width should match the stretched size, though.

> 
> By the way, I think the patch, if accepted, would not get into mainstream
> Safari until next September, so we have plenty of time to catch them…I am
> unsure about other platforms. 😂

Well, I think we can't say anything per https://trac.webkit.org/wiki/FAQ#WillfeatureXXXbeincludedinthenextreleaseofSafari

> Ok, my plan will be handle them in three steps:
> 
> 1. Cleanup layout code in RenderMathMLUnderOver.cpp.
> 2. Rework stretchHorizontalOperatorsAndLayoutChildren() so that it gives
> correct bounds. This one requires some test using getBoundingClientRect().
> 3. Remove the center code and closes this issue.

Sounds good to me, thanks.

> By the way, is there any other environment where an operator might be
> stretched? If so, I could look at them manually.

Yes, for example vertical operators in <mrow>. Search for "stretchTo" in rendering/mathml

[Minsheng Liu]

> I think the logical width should match the stretched size, though. 
Oh good! I mistook your target size for logical size. 

> Well, I think we can't say anything per https://trac.webkit.org/wiki/FAQ#WillfeatureXXXbeincludedinthenextreleaseofSafari
Hahahahahahahaha. True.

[Minsheng Liu]

@fred As you might have noticed, I have deleted the “centering” code in the paint() method in the other patch (179682), so the bug is gone. I plan to add a pixel test for this issue, how do you think?

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

(In reply to Minsheng Liu from comment #28)
> @fred As you might have noticed, I have deleted the “centering” code in the
> paint() method in the other patch (179682), so the bug is gone. I plan to
> add a pixel test for this issue, how do you think?

Please finish https://bugs.webkit.org/show_bug.cgi?id=179682#c76 first ;-)

As you might have noticed, pixel tests are easy to write but a pain to deal with, so I'd prefer you write javascript tests or reftests instead. See comment 16 for suggestions.

[Minsheng Liu]

(In reply to Frédéric Wang (:fredw) from comment #29)
> (In reply to Minsheng Liu from comment #28)
> > @fred As you might have noticed, I have deleted the “centering” code in the
> > paint() method in the other patch (179682), so the bug is gone. I plan to
> > add a pixel test for this issue, how do you think?
> 
> Please finish https://bugs.webkit.org/show_bug.cgi?id=179682#c76 first ;-)
> 
> As you might have noticed, pixel tests are easy to write but a pain to deal
> with, so I'd prefer you write javascript tests or reftests instead. See
> comment 16 for suggestions.

I doubt that any getBoundingRect() could work as the fault is on paint(). The render tree before and after the other patch for simple operator case should be the same. I will give reftests a try after my final today is dealt with.

[Minsheng Liu]

Created attachment 330229 [details]
Patch

[Daniel Bates]

Comment on attachment 330229 [details]
Patch

Thanks for the test, Minsheng!

Very minor, we should add a new line at the end of file mo-arrow-painted-center.html and it would be good to add <html>/</html> tags to both files.

[Minsheng Liu]

(In reply to Daniel Bates from comment #32)
> Comment on attachment 330229 [details]
> Patch
> 
> Thanks for the test, Minsheng!
> 
> Very minor, we should add a new line at the end of file
> mo-arrow-painted-center.html and it would be good to add <html>/</html> tags
> to both files.

You are absolutely right. Just a few minutes!

[Minsheng Liu]

Created attachment 330272 [details]
Patch

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

Thanks for finishing the work on this Minsheng. Some quick comments:

- "<mo> is painted correctly" is vague. Can you provide more details in the bug summary and description? If I remember correctly the issue was that operator spacing was incorrect for horizontally stretched operators, because some centering was performed before? And also maybe because the logical width was incorrect too?

- Is <mo lspace="100px" rspace="100px"> a good case? It seems that it won't be affected by the old "centering" behavior? I guess you want to check a case where lspace != rspace e.g. <mo lspace="100px" and rspace="200px">?

- While we are here, maybe test the RTL case too with <math dir="rtl">?

- I would put some visible <mspace> elements before and after the <mo> operator, so that one can really see the operator spacing. Actually, you could use such <mspace> elements to indicate the reference 100px/200px widths.

- red/green is generally used to suggest failure/success, so the reference widths could have more neutral colors. See http://web-platform-tests.org/writing-tests/rendering.html

[Minsheng Liu]

Sorry that I cannot get to you sooner.

> - "<mo> is painted correctly" is vague.

Agreed.

> - Is <mo lspace="100px" rspace="100px"> a good case? It seems that it won't
> be affected by the old "centering" behavior?

Actually it is fine. The old behavior is add a center offset (lspace plus rspace divided by 2) to the existing lspace. The effect of that code to the test case I propose would be having a left space of 200px and right space of 0px. Though I do agree with you that we could add three different cases, where

left = right
left < right
left > right

> - While we are here, maybe test the RTL case too with <math dir="rtl">?

And double the three to six.

> - I would put some visible <mspace> elements before and after the <mo>
> operator, so that one can really see the operator spacing. Actually, you
> could use such <mspace> elements to indicate the reference 100px/200px
> widths.

My intention was to have them aligned so that a vertical comparison is possible. But you are right that the operator spacing would be hard to see in this case. I might change it to have a combination of both.

> - red/green is generally used to suggest failure/success, so the reference
> widths could have more neutral colors. See
> http://web-platform-tests.org/writing-tests/rendering.html

I was not aware of that : )


(In reply to Frédéric Wang (:fredw) [back 03/01/2018] from comment #35)
> Thanks for finishing the work on this Minsheng. Some quick comments:
> 
> - "<mo> is painted correctly" is vague. Can you provide more details in the
> bug summary and description? If I remember correctly the issue was that
> operator spacing was incorrect for horizontally stretched operators, because
> some centering was performed before? And also maybe because the logical
> width was incorrect too?
> 
> - Is <mo lspace="100px" rspace="100px"> a good case? It seems that it won't
> be affected by the old "centering" behavior? I guess you want to check a
> case where lspace != rspace e.g. <mo lspace="100px" and rspace="200px">?
> 
> - While we are here, maybe test the RTL case too with <math dir="rtl">?
> 
> - I would put some visible <mspace> elements before and after the <mo>
> operator, so that one can really see the operator spacing. Actually, you
> could use such <mspace> elements to indicate the reference 100px/200px
> widths.
> 
> - red/green is generally used to suggest failure/success, so the reference
> widths could have more neutral colors. See
> http://web-platform-tests.org/writing-tests/rendering.html

[Minsheng Liu]

Created attachment 330392 [details]
Patch

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

Comment on attachment 330392 [details]
Patch

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

> LayoutTests/mathml/presentation/mo-paint-lspace-rspace-expected.html:19
> +    <p>The test passes if the arrow has a left space of 50px, which is as wide as the black block to the left,

150px

> LayoutTests/mathml/presentation/mo-paint-lspace-rspace-expected.html:34
> +      <<mspace width="200px" height="10px" depth="10px" mathbackground="black"></mspace>

extra <

> LayoutTests/mathml/presentation/mo-paint-lspace-rspace-expected.html:43
> +

right/left are inverted in RTL mode.

> LayoutTests/mathml/presentation/mo-paint-lspace-rspace-expected.html:52
> +    <p>The test passes if the arrow has a left space of 50px, which is as wide as the black block to the left,

150px

> LayoutTests/mathml/presentation/mo-paint-lspace-rspace.html:17
> +    <p>The test passes if the arrow has a left space of 50px, which is as wide as the black block to the left,

150px

> LayoutTests/mathml/presentation/mo-paint-lspace-rspace.html:30
> +      <<mspace width="200px" height="10px" depth="10px" mathbackground="black"></mspace>

extra <

> LayoutTests/mathml/presentation/mo-paint-lspace-rspace.html:36
> +      and a right space of 200px, which is as wide as the black block to the right.</p>

right/left should be inverted in RTL mode

(note that MathML3 defines lspace as "leading space" and rspace as "trailing space")

> LayoutTests/mathml/presentation/mo-paint-lspace-rspace.html:44
> +    <p>The test passes if the arrow has a left space of 50px, which is as wide as the black block to the left,

150px

[Minsheng Liu]

Created attachment 330429 [details]
Patch

[Minsheng Liu]

That is indeed a lot of typos.

I have rephrased the language to using leading/trailing space, but keep the "black block to the left/right" (adjusted for RTL and LTR) to make it easier for human readers. I also added two headings.

(In reply to Frédéric Wang (:fredw) [back 03/01/2018] from comment #38)
> Comment on attachment 330392 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330392&action=review
> 
> > LayoutTests/mathml/presentation/mo-paint-lspace-rspace-expected.html:19
> > +    <p>The test passes if the arrow has a left space of 50px, which is as wide as the black block to the left,
> 
> 150px
> 
> > LayoutTests/mathml/presentation/mo-paint-lspace-rspace-expected.html:34
> > +      <<mspace width="200px" height="10px" depth="10px" mathbackground="black"></mspace>
> 
> extra <
> 
> > LayoutTests/mathml/presentation/mo-paint-lspace-rspace-expected.html:43
> > +
> 
> right/left are inverted in RTL mode.
> 
> > LayoutTests/mathml/presentation/mo-paint-lspace-rspace-expected.html:52
> > +    <p>The test passes if the arrow has a left space of 50px, which is as wide as the black block to the left,
> 
> 150px
> 
> > LayoutTests/mathml/presentation/mo-paint-lspace-rspace.html:17
> > +    <p>The test passes if the arrow has a left space of 50px, which is as wide as the black block to the left,
> 
> 150px
> 
> > LayoutTests/mathml/presentation/mo-paint-lspace-rspace.html:30
> > +      <<mspace width="200px" height="10px" depth="10px" mathbackground="black"></mspace>
> 
> extra <
> 
> > LayoutTests/mathml/presentation/mo-paint-lspace-rspace.html:36
> > +      and a right space of 200px, which is as wide as the black block to the right.</p>
> 
> right/left should be inverted in RTL mode
> 
> (note that MathML3 defines lspace as "leading space" and rspace as "trailing
> space")
> 
> > LayoutTests/mathml/presentation/mo-paint-lspace-rspace.html:44
> > +    <p>The test passes if the arrow has a left space of 50px, which is as wide as the black block to the left,
> 
> 150px

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

Comment on attachment 330429 [details]
Patch

LGTM, Thanks!

[WebKit Commit Bot]

Comment on attachment 330429 [details]
Patch

Clearing flags on attachment: 330429

Committed r226403: <https://trac.webkit.org/changeset/226403>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36293853>