Skip to content

Commit

Permalink
Guard more MathML-specific code with the MathMLCoreEnabled flag
Browse files Browse the repository at this point in the history
After [1], MathMLElement instances are created even when MathMLCore is
disabled. Particular attention was paid to the (previously incorrect)
HasTagName(mathml_names::...) as well as to places where elements were
cast to MathMLElement types. This CL performs the remaining work for
callers of isMathMLElement(), checking possible behavior changes
when MathMLCore is disabled for now. In details, these places are:

1. Adjustment of display: math on MathML elements.
2. Adjustment of display: contents on MathML elements.
3. Application of custom style to MathML elements.
4. Sanitization of MathML elements via the sanitizer API.

For (1), there is already a guard in the CSS parser so there was no
behavior change (cf isDisplayInside). An ASSERT is added to guarantee
that.

For (2), we don't resolve display: contents to none on MathML elements
when MathMLCore is disabled, so they continue to behave the same as
HTML elements.

The code path for (3) performs dynamic casts to classes
derived from MathMLElement, relying on generated implementations of
AllowFrom(const Element&) that only check tag names. As a consequence
this leads to incorrect casts when MathMLCore is disabled, since all
MathML elements use the base MathMLElement class in that case.

Finally for (4), we keep the changed implied by [1] to the FromAPI()
function of sanitizer.cc i.e. the "math:" namespace designator is
always appended for nodes in the MathML namespace, whether or not
MathMLCore is enabled. Note that the Sanitizer API is not shipped
yet so it's fine to do this web-exposed change without API owner
approval.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/3506644
[2] https://source.chromium.org/search?q=mathml_element_type_helpers.h

Bug: 6606, 1272556, 1307667
Change-Id: Ib069944e55e102381192d90c13c676fc2cb0b51d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3536908
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Frédéric Wang <fwang@igalia.com>
Cr-Commit-Position: refs/heads/main@{#983716}
  • Loading branch information
fred-wang authored and Chromium LUCI CQ committed Mar 22, 2022
1 parent b9b60c8 commit 053d6bb
Show file tree
Hide file tree
Showing 7 changed files with 120 additions and 28 deletions.
10 changes: 8 additions & 2 deletions third_party/blink/renderer/core/css/resolver/style_adjuster.cc
Expand Up @@ -62,6 +62,7 @@
#include "third_party/blink/renderer/core/layout/layout_theme.h"
#include "third_party/blink/renderer/core/layout/list_marker.h"
#include "third_party/blink/renderer/core/layout/ng/inline/layout_ng_text_combine.h"
#include "third_party/blink/renderer/core/mathml/mathml_element.h"
#include "third_party/blink/renderer/core/style/computed_style.h"
#include "third_party/blink/renderer/core/style/computed_style_constants.h"
#include "third_party/blink/renderer/core/style/style_intrinsic_length.h"
Expand Down Expand Up @@ -783,6 +784,10 @@ void StyleAdjuster::AdjustComputedStyle(StyleResolverState& state,
}

auto* svg_element = DynamicTo<SVGElement>(element);

bool is_mathml_element = RuntimeEnabledFeatures::MathMLCoreEnabled() &&
IsA<MathMLElement>(element);

if (style.Display() != EDisplay::kNone) {
if (svg_element)
AdjustStyleForSvgElement(*svg_element, style);
Expand Down Expand Up @@ -819,8 +824,9 @@ void StyleAdjuster::AdjustComputedStyle(StyleResolverState& state,

// math display values on non-MathML elements compute to flow display
// values.
if ((!element || !element->IsMathMLElement()) &&
if ((!element || !is_mathml_element) &&
style.IsDisplayMathBox(style.Display())) {
DCHECK(RuntimeEnabledFeatures::MathMLCoreEnabled());
style.SetDisplay(style.Display() == EDisplay::kBlockMath
? EDisplay::kBlock
: EDisplay::kInline);
Expand Down Expand Up @@ -943,7 +949,7 @@ void StyleAdjuster::AdjustComputedStyle(StyleResolverState& state,
}
style.SetCssDominantBaseline(baseline);

} else if (element && element->IsMathMLElement()) {
} else if (is_mathml_element) {
if (style.Display() == EDisplay::kContents) {
// https://drafts.csswg.org/css-display/#unbox-mathml
style.SetDisplay(EDisplay::kNone);
Expand Down
Expand Up @@ -888,8 +888,10 @@ scoped_refptr<ComputedStyle> StyleResolver::ResolveStyle(
state.Style()->GetCurrentColor());
}

if (element->IsMathMLElement())
if (RuntimeEnabledFeatures::MathMLCoreEnabled() &&
IsA<MathMLElement>(element)) {
ApplyMathMLCustomStyleProperties(element, state);
}
} else if (IsHighlightPseudoElement(style_request.pseudo_id)) {
if (element->GetComputedStyle() &&
element->GetComputedStyle()->TextShadow() !=
Expand Down Expand Up @@ -1013,7 +1015,8 @@ void StyleResolver::InitStyleAndApplyInheritance(
void StyleResolver::ApplyMathMLCustomStyleProperties(
Element* element,
StyleResolverState& state) {
DCHECK(element && element->IsMathMLElement());
DCHECK(RuntimeEnabledFeatures::MathMLCoreEnabled() &&
IsA<MathMLElement>(element));
ComputedStyle& style = state.StyleRef();
if (auto* space = DynamicTo<MathMLSpaceElement>(*element)) {
space->AddMathBaselineIfNeeded(style, state.CssToLengthConversionData());
Expand Down
49 changes: 25 additions & 24 deletions third_party/blink/renderer/core/style/computed_style_test.cc
Expand Up @@ -1224,30 +1224,31 @@ TEST_F(ComputedStyleTest, ShouldApplyAnyContainment) {
ASSERT_TRUE(html);
ASSERT_TRUE(body);

auto display_types = {CSSValueID::kInline,
CSSValueID::kBlock,
CSSValueID::kListItem,
CSSValueID::kInlineBlock,
CSSValueID::kTable,
CSSValueID::kInlineTable,
CSSValueID::kTableRowGroup,
CSSValueID::kTableHeaderGroup,
CSSValueID::kTableFooterGroup,
CSSValueID::kTableRow,
CSSValueID::kTableColumnGroup,
CSSValueID::kTableColumn,
CSSValueID::kTableCell,
CSSValueID::kTableCaption,
CSSValueID::kWebkitBox,
CSSValueID::kWebkitInlineBox,
CSSValueID::kFlex,
CSSValueID::kInlineFlex,
CSSValueID::kGrid,
CSSValueID::kInlineGrid,
CSSValueID::kContents,
CSSValueID::kFlowRoot,
CSSValueID::kNone,
CSSValueID::kMath};
std::vector display_types = {CSSValueID::kInline,
CSSValueID::kBlock,
CSSValueID::kListItem,
CSSValueID::kInlineBlock,
CSSValueID::kTable,
CSSValueID::kInlineTable,
CSSValueID::kTableRowGroup,
CSSValueID::kTableHeaderGroup,
CSSValueID::kTableFooterGroup,
CSSValueID::kTableRow,
CSSValueID::kTableColumnGroup,
CSSValueID::kTableColumn,
CSSValueID::kTableCell,
CSSValueID::kTableCaption,
CSSValueID::kWebkitBox,
CSSValueID::kWebkitInlineBox,
CSSValueID::kFlex,
CSSValueID::kInlineFlex,
CSSValueID::kGrid,
CSSValueID::kInlineGrid,
CSSValueID::kContents,
CSSValueID::kFlowRoot,
CSSValueID::kNone};
if (RuntimeEnabledFeatures::MathMLCoreEnabled())
display_types.push_back(CSSValueID::kMath);

for (auto contain :
{CSSValueID::kNone, CSSValueID::kLayout, CSSValueID::kPaint,
Expand Down
@@ -0,0 +1,23 @@
<!DOCTYPE html>
<html>
<head>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
</head>
<body>
<math id="math" style="display: math"></math>
<math id="contents" style="display: contents"></math>
<script>
function getDisplayOf(id) {
const style = window.getComputedStyle(document.getElementById(id));
return style.getPropertyValue("display");
}
test(function() {
assert_equals(getDisplayOf("math"), "inline");
}, "Computation of 'display: math' on MathML elements when MathML Core is disabled.");
test(function() {
assert_equals(getDisplayOf("contents"), "contents");
}, "Computation of 'display: contents' on MathML elements when MathML Core is disabled.");
</script>
</body>
</html>
@@ -0,0 +1,3 @@
MathML elements should not cause any crash with MathMLCore disabled.


@@ -0,0 +1,42 @@
<!DOCTYPE html>
<html>
<head>
<script>
if (window.testRunner)
testRunner.dumpAsText();
</script>
</head>
<body>
<p>MathML elements should not cause any crash with MathMLCore disabled.</p>
<math><annotation></annotation></math>
<math><annotation-xml></annotation-xml></math>
<math><maction></maction></math>
<math><merror></merror></math>
<math><mfrac></mfrac></math>
<math><mi></mi></math>
<math><mmultiscripts></mmultiscripts></math>
<math><mn></mn></math>
<math><mo></mo></math>
<math><mover></mover></math>
<math><mpadded></mpadded></math>
<math><mphantom></mphantom></math>
<math><mprescripts></mprescripts></math>
<math><mroot></mroot></math>
<math><mrow></mrow></math>
<math><ms></ms></math>
<math><mspace/></math>
<math><msqrt></msqrt></math>
<math><mstyle></mstyle></math>
<math><msub></msub></math>
<math><msubsup></msubsup></math>
<math><msup></msup></math>
<math><mtable></mtable></math>
<math><mtd></mtd></math>
<math><mtext></mtext></math>
<math><mtr></mtr></math>
<math><munder></munder></math>
<math><munderover></munderover></math>
<math><none/></math>
<math><semantics></semantics></math>
</body>
</html>
@@ -0,0 +1,14 @@
<!DOCTYPE html>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script>
const source = `<math><mtext>Hello</mtext></math>`;
test(function() {
const sanitizer = new Sanitizer({allowElements: ["math:math", "math:mtext"]});
assert_equals(sanitizer.sanitizeFor("template", source).innerHTML, source);
}, `The 'math:' namespace designator allows to match MathML elements.`);
test(function() {
const sanitizer = new Sanitizer({allowElements: ["math", "mtext"]});
assert_equals(sanitizer.sanitizeFor("template", source).innerHTML, "Hello");
}, `The 'math:' namespace designator is required to match MathML elements.`);
</script>

0 comments on commit 053d6bb

Please sign in to comment.