Bug 228914 – [css-writing-modes] Fix absolutely positioning with orthogonal writing modes

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Sergio Villar Senin]

[css-writing-modes] Fix absolutely positioning with orthogonal writing modes

[Sergio Villar Senin]

Created attachment 435250 [details]
Patch

[Sergio Villar Senin]

Created attachment 435268 [details]
Patch

[Sergio Villar Senin]

Created attachment 435295 [details]
Patch

[Sergio Villar Senin]

Created attachment 435337 [details]
Patch

[Darin Adler]

Need to fix these iOS-wk2 failures?

[Sergio Villar Senin]

(In reply to Darin Adler from comment #5)
> Need to fix these iOS-wk2 failures?

Not really sure what to do with them. There is no special layout code for iOS so I'd bet it's a painting issue. The iOS port is painting the boxes 1px bigger than any other port. I'd bet it's a rounding issue.

I'll try to fix the WPT tests so that they don't generate those 1px digg issues. In the event I'm unable to, I guess I'll upload specific expectations for iOS.

[Radar WebKit Bug Importer]

<rdar://problem/81980953>

[Sergio Villar Senin]

Created attachment 435756 [details]
Patch

[Sergio Villar Senin]

Created attachment 436168 [details]
Patch

[Sergio Villar Senin]

Created attachment 436193 [details]
Patch

[Sergio Villar Senin]

Gentle ping for reviewers

[Rob Buis]

Comment on attachment 436193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436193&action=review

Not an expert on this, but it mostly looks good to me.

> Source/WebCore/ChangeLog:12
> +        This is a two level fix. First of all we had to modify the two methods that retrieve the static distance to use

I think it may be better to put this in present tense.

> Source/WebCore/rendering/RenderBox.cpp:3719
> +        LayoutUnit staticPosition = isOrthogonal(*child, *parent) ? child->layer()->staticBlockPosition() - containerBlock.borderBefore() : child->layer()->staticInlinePosition() - containerBlock.borderLogicalLeft();

I wonder if some of these statement could be a lambda to more easily understand what is going on.

> Source/WebCore/rendering/RenderBox.cpp:4275
> +    auto haveOrthogonalWritingModes = containerBlock.style().isHorizontalWritingMode() != child->style().isHorizontalWritingMode();

Have or has?

[Sergio Villar Senin]

Comment on attachment 436193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436193&action=review

>> Source/WebCore/ChangeLog:12
>> +        This is a two level fix. First of all we had to modify the two methods that retrieve the static distance to use
> 
> I think it may be better to put this in present tense.

Ok

>> Source/WebCore/rendering/RenderBox.cpp:3719
>> +        LayoutUnit staticPosition = isOrthogonal(*child, *parent) ? child->layer()->staticBlockPosition() - containerBlock.borderBefore() : child->layer()->staticInlinePosition() - containerBlock.borderLogicalLeft();
> 
> I wonder if some of these statement could be a lambda to more easily understand what is going on.

I'll contact you offline as I am not sure how to make it easier to understand with a lambda.

>> Source/WebCore/rendering/RenderBox.cpp:4275
>> +    auto haveOrthogonalWritingModes = containerBlock.style().isHorizontalWritingMode() != child->style().isHorizontalWritingMode();
> 
> Have or has?

I used have because it refers to the container and the child, not to the _this_ object. But I don't have an strong opinion here.

[Rob Buis]

Comment on attachment 436193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436193&action=review

>>> Source/WebCore/rendering/RenderBox.cpp:3719
>>> +        LayoutUnit staticPosition = isOrthogonal(*child, *parent) ? child->layer()->staticBlockPosition() - containerBlock.borderBefore() : child->layer()->staticInlinePosition() - containerBlock.borderLogicalLeft();
>> 
>> I wonder if some of these statement could be a lambda to more easily understand what is going on.
> 
> I'll contact you offline as I am not sure how to make it easier to understand with a lambda.

Ok, as discussed, lambdas are probably overkill in these cases.

[Manuel Rego Casasnovas]

Comment on attachment 436193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436193&action=review

> Source/WebCore/rendering/RenderBox.cpp:3691
> +// FIXME: evaluate whether this should be a method of RenderObject instead.

With the current implementation it seems like a good idea to move it there.

> Source/WebCore/rendering/RenderBox.cpp:3692
> +static inline bool isOrthogonal(const RenderObject& renderer, const RenderObject& ancestor)

Why you call this "ancestor" instead of "parent"?

Maybe the proper word would be "containingBlock"?

> Source/WebCore/rendering/RenderBox.cpp:3737
> +        LayoutUnit staticPosition = isOrthogonal(*child, *parent) ? child->layer()->staticBlockPosition() + containerLogicalWidth + containerBlock.borderBefore() : child->layer()->staticInlinePosition() + containerLogicalWidth + containerBlock.borderLogicalLeft();

I don't understand this: "containerLogicalWidth + containerBlock.borderBefore()", it's like you're summing up a width with a height related stuff.
Maybe the name of the variable "containerLogicalWidth" is wrong... dunno really.

> Source/WebCore/rendering/RenderBox.cpp:3928
> +    auto haveOrthogonalWritingModes = containerBlock.style().isHorizontalWritingMode() != child->style().isHorizontalWritingMode();

Couldn't you use "isOrthogonal(*child, *containerBlock)"?

> Source/WebCore/rendering/RenderBox.cpp:3934
> +            || (logicalLeftAndRightAreAuto && !containerBlock.style().isLeftToRightDirection()))) {

I don't get what logicalLeftAndRightAreAuto has to do with all this, should we update the comment to mention something about this?

> Source/WebCore/rendering/RenderBox.cpp:4274
> +    auto logicalTopAndBottomAreAuto = child->style().logicalTop().isAuto() && child->style().logicalBottom().isAuto();

Again I don't get why we need this.

>>> Source/WebCore/rendering/RenderBox.cpp:4275
>>> +    auto haveOrthogonalWritingModes = containerBlock.style().isHorizontalWritingMode() != child->style().isHorizontalWritingMode();
>> 
>> Have or has?
> 
> I used have because it refers to the container and the child, not to the _this_ object. But I don't have an strong opinion here.

Ditto about using isOrthogonal().

[Rob Buis]

Comment on attachment 436193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436193&action=review

>> Source/WebCore/rendering/RenderBox.cpp:3928
>> +    auto haveOrthogonalWritingModes = containerBlock.style().isHorizontalWritingMode() != child->style().isHorizontalWritingMode();
> 
> Couldn't you use "isOrthogonal(*child, *containerBlock)"?

I thought the same thing, but one goes through renderer and one through style :)

[Sergio Villar Senin]

Comment on attachment 436193 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436193&action=review

>> Source/WebCore/rendering/RenderBox.cpp:3691
>> +// FIXME: evaluate whether this should be a method of RenderObject instead.
> 
> With the current implementation it seems like a good idea to move it there.

Sure but if you don't mind I'd leave it for a follow up as there are currently several other pieces of code checking exactly the same, so I guess it'd be better to migrate them all at once.

>> Source/WebCore/rendering/RenderBox.cpp:3692
>> +static inline bool isOrthogonal(const RenderObject& renderer, const RenderObject& ancestor)
> 
> Why you call this "ancestor" instead of "parent"?
> 
> Maybe the proper word would be "containingBlock"?

Because I don't want to make any assumption about the relationship between both of them. It doesn't have to be the containing block or the parent. Actually it does not even have to be an ancestor, we just compare the writing modes.

>> Source/WebCore/rendering/RenderBox.cpp:3737
>> +        LayoutUnit staticPosition = isOrthogonal(*child, *parent) ? child->layer()->staticBlockPosition() + containerLogicalWidth + containerBlock.borderBefore() : child->layer()->staticInlinePosition() + containerLogicalWidth + containerBlock.borderLogicalLeft();
> 
> I don't understand this: "containerLogicalWidth + containerBlock.borderBefore()", it's like you're summing up a width with a height related stuff.
> Maybe the name of the variable "containerLogicalWidth" is wrong... dunno really.

I'm starting to think this is not correct and is forcing us to add extra conditions to computeLogicalLeftPositionedOffset(). I'll double check

>>> Source/WebCore/rendering/RenderBox.cpp:3928
>>> +    auto haveOrthogonalWritingModes = containerBlock.style().isHorizontalWritingMode() != child->style().isHorizontalWritingMode();
>> 
>> Couldn't you use "isOrthogonal(*child, *containerBlock)"?
> 
> I thought the same thing, but one goes through renderer and one through style :)

Right, I'll unify those.

>> Source/WebCore/rendering/RenderBox.cpp:3934
>> +            || (logicalLeftAndRightAreAuto && !containerBlock.style().isLeftToRightDirection()))) {
> 
> I don't get what logicalLeftAndRightAreAuto has to do with all this, should we update the comment to mention something about this?

That's to check whether or not the item is auto positioned. When any of those properties are not auto then computeInlineStaticDistance() or computeBlockStaticDistance() early return. This means that orthogonal flow corrections are not applied in those cases and we have to do it now. That said I think we can get rid off of one of the branches if we adjust properly computeInlinStaticPosition(). Will provide a new patch.

>> Source/WebCore/rendering/RenderBox.cpp:4274
>> +    auto logicalTopAndBottomAreAuto = child->style().logicalTop().isAuto() && child->style().logicalBottom().isAuto();
> 
> Again I don't get why we need this.

Same comment as before

>>>> Source/WebCore/rendering/RenderBox.cpp:4275
>>>> +    auto haveOrthogonalWritingModes = containerBlock.style().isHorizontalWritingMode() != child->style().isHorizontalWritingMode();
>>> 
>>> Have or has?
>> 
>> I used have because it refers to the container and the child, not to the _this_ object. But I don't have an strong opinion here.
> 
> Ditto about using isOrthogonal().

Sure.

[Sergio Villar Senin]

Created attachment 437127 [details]
Patch

[Sergio Villar Senin]

Created attachment 437168 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 437168 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437168&action=review

Awesome patch, that improves a lot of cases. r=me

> Source/WebCore/rendering/RenderBox.cpp:3693
> +static inline bool isOrthogonal(const RenderObject& renderer, const RenderObject& ancestor)

Nit: If it doesn't have to be an ancestor, then maybe use "other" or something like that.

[Sergio Villar Senin]

Committed r281995 (241302@main): <https://commits.webkit.org/241302@main>