Add logical values for 'float' and 'clear': https://drafts.csswg.org/css-logical-1/#float-clear
<rdar://problem/70578855>
Created attachment 425018 [details] Patch
Created attachment 425028 [details] Patch
Comment on attachment 425028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425028&action=review > Source/WebCore/layout/layouttree/LayoutBox.cpp:208 > +Float Box::physicalFloat() const > +{ > + bool isInlineFlipped = containingBlockDirection() == TextDirection::RTL; > + Float currentValue = style().floating(); > + > + if (currentValue == Float::InlineStart) > + return !isInlineFlipped ? Float::Left : Float::Right; > + > + if (currentValue == Float::InlineEnd) > + return !isInlineFlipped ? Float::Right : Float::Left; > + > + return currentValue; > +} I think you can move `physicalFloat` and `physicalClear` to RenderStyle, which stores the inherited direction property. https://github.com/WebKit/WebKit/blob/538d690b5989b90efcb107a879dbbae8c018e548/Source/WebCore/rendering/style/RenderStyle.h#L392 Float RenderStyle::physicalFloat() const { bool isLtr = direction() == TextDirection::LTR; Float value = style().floating(); if (currentValue == Float::InlineStart) return isLtr ? Float::Left : Float::Right; if (currentValue == Float::InlineEnd) return isLtr ? Float::Right : Float::Left; return value; } > Source/WebCore/rendering/style/RenderStyle.h:1872 > + unsigned clear : 4; // Clear This can be represented by 3 bits (up to 8 values). > Source/WebCore/rendering/style/RenderStyle.h:1875 > + unsigned floating : 4; // Float This can be represented by 3 bits (up to 8 values).
(In reply to Tyler Wilcock from comment #4) > Comment on attachment 425028 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425028&action=review > > > Source/WebCore/layout/layouttree/LayoutBox.cpp:208 > > +Float Box::physicalFloat() const > > +{ > > + bool isInlineFlipped = containingBlockDirection() == TextDirection::RTL; > > + Float currentValue = style().floating(); > > + > > + if (currentValue == Float::InlineStart) > > + return !isInlineFlipped ? Float::Left : Float::Right; > > + > > + if (currentValue == Float::InlineEnd) > > + return !isInlineFlipped ? Float::Right : Float::Left; > > + > > + return currentValue; > > +} > > I think you can move `physicalFloat` and `physicalClear` to RenderStyle, > which stores the inherited direction property. > > https://github.com/WebKit/WebKit/blob/ > 538d690b5989b90efcb107a879dbbae8c018e548/Source/WebCore/rendering/style/ > RenderStyle.h#L392 > > Float RenderStyle::physicalFloat() const > { > bool isLtr = direction() == TextDirection::LTR; > Float value = style().floating(); > > if (currentValue == Float::InlineStart) > return isLtr ? Float::Left : Float::Right; > > if (currentValue == Float::InlineEnd) > return isLtr ? Float::Right : Float::Left; > > return value; > } This needs to be done at the layout level, the spec clearly says: "The mapping on these properties uses the writing mode of the element’s containing block." Doing this in `RenderStyle` fails because in this case: > <div style="direction: rtl"><div style="display: inline; direction: ltr;"><div style="float: inline-start"></div></div></div> should be like `float: right`, not `float: left`. > > Source/WebCore/rendering/style/RenderStyle.h:1872 > > + unsigned clear : 4; // Clear > > This can be represented by 3 bits (up to 8 values). > > > Source/WebCore/rendering/style/RenderStyle.h:1875 > > + unsigned floating : 4; // Float > > This can be represented by 3 bits (up to 8 values). Thanks! Will fix.
> This needs to be done at the layout level, the spec clearly says: "The mapping on these properties uses the writing mode of the element’s containing block." > Doing this in `RenderStyle` fails because in this case: Ah, my mistake. Thanks for the explanation.
Comment on attachment 425028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425028&action=review > Source/WebCore/rendering/RenderObject.cpp:2113 > +Float RenderObject::physicalFloat() const > +{ > + bool isInlineFlipped = containingBlockDirection() == TextDirection::RTL; > + Float currentValue = style().floating(); > + > + if (currentValue == Float::InlineStart) > + return !isInlineFlipped ? Float::Left : Float::Right; > + > + if (currentValue == Float::InlineEnd) > + return !isInlineFlipped ? Float::Right : Float::Left; > + > + return currentValue; > +} Would these read better as the following? if (currentValue == Float::InlineStart) return isLtr ? Float::Left : Float::Right; if (currentValue == Float::InlineEnd) return isLtr ? Float::Right : Float::Left; Or, keeping `isInlineFlipped`: if (currentValue == Float::InlineStart) return isInlineFlipped ? Float::Right : Float::Left; if (currentValue == Float::InlineEnd) return isInlineFlipped ? Float::Left : Float::Right; > Source/WebCore/rendering/line/BreakingContext.h:296 > + if (m_ignoringSpaces && currentObject()->physicalClear() != Clear::None) Can you use the existing `br` local ~10 lines up vs. calling `currentObject()`? > Source/WebCore/rendering/line/BreakingContext.h:305 > + clear = currentObject()->physicalClear(); Ditto.
Antti and I had a chat on this and since he is planning on making some changes of how computed styles are exposed to the layout process, I think for now we should go with something like this: - add 2 static helper functions to RenderStyle (we've got a few of these e.g. collapseWhiteSpace). These functions would take a renderer and return something like enum class UsedFloat(Clear) { } to make it clear that these are not the computed values (these functions would eventually move over to one of the new style classes) - try not use the name physical in the context of layout. It makes it confusing especially when you start involving vertical layout. - containingBlockDirection() could be just a simple auto containingBlockDirection = renderer.containingBlock().style().direction(); I would not add such a single-use function to the renderer interface. e.g class RenderStyle { ... enum class UsedFloat { No, Left, Right }; static UsedFloat usedFloat(const RenderElement&); ... } This is still rather error-prone (like how do you know whether to call this instead of the original RenderStyle function) but this is supposed to be temporary as the upcoming refactoring will scope it properly. (and you could just ignore the LFC codepath for now. or alternatively you could make it a bit more generic by not taking the renderer -but the LFC codebase is not enabled so not really a priority)
Comment on attachment 425028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425028&action=review > Source/WebCore/layout/layouttree/LayoutBox.cpp:198 > + bool isInlineFlipped = containingBlockDirection() == TextDirection::RTL; Given we only need this for start/end, is it OK that we are computing it unconditionally? Will the extra work have a measurable performance impact? > Source/WebCore/layout/layouttree/LayoutBox.cpp:212 > + bool isInlineFlipped = containingBlockDirection() == TextDirection::RTL; Ditto. > Source/WebCore/rendering/RenderObject.cpp:2103 > + bool isInlineFlipped = containingBlockDirection() == TextDirection::RTL; Is there a way to avoid having the code be so repetitive? This is the third almost-identical function of four. > LayoutTests/ChangeLog:14 > + * imported/w3c/web-platform-tests/css/css-logical/logical-values-float-clear-1.html: Added. > + * imported/w3c/web-platform-tests/css/css-logical/logical-values-float-clear-2.html: Added. > + * imported/w3c/web-platform-tests/css/css-logical/logical-values-float-clear-3.html: Added. > + * imported/w3c/web-platform-tests/css/css-logical/logical-values-float-clear-4.html: Added. ChangeLog does not mention TestExpectations, nor explain the changes in that file. > LayoutTests/TestExpectations:2947 > +webkit.org/b/224104 imported/w3c/web-platform-tests/css/css-logical/logical-values-float-clear-reftest.html [ Skip ] I understand that this test fails. But I don’t understand why we need to skip this test instead of expecting a failure now. Why is that?
Created attachment 426221 [details] Patch
Comment on attachment 426221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426221&action=review This looks much better now. Thanks! > Source/WebCore/rendering/ComplexLineLayout.cpp:1362 > + UsedClear clear = RenderStyle::usedClear(*lastObject); auto clear = > Source/WebCore/rendering/RenderBlock.cpp:2345 > + UsedClear childUsedClear = RenderStyle::usedClear(*child); auto childUsedClear = > Source/WebCore/rendering/RenderBlockFlow.cpp:2895 > + UsedClear usedClear = RenderStyle::usedClear(child); auto > Source/WebCore/rendering/RenderBlockFlow.cpp:4328 > + UsedFloat prevFloatValue = RenderStyle::usedFloat(*prevFloat); > + UsedClear childClearValue = RenderStyle::usedClear(*child); auto and please change prev to previous > Source/WebCore/rendering/style/RenderStyle.cpp:2668 > +UsedClear RenderStyle::usedClear(const RenderObject& renderer) We should never call this on a RenderText so it could just be a RenderElement (however some of the call sites may still pass in a RenderObject. I'd give it a try but don't not a must -same for ::UsedFloat) > Source/WebCore/rendering/style/RenderStyle.cpp:2670 > + Clear specifiedValue = renderer.style().clear(); auto and computedValue > Source/WebCore/rendering/style/RenderStyle.cpp:2691 > + Float specifiedValue = renderer.style().floating(); auto and computedValue
Antti may wanna look at the css bits but I am sure it's all good. Will talk to him and r+ it later today.
Comment on attachment 426221 [details] Patch looks good
Comment on attachment 426221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426221&action=review > Source/WebCore/rendering/style/RenderStyle.cpp:2689 > +UsedFloat RenderStyle::usedFloat(const RenderObject& renderer) At some point we should add something like RenderStyleHelpers.h where we can put this sort of things as standalone functions. RenderStyle is big enough as-is, semantics should go somewhere else.
Comment on attachment 426221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426221&action=review > Source/WebCore/rendering/RenderBlockFlow.cpp:4327 > + UsedFloat prevFloatValue = RenderStyle::usedFloat(*prevFloat); Aren't you potentially dereferencing a nullptr here?
Comment on attachment 426221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426221&action=review > Source/WebCore/rendering/line/BreakingContext.h:296 > + if (m_ignoringSpaces && RenderStyle::usedClear(*currentObject()) != UsedClear::None) What guarantees currentObject() is non-null here? > Source/WebCore/rendering/line/BreakingContext.h:305 > + usedClear = RenderStyle::usedClear(*currentObject()); And here
Created attachment 426235 [details] Patch
Created attachment 426238 [details] Patch
Created attachment 426350 [details] Patch
Committed r276216 (236698@main): <https://commits.webkit.org/236698@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426350 [details].