Bug 232012 – Add over-constrained direction check to computePositionedLogicalHeightUsing

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

[Rob Buis]

Fix abspos-020.html

[Rob Buis]

Created attachment 441875 [details]
Patch

[Rob Buis]

Created attachment 442315 [details]
Patch

[Rob Buis]

Created attachment 442340 [details]
Patch

[Rob Buis]

Created attachment 442363 [details]
Patch

[Rob Buis]

Created attachment 442387 [details]
Patch

[Rob Buis]

Created attachment 442397 [details]
Patch

[Rob Buis]

Created attachment 442402 [details]
Patch

[Rob Buis]

Created attachment 442406 [details]
Patch

[Rob Buis]

Created attachment 442581 [details]
Patch

[Rob Buis]

Created attachment 442582 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/84707554>

[Rob Buis]

Created attachment 442593 [details]
Patch

[Rob Buis]

Created attachment 442605 [details]
Patch

[Rob Buis]

Created attachment 442936 [details]
Patch

[Rob Buis]

Created attachment 443233 [details]
Patch

[Rob Buis]

Created attachment 443242 [details]
Patch

[Rob Buis]

Created attachment 443282 [details]
Patch

[Rob Buis]

Created attachment 443291 [details]
Patch

[Rob Buis]

Created attachment 443295 [details]
Patch

[Rob Buis]

Created attachment 443339 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 443339 [details]
Patch

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

The patch looks sane and correct overall. I wonder whether we're fixing too many cases in a single patch. Would it be possible to split the patch in 2-3 ones or does it only make sense when all the changes are in place?

I was wondering whether we could use an iterative approach fixing case by case.

> Source/WebCore/rendering/RenderBox.cpp:4036
> +            if (isLeftRightOverconstrained && containerDirection == TextDirection::RTL)

Nit: perhaps it's enough to directly use the !isOrthogonal in the if condition and avoid having that extra variable

> Source/WebCore/rendering/RenderBox.cpp:4396
> +            if (isLeftRightOverconstrained) {

Ditto.

> LayoutTests/ChangeLog:9
> +        * platform/ios/TestExpectations:

Add something here about the tests the patch is fixing. Also the new iOS failures require an explanation.

[Rob Buis]

Created attachment 444394 [details]
Patch

[Rob Buis]

Comment on attachment 443339 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:4036
>> +            if (isLeftRightOverconstrained && containerDirection == TextDirection::RTL)
> 
> Nit: perhaps it's enough to directly use the !isOrthogonal in the if condition and avoid having that extra variable

Sure, fixed.

>> Source/WebCore/rendering/RenderBox.cpp:4396
>> +            if (isLeftRightOverconstrained) {
> 
> Ditto.

Done.

>> LayoutTests/ChangeLog:9
>> +        * platform/ios/TestExpectations:
> 
> Add something here about the tests the patch is fixing. Also the new iOS failures require an explanation.

Done.

[Rob Buis]

(In reply to Sergio Villar Senin from comment #21)
> Comment on attachment 443339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443339&action=review
> 
> The patch looks sane and correct overall. I wonder whether we're fixing too
> many cases in a single patch. Would it be possible to split the patch in 2-3
> ones or does it only make sense when all the changes are in place?
> 
> I was wondering whether we could use an iterative approach fixing case by
> case.

Good idea, I created https://bugs.webkit.org/show_bug.cgi?id=233189 which probably should go in first.

[Rob Buis]

Created attachment 444494 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 444494 [details]
Patch

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

Thanks for your patience.

> Source/WebCore/rendering/RenderBox.cpp:4453
> +                    logicalTopValue = (availableSpace + logicalTopValue) - computedValues.m_margins.m_before - computedValues.m_margins.m_after;

I don't understand why we're using the availableSpace here. The available space makes sense in the block direction of the container. But the container block direction is orthogonal to the child's top-bottom direction. If any we should use the available space in the child's block direction (since we're looking for the logical top).

> Source/WebCore/rendering/RenderBox.cpp:4455
> +                    logicalTopValue = valueForLength(style().left(), containerLogicalHeight);

I don't understand this too.

[Rob Buis]

Created attachment 445824 [details]
Patch

[Rob Buis]

Comment on attachment 444494 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:4453
>> +                    logicalTopValue = (availableSpace + logicalTopValue) - computedValues.m_margins.m_before - computedValues.m_margins.m_after;
> 
> I don't understand why we're using the availableSpace here. The available space makes sense in the block direction of the container. But the container block direction is orthogonal to the child's top-bottom direction. If any we should use the available space in the child's block direction (since we're looking for the logical top).

I applied the same principle as in the similar over-constrained case in computePositionedLogicalWidthUsing.

>> Source/WebCore/rendering/RenderBox.cpp:4455
>> +                    logicalTopValue = valueForLength(style().left(), containerLogicalHeight);
> 
> I don't understand this too.

This is to chose explicit left instead of logical left, to implement the substitution rules explained in for example abs-pos-non-replaced-vrl-220.xht.
I noticed this was not done in case the direction is rtf, so I fixed that in the latest patch.

[Rob Buis]

Created attachment 446140 [details]
Patch

[Rob Buis]

Created attachment 449148 [details]
Patch

[Rob Buis]

Created attachment 458043 [details]
Patch

[Rob Buis]

Created attachment 461793 [details]
Patch

[Rob Buis]

Created attachment 462183 [details]
Patch

[EWS]

Committed 254324@main (1787f669ccb1): <https://commits.webkit.org/254324@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 462183 [details].