A percentage may be resolved against a negative value, which is allowed only in 'background-position' property. Currently in CSSCalcExpressionNodeParser::parseCalc, it creates CSSCalcExpressionNode tree result and does simplification for it. But during it, min(50%, 10%) is simplified to min(10%) and max(50%, 10%) is simplified to max(50%), which is the opposite result what should be done against negative basis. We should not do simplification for percentage comparison resolution against negative reference values.
Created attachment 446316 [details] Check layout test result
Created attachment 446343 [details] Patch
Comment on attachment 446343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446343&action=review Change looks good, but there are enough coding style mistakes that I think we should review an improved version. > Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:66 > + auto setAllowsNegativePercentageReferenceIfNeeded = [](RefPtr<CSSCalcExpressionNode>& expression) { This is using the wrong type. This should not take a RefPtr&, because that’s for an in/out argument. This is a "never null" expression node, so the argument type should just be CSSCalcExpressionNode&. > Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:67 > + if (expression->type() == CSSCalcExpressionNode::Type::CssCalcOperation) { Since we are using downcast below anyway, I recommend using is<> rather than writing this expression out, making the code easier to read: if (is<CSSCalcOperationNode>(*expression)) { > Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:70 > + auto& operationNode = downcast<CSSCalcOperationNode>(*expression); > + if (operationNode.isMinOrMaxNode()) > + operationNode.setAllowsNegativePercentageReference(); A nice way to write this is: if (auto& node = downcast<CSSCalcOperationNode>(*expression); node.isMinOrMaxNode()) node.setAllowsNegativePercentageReference(); The if statement with the semicolon keeps the local variable scope tight rather than letting it spill into the code below. The one-word local variable name is then good when the scope is so narrow; easy to see which node it is, and not having to write that out in the variable name keeps the code small and likely more readable. > Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:77 > + if (child->type() == CSSCalcExpressionNode::Type::CssCalcOperation) { > + auto& operationNode = downcast<CSSCalcOperationNode>(child.get()); > + if (operationNode.isMinOrMaxNode()) > + operationNode.setAllowsNegativePercentageReference(); > + } Same comments here, except also this code is repeated, so we should be using a lambda to not write it out twice. When you combine that with WebKit coding style things like early return, you get this: auto setAllowsNegativePercentageReferenceIfNeeded = [] (CSSCalcExpressionNode& expression) { auto set = [] (CSSCalcOperationNode& operation) { if (operation.isMinOrMaxNode()) operation.setAllowsNegativePercentageReference(); }; if (is<CSSCalcOperationNode>(expression)) { auto& operation = downcast<CSSCalcOperationNode>(expression); set(operation); for (auto& child : operation.children()) { if (is<CSSCalcOperationNode>(child)) set(downcast<CSSCalcOperationNode>(child)); } } }; But this code assumes there are no grandchildren. If that’s wrong, then it should be written with recursion instead: auto setAllowsNegativePercentageReferenceIfNeeded = [] (CSSCalcExpressionNode& expression) { if (is<CSSCalcOperationNode>(expression)) { auto& operation = downcast<CSSCalcOperationNode>(expression); if (operation.isMinOrMaxNode()) operation.setAllowsNegativePercentageReference(); for (auto& child : operation.children()) setAllowsNegativePercentageReferenceIfNeeded(child); } }; > Source/WebCore/css/calc/CSSCalcExpressionNodeParser.h:47 > + RefPtr<CSSCalcExpressionNode> parseCalc(CSSParserTokenRange, CSSValueID function, bool); The argument type bool does not make it clear what the argument is, so it needs a name, allowNegativePercentageReference, here. > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:838 > + if (unitType == CSSUnitType::CSS_PERCENTAGE && numberOfChildren > 1) > + return true; > + > + return false; No need to write this boolean expression so long. Just this: return unitType == CSSUnitType::CSS_PERCENTAGE && numberOfChildren > 1; Other tiny style thoughts: It can be tricky to use a lambda in a way that the code *easier* to read; the current version makes it a little too hard. And I find it a bit arbitrary to do the different checks in two if statements rather than three or just one. If you do still like the lambda to document things, then this might read better: auto involvesPercentageComparisons = [&] { return combinedUnitType == CSSUnitType::CSS_PERCENTAGE && m_children.size() > 1; }; if (isMinOrMaxNode() && allowsNegativePercentageReference() && involvesPercentageComparisons()) return; This should be just as efficient and I think makes it pretty readable. I believe the five lines are easier to read than the longer code block. Using capturing rather than arguments for combinedUnitType and m_children does seem to help rather than hurt.. > Source/WebCore/css/calc/CSSCalcOperationNode.h:149 > + unsigned m_allowsNegativePercentageReference : 1; This can just be a bool. There is no value to using a bitfield here, doesn't save memory and makes bigger code. Also if we are using. bool we can initialize here and don’t need to change the constructors. > Source/WebCore/css/parser/CSSPropertyParser.cpp:5562 > + auto position = consumePositionCoordinates(range, context.mode, unitless, PositionSyntax::BackgroundPosition, /* allowsNegativePercentageReference */ true); This is why we use enumerations in WebKit for this kind of idiom. We don’t use comments to make such argument clear, instead we used named enumeration values that you can understand without a comment. > Source/WebCore/css/parser/CSSPropertyParserHelpers.h:133 > +std::optional<PositionCoordinates> consumePositionCoordinates(CSSParserTokenRange&, CSSParserMode, UnitlessQuirk, PositionSyntax, bool = false); Needs a comment here, not at all obvious that this bool is allowsNegativePercentageReference.
Created attachment 446504 [details] Patch
Comment on attachment 446343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446343&action=review Thank you for your review, Darin. I applied all the comments you left in the previous patchset. Could you please review this patchset? >> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:66 >> + auto setAllowsNegativePercentageReferenceIfNeeded = [](RefPtr<CSSCalcExpressionNode>& expression) { > > This is using the wrong type. This should not take a RefPtr&, because that’s for an in/out argument. This is a "never null" expression node, so the argument type should just be CSSCalcExpressionNode&. Done. >> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:67 >> + if (expression->type() == CSSCalcExpressionNode::Type::CssCalcOperation) { > > Since we are using downcast below anyway, I recommend using is<> rather than writing this expression out, making the code easier to read: > > if (is<CSSCalcOperationNode>(*expression)) { Done. >> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:70 >> + operationNode.setAllowsNegativePercentageReference(); > > A nice way to write this is: > > if (auto& node = downcast<CSSCalcOperationNode>(*expression); node.isMinOrMaxNode()) > node.setAllowsNegativePercentageReference(); > > The if statement with the semicolon keeps the local variable scope tight rather than letting it spill into the code below. The one-word local variable name is then good when the scope is so narrow; easy to see which node it is, and not having to write that out in the variable name keeps the code small and likely more readable. Thank you for your advice for aspects in here. I will keep it in mind. >> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:77 >> + } > > Same comments here, except also this code is repeated, so we should be using a lambda to not write it out twice. When you combine that with WebKit coding style things like early return, you get this: > > auto setAllowsNegativePercentageReferenceIfNeeded = [] (CSSCalcExpressionNode& expression) { > auto set = [] (CSSCalcOperationNode& operation) { > if (operation.isMinOrMaxNode()) > operation.setAllowsNegativePercentageReference(); > }; > if (is<CSSCalcOperationNode>(expression)) { > auto& operation = downcast<CSSCalcOperationNode>(expression); > set(operation); > for (auto& child : operation.children()) { > if (is<CSSCalcOperationNode>(child)) > set(downcast<CSSCalcOperationNode>(child)); > } > } > }; > > But this code assumes there are no grandchildren. If that’s wrong, then it should be written with recursion instead: > > auto setAllowsNegativePercentageReferenceIfNeeded = [] (CSSCalcExpressionNode& expression) { > if (is<CSSCalcOperationNode>(expression)) { > auto& operation = downcast<CSSCalcOperationNode>(expression); > if (operation.isMinOrMaxNode()) > operation.setAllowsNegativePercentageReference(); > for (auto& child : operation.children()) > setAllowsNegativePercentageReferenceIfNeeded(child); > } > }; I originally intended recursion here, so it was my mistake. I have changed this to recursion here. >> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.h:47 >> + RefPtr<CSSCalcExpressionNode> parseCalc(CSSParserTokenRange, CSSValueID function, bool); > > The argument type bool does not make it clear what the argument is, so it needs a name, allowNegativePercentageReference, here. Done. >> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:838 >> + return false; > > No need to write this boolean expression so long. Just this: > > return unitType == CSSUnitType::CSS_PERCENTAGE && numberOfChildren > 1; > > Other tiny style thoughts: It can be tricky to use a lambda in a way that the code *easier* to read; the current version makes it a little too hard. And I find it a bit arbitrary to do the different checks in two if statements rather than three or just one. If you do still like the lambda to document things, then this might read better: > > auto involvesPercentageComparisons = [&] { > return combinedUnitType == CSSUnitType::CSS_PERCENTAGE && m_children.size() > 1; > }; > if (isMinOrMaxNode() && allowsNegativePercentageReference() && involvesPercentageComparisons()) > return; > > This should be just as efficient and I think makes it pretty readable. I believe the five lines are easier to read than the longer code block. Using capturing rather than arguments for combinedUnitType and m_children does seem to help rather than hurt.. Thank you for your suggestion. I applied your 5 lines in the next patchset. >> Source/WebCore/css/calc/CSSCalcOperationNode.h:149 >> + unsigned m_allowsNegativePercentageReference : 1; > > This can just be a bool. There is no value to using a bitfield here, doesn't save memory and makes bigger code. Also if we are using. bool we can initialize here and don’t need to change the constructors. Done. >> Source/WebCore/css/parser/CSSPropertyParser.cpp:5562 >> + auto position = consumePositionCoordinates(range, context.mode, unitless, PositionSyntax::BackgroundPosition, /* allowsNegativePercentageReference */ true); > > This is why we use enumerations in WebKit for this kind of idiom. We don’t use comments to make such argument clear, instead we used named enumeration values that you can understand without a comment. Yepp,I thought that you might point this out, but I wanted to get review for this patch to check that if this change is ok to go on. I will add enum class as below to a new file NegativePercentageReference.h. enum class NegativePercentageReference : uint8_t { Allow, Forbid }; >> Source/WebCore/css/parser/CSSPropertyParserHelpers.h:133 >> +std::optional<PositionCoordinates> consumePositionCoordinates(CSSParserTokenRange&, CSSParserMode, UnitlessQuirk, PositionSyntax, bool = false); > > Needs a comment here, not at all obvious that this bool is allowsNegativePercentageReference. With a new enum class NegativePercentageReference, Done.
Comment on attachment 446343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446343&action=review >>> Source/WebCore/css/parser/CSSPropertyParser.cpp:5562 >>> + auto position = consumePositionCoordinates(range, context.mode, unitless, PositionSyntax::BackgroundPosition, /* allowsNegativePercentageReference */ true); >> >> This is why we use enumerations in WebKit for this kind of idiom. We don’t use comments to make such argument clear, instead we used named enumeration values that you can understand without a comment. > > Yepp,I thought that you might point this out, but I wanted to get review for this patch to check that if this change is ok to go on. > I will add enum class as below to a new file NegativePercentageReference.h. > > enum class NegativePercentageReference : uint8_t { > Allow, > Forbid > }; I would suggest something more like one of these: enum class NegativePercentagePolicy : bool { Allow, Forbid }; enum class NegativePercentageReferencePolicy : bool { Allow, Forbid }; The multi-line formatting doesn’t add much. The class shouldn’t be named NegativePercentageReference, because it’s not a reference, it’s a policy about references. And the name is getting so super-long, try to make it as short as possible.
Comment on attachment 446504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446504&action=review I’d be OK landing this, but I think we can do better. > Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:67 > + std::function<void(CSSCalcExpressionNode&)> setAllowsNegativePercentageReferenceIfNeeded = [&setAllowsNegativePercentageReferenceIfNeeded](CSSCalcExpressionNode& expression) { Type here should be "auto", not "std::function<...>". > Source/WebCore/css/calc/CSSCalcExpressionNodeParser.h:39 > +enum class NegativePercentageReference : uint8_t; bool (see also my naming suggestion above). > Source/WebCore/css/calc/CSSCalcExpressionNodeParser.h:49 > + RefPtr<CSSCalcExpressionNode> parseCalc(CSSParserTokenRange, CSSValueID function, NegativePercentageReference allowsNegativePercentageReference); With the right enum class name (see my naming suggestion above), we would not need an argument name here. That is a goal. Not great to say things twice. > Source/WebCore/css/calc/CSSCalcOperationNode.cpp:836 > auto combinedUnitType = m_children[0]->primitiveType(); > + auto involvesPercentageComparisons = [&]() { > + return combinedUnitType == CSSUnitType::CSS_PERCENTAGE && m_children.size() > 1; > + }; > + > + if (isMinOrMaxNode() && allowsNegativePercentageReference() && involvesPercentageComparisons()) > + return; Not really sure about the paragraphing here. The lack of blank line before definition the involvesPercentageComparisons, and the blank line after. Also, pretty sure we typically write [&] { ... }, without the () for the empty argument list. > Source/WebCore/css/calc/CSSCalcOperationNode.h:76 > + bool allowsNegativePercentageReference() { return m_allowsNegativePercentageReference; } Should be a const member function. > Source/WebCore/css/calc/CSSCalcValue.h:54 > + static RefPtr<CSSCalcValue> create(CSSValueID function, const CSSParserTokenRange&, CalculationCategory destinationCategory, ValueRange, const CSSCalcSymbolTable&, NegativePercentageReference = NegativePercentageReference::Forbid); Could use overloading here rather than an argument with a default value; one create function with and one without. That would remove the need to include the enum class definition in a header. > Source/WebCore/css/calc/NegativePercentageReference.h:33 > +enum class NegativePercentageReference : uint8_t { > + Allow, > + Forbid > +}; Should try to avoid having to create an entire header file just for this single enum class. > Source/WebCore/css/parser/CSSPropertyParserHelpers.h:96 > +RefPtr<CSSPrimitiveValue> consumeLengthOrPercent(CSSParserTokenRange&, CSSParserMode, ValueRange, UnitlessQuirk = UnitlessQuirk::Forbid, NegativePercentageReference = NegativePercentageReference::Forbid); Here this could be a boolean; no one is calling this with a constant, so the enumeration is not needed. We don’t have to use the enumerations all the way down unless you think it’s preferable. > Source/WebCore/css/parser/CSSPropertyParserHelpers.h:134 > +std::optional<PositionCoordinates> consumePositionCoordinates(CSSParserTokenRange&, CSSParserMode, UnitlessQuirk, PositionSyntax, NegativePercentageReference = NegativePercentageReference::Forbid); Ditto.
(In reply to Darin Adler from comment #6) > Comment on attachment 446343 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=446343&action=review > > >>> Source/WebCore/css/parser/CSSPropertyParser.cpp:5562 > >>> + auto position = consumePositionCoordinates(range, context.mode, unitless, PositionSyntax::BackgroundPosition, /* allowsNegativePercentageReference */ true); > >> > >> This is why we use enumerations in WebKit for this kind of idiom. We don’t use comments to make such argument clear, instead we used named enumeration values that you can understand without a comment. > > > > Yepp,I thought that you might point this out, but I wanted to get review for this patch to check that if this change is ok to go on. > > I will add enum class as below to a new file NegativePercentageReference.h. > > > > enum class NegativePercentageReference : uint8_t { > > Allow, > > Forbid > > }; > > I would suggest something more like one of these: > > enum class NegativePercentagePolicy : bool { Allow, Forbid }; > enum class NegativePercentageReferencePolicy : bool { Allow, Forbid }; > > The multi-line formatting doesn’t add much. The class shouldn’t be named > NegativePercentageReference, because it’s not a reference, it’s a policy > about references. And the name is getting so super-long, try to make it as > short as possible. Then I will use NegativePercentagePolicy, the shorter one.
Comment on attachment 446504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446504&action=review I applied all the comments here in the next patchset. Thank you for your review, Darin. >> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.cpp:67 >> + std::function<void(CSSCalcExpressionNode&)> setAllowsNegativePercentageReferenceIfNeeded = [&setAllowsNegativePercentageReferenceIfNeeded](CSSCalcExpressionNode& expression) { > > Type here should be "auto", not "std::function<...>". It seems that we can't use lambda declared with auto recursively. So for now I will leave std::function here. But if I find a way, I will visit here and change it then. >> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.h:39 >> +enum class NegativePercentageReference : uint8_t; > > bool (see also my naming suggestion above). I removed this because we use bool here. >> Source/WebCore/css/calc/CSSCalcExpressionNodeParser.h:49 >> + RefPtr<CSSCalcExpressionNode> parseCalc(CSSParserTokenRange, CSSValueID function, NegativePercentageReference allowsNegativePercentageReference); > > With the right enum class name (see my naming suggestion above), we would not need an argument name here. That is a goal. Not great to say things twice. This was changed to bool too. >> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:836 >> + return; > > Not really sure about the paragraphing here. The lack of blank line before definition the involvesPercentageComparisons, and the blank line after. > > Also, pretty sure we typically write [&] { ... }, without the () for the empty argument list. I removed the blank line after the definition of involvesPercentageComparisons. >> Source/WebCore/css/calc/CSSCalcOperationNode.h:76 >> + bool allowsNegativePercentageReference() { return m_allowsNegativePercentageReference; } > > Should be a const member function. Done. >> Source/WebCore/css/calc/CSSCalcValue.h:54 >> + static RefPtr<CSSCalcValue> create(CSSValueID function, const CSSParserTokenRange&, CalculationCategory destinationCategory, ValueRange, const CSSCalcSymbolTable&, NegativePercentageReference = NegativePercentageReference::Forbid); > > Could use overloading here rather than an argument with a default value; one create function with and one without. That would remove the need to include the enum class definition in a header. I changed this to bool because no one calls it directly with enum constant. >> Source/WebCore/css/calc/NegativePercentageReference.h:33 >> +}; > > Should try to avoid having to create an entire header file just for this single enum class. I removed this and add below to CSSPropertyParserHelpers.h enum class NegativePercentagePolicy : bool { Forbid, Allow }; >> Source/WebCore/css/parser/CSSPropertyParserHelpers.h:96 >> +RefPtr<CSSPrimitiveValue> consumeLengthOrPercent(CSSParserTokenRange&, CSSParserMode, ValueRange, UnitlessQuirk = UnitlessQuirk::Forbid, NegativePercentageReference = NegativePercentageReference::Forbid); > > Here this could be a boolean; no one is calling this with a constant, so the enumeration is not needed. We don’t have to use the enumerations all the way down unless you think it’s preferable. Ok, I will change this to bool. >> Source/WebCore/css/parser/CSSPropertyParserHelpers.h:134 >> +std::optional<PositionCoordinates> consumePositionCoordinates(CSSParserTokenRange&, CSSParserMode, UnitlessQuirk, PositionSyntax, NegativePercentageReference = NegativePercentageReference::Forbid); > > Ditto. This one is called with enum constant from CSSPropertyParser.cpp above, so I will leave this as enum type.
Created attachment 446729 [details] Patch for landing
Committed r286897 (245125@main): <https://commits.webkit.org/245125@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 446729 [details].
<rdar://problem/86349764>