MathML: check LayoutObject tree for comment-split <mo> when layout [chromium/src : main]
THANK FLY (Gerrit)
THANK FLY added 4 comments![]( "Open in Gerrit")
THANK FLYSo the relevant spec text is here:
https://w3c.github.io/mathml-core/#layout-of-operators
> If the content of the <mo> element is not made of a single character c then fall back to the layout algorithm of 3.2.1.1 Layout of <mtext>.https://w3c.github.io/mathml-core/#layout-of-mtext
> If the <mtext> element contains only text content without forced line break or soft wrap opportunity then, the anonymous child node generated for that text is laid out as defined in the relevant CSS specification andI don't understand what "floating" means here? The mo in the test case has multiple character and a line breaking, so indeed it should not be laid out using the MathML-specific code per the above text. But I don't see any floating elements there (and I think we handle float property as per https://w3c.github.io/mathml-core/#css-styling).
Also, it seems you are excluding cases when there are multiple in-flow text nodes but I don't see anything for that in the spec. Does the parenthesis still stretch in the following case with two text nodes in the DOM?
```
<math><mrow><mo id="mo2"></mo><mfrac><mfrac><mn>1</mn><mn>2</mn></mfrac><mfrac><mn>3</mn><mn>4</mn></mfrac></mfrac></mrow></math>
<script>
mo2.appendChild(document.createTextNode("("));
mo2.appendChild(document.createTextNode("")); // empty
</script>
```
Sorry for the delay in getting back to this issue.
Regarding the float issue you mentioned, it was caused by other HTML content I included in previous tests. It can actually be ignored. Sorry for the confusion.
I have adjusted the fix approach. Currently, we ensure only one layout node exists when entering the MathTokenLayoutAlgorithm by checking that there is exactly one Layout object. This guarantees correct rendering and prevents crashes.
I’ve also tested the test case you sent with the empty node. There should effectively be only one valid layout node in practice, so it will stretch normally now.
Fix(mathml): Fix text node detection logic to properly handle floating
elements.THANK FLY```suggestion
[mathml] Text node detection logic to properly handle floating elements.
```Maybe that fits on one line?
Hi I adjusted it now. Sorry I didn’t get a chance to handle this issue earlier. I have revised it recently. Could you please review it again?
After the fix: renders "!→" without dcheckTHANK FLYThis suggests that this is fixing a correctness issue. So how about a correctness test, and not just a crash test?
Now add a mathml_operator_element_test.cc for correctness test.
if (!node.IsBlock() || !node.IsMathML() || !node.FirstChild().IsInline())THANK FLYThis is a rather useless check, given that the parameter is `BlockNode`. Maybe replace with `!node.IsBlockFlow()` here, instead of the dynamic LayoutBlockFlow cast below.
I have adjusted the fix approach. Currently, we ensure only one layout node exists when entering the MathTokenLayoutAlgorithm by checking that there is exactly one Layout object. This guarantees correct rendering and prevents crashes.
Related details
- Frédéric Wang Nélar
- Morten Stenshorne
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
THANK FLY (Gerrit)
THANK FLY added 1 comment![]( "Open in Gerrit")
if (!block_flow || !block_flow->ChildrenInline()) {THANK FLYWon't this be covered by the above `node.FirstChild().IsInline()` check?
I have adjusted fix.
Related details
- Frédéric Wang Nélar
- Morten Stenshorne
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 6 comments![]( "Open in Gerrit")
Need help from the MathML folks, but I'm fairly convinced that this fix is wrong.
Why should:
```
<math>
<mo style="width:1px;">a <!-- -->b</mo>
</math>
```
render differently from:
```
<math>
<mo style="width:1px;">a b</mo>
</math>
```
'b' is visible in the first example, but not in the second.
After the fix: renders "!→" without dcheckTHANK FLYThis suggests that this is fixing a correctness issue. So how about a correctness test, and not just a crash test?
Now add a mathml_operator_element_test.cc for correctness test.
Why not a web test? Is there no visual correctness issue here?
const auto& token_content = element->GetTokenContent();Avoid auto, when the type isn't obvious, to make it more readable.
auto* layout_object = element->GetLayoutObject();You're assuming it's a LayoutBox further below. Should move the cast here.
ASSERT_TRUE(first_child);What about these children? Should they be examined? Why is it important to check that there are at least two children?
Related details
- Frédéric Wang Nélar
- Philip Rogers
- THANK FLY
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
THANK FLY (Gerrit)
THANK FLY added 1 comment![]( "Open in Gerrit")
After the fix: renders "!→" without dcheckTHANK FLYThis suggests that this is fixing a correctness issue. So how about a correctness test, and not just a crash test?
Morten StenshorneNow add a mathml_operator_element_test.cc for correctness test.
Why not a web test? Is there no visual correctness issue here?
Thanks firstly. May I ask where the correct test should be placed? I'm actually not familiar with this test. third_party/blink/web_tests/external/wpt/mathml/crashtests/comment-and-entity-in-mo.html can be visually verified. Is this enough?
Related details
- Frédéric Wang Nélar
- Morten Stenshorne
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
THANK FLY (Gerrit)
THANK FLY added 1 comment![]( "Open in Gerrit")
Need help from the MathML folks, but I'm fairly convinced that this fix is wrong.
Why should:
```
<math>
<mo style="width:1px;">a <!-- -->b</mo>
</math>
```render differently from:
```
<math>
<mo style="width:1px;">a b</mo>
</math>
```'b' is visible in the first example, but not in the second.
Indeed, only a appears in the second example. This is because, in the logic I modified, the first case has two layout objects split by a comment, so it uses normal text layout and renders both text items.
For the second case, I believe there is only one layout object, so it goes through MathTokenLayoutAlgorithm. This algorithm likely only supports a single character, so the trailing b does not get laid out or displayed.
I'm not even sure how this case should be fixed at this point.I think the issue is probably with the MathTokenLayoutAlgorithm; it might not be able to enforce constraints properly.
Morten Stenshorne (Gerrit)
Morten Stenshorne added 2 comments![]( "Open in Gerrit")
THANK FLYNeed help from the MathML folks, but I'm fairly convinced that this fix is wrong.
Why should:
```
<math>
<mo style="width:1px;">a <!-- -->b</mo>
</math>
```render differently from:
```
<math>
<mo style="width:1px;">a b</mo>
</math>
```'b' is visible in the first example, but not in the second.
Indeed, only a appears in the second example. This is because, in the logic I modified, the first case has two layout objects split by a comment, so it uses normal text layout and renders both text items.
For the second case, I believe there is only one layout object, so it goes through MathTokenLayoutAlgorithm. This algorithm likely only supports a single character, so the trailing b does not get laid out or displayed.I'm not even sure how this case should be fixed at this point.I think the issue is probably with the MathTokenLayoutAlgorithm; it might not be able to enforce constraints properly.
My point is that an HTML comment should typically never affect rendering. But now it does.
After the fix: renders "!→" without dcheckTHANK FLYThis suggests that this is fixing a correctness issue. So how about a correctness test, and not just a crash test?
Morten StenshorneNow add a mathml_operator_element_test.cc for correctness test.
THANK FLYWhy not a web test? Is there no visual correctness issue here?
Thanks firstly. May I ask where the correct test should be placed? I'm actually not familiar with this test. third_party/blink/web_tests/external/wpt/mathml/crashtests/comment-and-entity-in-mo.html can be visually verified. Is this enough?
That's a crash test. We typically need a reftest or something using testharness, probably in third_party/blink/web_tests/external/wpt/mathml/ . Keeping the existing crash test is fine.
Documentation here: https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/testing/web_platform_tests.md
Related details
- Frédéric Wang Nélar
- Philip Rogers
- THANK FLY