Fix category calculation for complex-typed expressions [chromium/src : main]
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 2 comments![]( "Open in Gerrit")
// If we didn't have percent hint context, we still might end up with:You ended up *not* adding this context, right? Adjust the comment?
// (1% * 1%) / 1px -> category should be kLength. So just add the power ofPlease add unit tests for this, if we don't have them already. E.g.:
```
1% / 1px;
(1% * 1%) / 1px;
(1% * 1% * 1%) / 1px;
(1% * 1deg) / 1px; (Nothing will parse this, but is it possible to express with CSSMathType using its API?)
```
Related details
- Daniil Sakhapov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
2 comments
// If we didn't have percent hint context, we still might end up with:You ended up *not* adding this context, right? Adjust the comment?
Done
// (1% * 1%) / 1px -> category should be kLength. So just add the power ofPlease add unit tests for this, if we don't have them already. E.g.:
```
1% / 1px;
(1% * 1%) / 1px;
(1% * 1% * 1%) / 1px;
(1% * 1deg) / 1px; (Nothing will parse this, but is it possible to express with CSSMathType using its API?)
```
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
if (percentage_hint_) {I guess dividing by `1px` does _not_ result in a percentage hint, then? (Please remind me where percentage hints come from in the first place.)
Related details
- Daniil Sakhapov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
2 comments
I've updated the test to correct values.
But I found the bug for cases where we have percents inside complex-typed arithmetic exression, that's not realted to this CL, so I suggest keep expectations for that test case and I come up with a fix in following CL.
if (percentage_hint_) {I guess dividing by `1px` does _not_ result in a percentage hint, then? (Please remind me where percentage hints come from in the first place.)
Percent hint comes from the context an expression will be used in. E.g. for width that would be Length.
The idea is to know what to do with percentes in typed arithmetic.
`/ 1px` doesn't give you enough info, as the whole expression can be `1% * 1% / 1px / 1deg * 1deg / 1deg * 1px` and used in Angle context later.
For us the problem is we do type checking at parse time, but for e.g. CSSOM we don't know the context beforhand, hence this percent trick is used to deduce percent hint by looking at the final type info (for given example we end up with `[percent -> 2], [angle -> -1], ([length -> 0])`), so we can check that we only have one non-percent entry and emulate `applying percent hint` Angle here. But if we saw `1% / 1px` and applied percent hint Legnth, that would cause kill the whole expression.
That percent hint helps us to know the final category of expression, so later the call side checks result category of an expression with what it expects and accepts/not accepts it.
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 3 comments![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
3 comments
// Since we don't have percent hint context, we might end up with:
// (1% * 1%) / 1px -> category should be kLength.
// So just add the power of percent as if we deduced percent hint from the
// expression itself (note that correctness will be checked at the call
// side - e.g. that category is Length if the call side expects Length).```suggestion
DCHECK(!percentage_hint_.has_value());
// Because we don't provide the percent hint from the "outside"
// as css-values expects [1], it's possible for the percentage
// hint to be unset even for expressions that involve percentages
// as well some other type, e.g.:
//
// width: (1% * 1%) / 1px;
//
// The CSSMathType for the above expression would contain
// [percent -> 2, length -> -1] with a null percent hint.
// However, the expression is clearly valid for 'width', since
// the percentages resolve against lengths. To address this,
// we effectively deduce what the percent hint should have been
// from the other (non-percent) base type powers:
//
// If there is more than one non-percent base type with non-zero power,
// it's intermediate (handled by early-out above).
//
// Otherwise, if there is exactly *one* other (non-percent) base type
// with a non-zero power, then we assume that percentages were intended
// to resolve against that base type, and basically move the powers from
// "percent" to the other base type. Continuing the example from before:
//
// [percent -> 2, length -> -1] => [percent -> 0, length -> 1]
//
// However, we do not actually need to modify `base_type_powers_`
// according to the above; the remainder of this function only
// checks `types_sum`, so it's enough to "pretend" that these numbers
// were combined all along.
//
// Note: This relies on kPercent appearing *last* in the enum.
//
// Note: Whether or not this deduced category is valid for
// the relevant context will be determined by the call
// site. For example, "width:(1% * 1%) / 1deg" would produce
// kAngle for this algorithm, but ultimately be rejected.
//
// [1] https://drafts.csswg.org/css-values-4/#determine-the-type-of-a-calculation
```
if (percentage_hint_) {Daniil SakhapovI guess dividing by `1px` does _not_ result in a percentage hint, then? (Please remind me where percentage hints come from in the first place.)
Percent hint comes from the context an expression will be used in. E.g. for width that would be Length.
The idea is to know what to do with percentes in typed arithmetic.
`/ 1px` doesn't give you enough info, as the whole expression can be `1% * 1% / 1px / 1deg * 1deg / 1deg * 1px` and used in Angle context later.
For us the problem is we do type checking at parse time, but for e.g. CSSOM we don't know the context beforhand, hence this percent trick is used to deduce percent hint by looking at the final type info (for given example we end up with `[percent -> 2], [angle -> -1], ([length -> 0])`), so we can check that we only have one non-percent entry and emulate `applying percent hint` Angle here. But if we saw `1% / 1px` and applied percent hint Legnth, that would cause kill the whole expression.
That percent hint helps us to know the final category of expression, so later the call side checks result category of an expression with what it expects and accepts/not accepts it.
Thanks, I think I finally understand everything. I suggested an improvement to the comment.
test_math_used("calc(10% / 1px)", "1px", {"prop": "line-height"});So the used value when literally specifying `line-height:0.1` under `font-size:10px` should serialize as `1px` and not `0.1`? Is that correct?
Related details
- Daniil Sakhapov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
I've updated the test to correct values.
But I found the bug for cases where we have percents inside complex-typed arithmetic exression, that's not realted to this CL, so I suggest keep expectations for that test case and I come up with a fix in following CL.
Done
// Since we don't have percent hint context, we might end up with:
// (1% * 1%) / 1px -> category should be kLength.
// So just add the power of percent as if we deduced percent hint from the
// expression itself (note that correctness will be checked at the call
// side - e.g. that category is Length if the call side expects Length).Done
test_math_used("calc(10% / 1px)", "1px", {"prop": "line-height"});So the used value when literally specifying `line-height:0.1` under `font-size:10px` should serialize as `1px` and not `0.1`? Is that correct?
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
// If line-height is set as number, it should be used as a multiplier to
// font-size.
if (primitive_value->IsNumber()) {
return Length::Percent(
ClampTo<float>(100.f * zoomed_length.NonNanCalculatedValue(
computed_font_size, {})));
}This looks like the wrong fix to me. I think you should not tackle this problem in this CL, and just land with an -expected.txt file.
Related details
- Daniil Sakhapov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
1 comment
// If line-height is set as number, it should be used as a multiplier to
// font-size.
if (primitive_value->IsNumber()) {
return Length::Percent(
ClampTo<float>(100.f * zoomed_length.NonNanCalculatedValue(
computed_font_size, {})));
}This looks like the wrong fix to me. I think you should not tackle this problem in this CL, and just land with an -expected.txt file.
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
1 comment
Also, this CL fixes line-height calculation in style builder converter.- Daniil Sakhapov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +2 |
1 comment
Also, this CL fixes line-height calculation in style builder converter.Daniil Sakhapovnit
ah, removed it in local commit message, but it didn't propagate to the CL description.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Fix category calculation for complex-typed expressions
Category can only be determined for a math node if a final sum of all
powers is either 0 (then it's a number), or 1 (then it's that 1 type).
But there has been a problem that powers weren't summed up in abs
values, leading to e.g. (1px / 1Hz) resulting in number, since the final
sum ended up being 0. So, this CL reworks category calculation code a
bit by checking that if we have more than one non-null type powers, the
category should be kCalcIntermediate.
But there is still a problem left with kPercent and percent_hint:
Currently, we don't supply percent hint context (info about what
percents in an expression would be resolved against, e.g. kCalcLength),
we rather check at a call side that category of a parsed expression is
the same as we expect. But we can end up in situation like: (1% * 1%) /
1px - which would have two powers (+2 for percent, -1 for length). To
solve this, this CL does a trick in which if we have only two non-null
type powers, once of which is percent, we simulate ApplyPercentHint to
the expression in Category() call. With this, we don't need to supply
the percent hint context info, but solve the problem of not knowing it.
Note also that this solves CSSOM problem, since there we don't know
percent hint context at all during parse time.
Also this CL adds printing for CSSMathType to ease future debugging.
- M third_party/blink/renderer/core/css/css_math_expression_node.cc
- M third_party/blink/renderer/core/css/css_math_expression_node.h
- A third_party/blink/web_tests/external/wpt/css/css-values/typed-arithmetic-different-categories-crash.html
- A third_party/blink/web_tests/external/wpt/css/css-values/typed_arithmetic-expected.txt
- M third_party/blink/web_tests/external/wpt/css/css-values/typed_arithmetic.html
Code-Review: +1 by Anders Hartvoll Ruud
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/55096.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/55096
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |