Bug 181159 – Reduce code duplication between RenderMathMLRow, RenderMathMLEnclose, and RenderMathMLRoot

• Component: MathML
• Status: NEW

[Minsheng Liu]

I propose the following changes:

1. Discard margin/padding/border info set on <mrow>/<menclose>/<msqrt>/<mroot>.

The current implementation regarding those CSS properties are already inconsistent:
* https://bugs.webkit.org/show_bug.cgi?id=181157
* https://bugs.webkit.org/show_bug.cgi?id=181158

And the spec said those CSS properties that can affect layout can be omitted. So let's get rid of them.

2. Create a new virtual method spaceAroundContent() in RenderMathMLRow that behaves differently for RenderMathMLEnclose and RenderMathMLRoot.

We might need a different variant called computeSpaceAroundContent() for <mroot>, as index() (the n part of n-th root) must be layout-ed before the space can be computed. Once it is layout-ed, the method is constant.

3. Move the proposed contentRect() method to RenderMathMLRow (See https://bugs.webkit.org/show_bug.cgi?id=181037). It is helpful for both RenderMathMLEnclose and RenderMathMLRoot.

4. Combine the layoutBlock in the three classes into a generic one in RenderMathMLRow.

5. And change the paint() in RenderMathMLEnclose and RenderMathMLRoot accordingly.

The change is quite significant so I hope someone familiar with MathML code could discuss it with me before I work on it.

If approved, I will work on (1) first and consolidate some test cases. The actual refactoring will be done in a separate patch.

[Minsheng Liu]

Created attachment 330220 [details]
A preliminary patch

This is a preliminary patch illustrating my proposal. It combines code in

* RenderMathMLRow
* RenderMathMLMenclose
* RenderMathMLPadded
* RenderMathMLRoot

There is no regression, but we will need several new test cases.

I did not touch the issue of padding or border as I originally planned. We probably need Fred to come back to discuss that.

The refactoring process is not completed as many lines can still be improved. But that could wait for later patches.

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

I would need time to review and think about the whole patch, but I think we can already take some of these changes in separate preliminary bugs:

1) structures: make the getters const function and initialize constant values directly in the *.h. We should check if there are more instances than what is done in the patch. For example  e.g.

   struct SpaceAroundContent {
        LayoutUnit left = 0;
        LayoutUnit right = 0;
        LayoutUnit top = 0;
        LayoutUnit bottom = 0;
    };

3) Centering <math display="block">: I believe we should actually try to move it in the RenderMathMLMath class rather than keeping it in RenderMathMLRow.

3) Fixing width of <math display="block">: https://bugs.webkit.org/show_bug.cgi?id=160547 ; I think this is a serious bug and can already be fixed after bug 180348, so I'd propose to try it and to write tests before starting the bigger refactoring.

4) Removing margin/padding/border and update tests accordingly ( https://bugs.webkit.org/show_bug.cgi?id=181158 ). I personally think that ideally, we should support these CSS properties as they make sense for MathML boxes however:
  - The MathML spec is not clear
  - Gecko ignores them
  - WebKit already ignores them for most MathML elements.
  - It's not used by existing MathML content.
  So I would agree to remove them for now. My only concern would be that maybe some people would use negative margins to workaround the lack of negative spacing support (bug 120164 and bug 124830).

[Minsheng Liu]

160547 is almost done so I can work on this now.

1. I am pretty sure some of the change I proposed here will change the behavior of CSS, but they are not currently documented in LayoutTests. My question is, before we take the final action on removing margin/padding/border support in MathML (for now or forever), should we document those changes via tests?

2. A complain about getContentBoundingBox. The style of that function seems so C-ish. Is there a reason not to have something like the following?

struct ContentBoundingBox {
  LayoutUnit ascent, descent, width
};

ContentBoundingBox RenderMathMLRow::contentBoundingBox() const;

3. I think we can combine RenderMathMLRow/Math/Enclose/Root altogether. Code to center children can be done using the same sapceAroundContent virtual method. Hence a timeline:

* Start to move SpaceAroundContent to RenderMathMLRow and simplify initialization of that object. Introduce spaceAroundContent as a virtual method in RenderMathMLRow.
* Reduce code duplication between RenderMathMLRow and RenderMathMLMath.
* Work on RenderMathMLEnclose (and remove cache altogether).
* Work on RenderMathMLRoot (and remove cache altogether).

(In reply to Frédéric Wang (:fredw) from comment #2)
> I would need time to review and think about the whole patch, but I think we
> can already take some of these changes in separate preliminary bugs:
> 
> 1) structures: make the getters const function and initialize constant
> values directly in the *.h. We should check if there are more instances than
> what is done in the patch. For example  e.g.
> 
>    struct SpaceAroundContent {
>         LayoutUnit left = 0;
>         LayoutUnit right = 0;
>         LayoutUnit top = 0;
>         LayoutUnit bottom = 0;
>     };
> 
> 3) Centering <math display="block">: I believe we should actually try to
> move it in the RenderMathMLMath class rather than keeping it in
> RenderMathMLRow.
> 
> 3) Fixing width of <math display="block">:
> https://bugs.webkit.org/show_bug.cgi?id=160547 ; I think this is a serious
> bug and can already be fixed after bug 180348, so I'd propose to try it and
> to write tests before starting the bigger refactoring.
> 
> 4) Removing margin/padding/border and update tests accordingly (
> https://bugs.webkit.org/show_bug.cgi?id=181158 ). I personally think that
> ideally, we should support these CSS properties as they make sense for
> MathML boxes however:
>   - The MathML spec is not clear
>   - Gecko ignores them
>   - WebKit already ignores them for most MathML elements.
>   - It's not used by existing MathML content.
>   So I would agree to remove them for now. My only concern would be that
> maybe some people would use negative margins to workaround the lack of
> negative spacing support (bug 120164 and bug 124830).

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

Can you please try the 1) of comment 2 first? That should be fast and will allow to see the changes more clearly. I think you can do your proposal for ContentBoundingBox or any other similar getter for metrics/parameters in the same bug.

I would say we don't need tests for margin/padding/border since there is no standard behavior defined in MathML and actually we should ideally support CSS properties when they make sense.

The "combine RenderMathMLRow/Math/Enclose/Root altogether" seems a bit frightening. You mean doing one patch for all of them? Or merge them into one class? The latter does not sound a good idea. As for the former, it would nicer for review if you could do one patch for "stuff into RenderMathMLRow" and separate patches for each subclass, but let's first see how the patch looks like after the preliminary cleanup work is done.