Page MenuHomePhabricator

Bug 1916988 - Support CSS width/height properties on MathML elements. r=emilio
ClosedPublic

Authored by fredw on Sep 9 2024, 9:43 AM.
Referenced Files
F26118096: D221436.1749646784.diff
Tue, Jun 10, 12:59 PM
Unknown Object (File)
Thu, May 29, 5:41 PM
Unknown Object (File)
Mon, May 19, 3:08 PM
Unknown Object (File)
Mon, May 19, 7:51 AM
Unknown Object (File)
Sat, May 17, 12:14 AM
Unknown Object (File)
Fri, May 16, 6:30 AM
Unknown Object (File)
Wed, May 14, 6:49 AM
Unknown Object (File)
May 9 2025, 9:50 AM

Details

Summary

This patch implements support for the width/height properties on
MathML elements [1]. The general algorithm from the spec is as
follows:

(1) The outcome of the math layout is a "math content box".
(2) The content box sets its size from computed width/height values. If

auto, it's the one of the "math content box". This patch ignores
percentage values for now [2] [3].

(3) math content box is shifted so that its inline-start and top edges

aligns with the ones of the content box. There are exceptions
elements like mfrac and munder/mover/munderover which instead
horizontally center the math content box within the content box.
For baseline adjustment, we follow what Chromium does, see [4].

(4) Padding+border are added around the content box. Note that we

ignore the box-sizing property for now [5].

The patch essentially tweaks the various MathML layout algorithms to
perform steps (3) and (4) before the calls to
GetBorderPaddingForPlace and InflateReflowAndBoundingMetrics.

[1] https://w3c.github.io/mathml-core/#layout-algorithms
[2] https://github.com/w3c/mathml-core/issues/76
[3] https://github.com/w3c/mathml-core/issues/77
[4] https://github.com/w3c/mathml-core/issues/259
[5] https://github.com/w3c/mathml-core/issues/257

Below is more information about test coverage:

  • width-height-001: Verify that width, height, inline-size and block-size properties sets the size of the content box. This test used to verify they are ignored, this patch fixes the tag. It also adds a test for the case the specified size is smaller than the content (we force non empty descendants to make sure this content is large enough) and to verify the width is used for the preferred width.
  • width-height-002, width-height-003: These are reftests visually checking offsets of the math content box within a larger content box (specified by width/height) for the mtext, mrow, mpadded, mfrac, msqrt, mroot, in LTR/RTL modes. In particular they allow to verify some painted elements like fraction bar and radical symbols.
  • width-height-004: This test more directly checks that the math content box is horizontally centered within a larger content box for munder, mover, munderover and mfrac. This patch extends the test to cover the case when the math content box is wider (i.e. overflowing outside the content box) and removes unnecessary specified height.
  • width-height-005: New test for other layout algorithm that don't center the math content box, checking inline-start edges of children when a width is specified. We check both LTR/RTL modes and wider/narrower content boxes.
  • width-height-006: Same but checking the top edges for larger/smaller height and verifying that baseline is perserved.

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

fredw planned changes to this revision.Sep 9 2024, 9:43 AM
fredw created this revision.
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 9 2024, 9:43 AM
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.

Code analysis found 1 defect in diff 914356:

  • 1 defect found by clang-format
WARNING: Found 1 defect (warning level) that can be dismissed.

You can run this analysis locally with:

  • ./mach clang-format -p layout/mathml/nsMathMLmrootFrame.cpp

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).


If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 914356.

1 defect closed compared to the previous diff 914611.

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).


If you see a problem in this automated review, please report it here.

fredw updated this revision to Diff 915260.
fredw edited the summary of this revision. (Show Details)
fredw edited the summary of this revision. (Show Details)
fredw updated this revision to Diff 915906.
fredw retitled this revision from WIP: Bug 1916988 - Support CSS width/height properties on MathML elements. r=emilio to Bug 1916988 - Support CSS width/height properties on MathML elements. r=emilio.
fredw edited the summary of this revision. (Show Details)
fredw added a reviewer: emilio.

Code analysis found 1 defect in diff 915906:

  • 1 defect found by clang-format
WARNING: Found 1 defect (warning level) that can be dismissed.

You can run this analysis locally with:

  • ./mach clang-format -p layout/mathml/nsMathMLmrootFrame.cpp

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).


If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 915906.

emilio requested changes to this revision.Sep 18 2024, 3:21 PM

It seems this needs at least some vertical writing-mode tests? We should not be mixing physical and logical sizes.

layout/mathml/nsMathMLContainerFrame.cpp
151

A comment here would be useful (something like "we don't need to adjust block size for intrinsic sizing because intrinsic sizing works in the inline axis" or something like that?)

170

Same here, it seems you're mixing logical and physical sizes.

186

This feels wrong, shouldn't you set the ReflowOutput's BSize? Or alternatively use the physical width / height instead?

layout/mathml/nsMathMLContainerFrame.h
256

I think we generally use mozilla::Maybe which has release assertions rather than UB if you misuse them.

This revision now requires changes to proceed.Sep 18 2024, 3:21 PM
fredw requested review of this revision.Sep 19 2024, 8:46 AM
fredw updated this revision to Diff 916610.
fredw marked 3 inline comments as done.

It seems this needs at least some vertical writing-mode tests? We should not be mixing physical and logical sizes.

Yes, sorry for the confusion...

Vertical math was initially discussed/considered in https://github.com/w3c/mathml-core/issues/175 but for now the UA sheet just forces horizontal-tb on all MathML elements:

https://w3c.github.io/mathml-core/#user-agent-stylesheet
https://searchfox.org/mozilla-central/rev/56fd3b43187bca036141c384b809e61d51a447de/layout/mathml/mathml.css#13

In case we eventually support this, MathML Core spec keeps using the logical terms and that corresponds to Chromium's LayoutNG implementation too.

Short story: I started to use the logical terms in the patch and then realized that our current code uses physical terms and that it does not matter because we are always horizontal-tb.

Anyway, I updated the patch to use physical terms everywhere.

layout/mathml/nsMathMLContainerFrame.cpp
151

Do you still think it's needed with the new patch?

layout/mathml/nsMathMLContainerFrame.h
256

OK I saw it in other places but I thought it was only when it matches optional param in DOM IDL .

1 defect closed compared to the previous diff 915906.


If you see a problem in this automated review, please report it here.

This revision is now accepted and ready to land.Sep 29 2024, 12:02 AM

The commit message should say box-sizing rather than box-size I believe.

fredw edited the summary of this revision. (Show Details)