Bug 218087 – [css-logical] Add logical values for 'float' and 'clear'

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

Add logical values for 'float' and 'clear':
https://drafts.csswg.org/css-logical-1/#float-clear

[Radar WebKit Bug Importer]

<rdar://problem/70578855>

[Tim Nguyen (:ntim)]

Created attachment 425018 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 425028 [details]
Patch

[Tyler Wilcock]

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).

[Tim Nguyen (:ntim)]

(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.

[Tyler Wilcock]

> 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.

[Tyler Wilcock]

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.

[zalan]

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)

[Darin Adler]

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?

[Tim Nguyen (:ntim)]

Created attachment 426221 [details]
Patch

[zalan]

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

[zalan]

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.

[Antti Koivisto]

Comment on attachment 426221 [details]
Patch

looks good

[Antti Koivisto]

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.

[Antti Koivisto]

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?

[Antti Koivisto]

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

[Tim Nguyen (:ntim)]

Created attachment 426235 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 426238 [details]
Patch

[zalan]

Created attachment 426350 [details]
Patch

[EWS]

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].