Bug 232012

Summary: Add over-constrained direction check to computePositionedLogicalHeightUsing
Product: WebKit Reporter: Rob Buis <rbuis>
Component: Layout and RenderingAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, simon.fraser, svillar, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=209080
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Rob Buis 2021-10-20 06:33:58 PDT 
Fix abspos-020.html
Comment 1 Rob Buis 2021-10-20 06:37:07 PDT 
Created attachment 441875 [details]
Patch
Comment 2 Rob Buis 2021-10-24 12:06:01 PDT 
Created attachment 442315 [details]
Patch
Comment 3 Rob Buis 2021-10-24 23:34:07 PDT 
Created attachment 442340 [details]
Patch
Comment 4 Rob Buis 2021-10-25 05:53:16 PDT 
Created attachment 442363 [details]
Patch
Comment 5 Rob Buis 2021-10-25 10:58:59 PDT 
Created attachment 442387 [details]
Patch
Comment 6 Rob Buis 2021-10-25 12:20:54 PDT 
Created attachment 442397 [details]
Patch
Comment 7 Rob Buis 2021-10-25 12:50:25 PDT 
Created attachment 442402 [details]
Patch
Comment 8 Rob Buis 2021-10-25 13:41:59 PDT 
Created attachment 442406 [details]
Patch
Comment 9 Rob Buis 2021-10-27 05:14:47 PDT 
Created attachment 442581 [details]
Patch
Comment 10 Rob Buis 2021-10-27 06:19:10 PDT 
Created attachment 442582 [details]
Patch
Comment 11 Radar WebKit Bug Importer 2021-10-27 06:34:19 PDT 
<rdar://problem/84707554>
Comment 12 Rob Buis 2021-10-27 08:20:21 PDT 
Created attachment 442593 [details]
Patch
Comment 13 Rob Buis 2021-10-27 10:35:37 PDT 
Created attachment 442605 [details]
Patch
Comment 14 Rob Buis 2021-10-31 07:48:57 PDT 
Created attachment 442936 [details]
Patch
Comment 15 Rob Buis 2021-11-03 13:26:28 PDT 
Created attachment 443233 [details]
Patch
Comment 16 Rob Buis 2021-11-03 14:47:51 PDT 
Created attachment 443242 [details]
Patch
Comment 17 Rob Buis 2021-11-04 00:58:04 PDT 
Created attachment 443282 [details]
Patch
Comment 18 Rob Buis 2021-11-04 04:05:02 PDT 
Created attachment 443291 [details]
Patch
Comment 19 Rob Buis 2021-11-04 04:49:20 PDT 
Created attachment 443295 [details]
Patch
Comment 20 Rob Buis 2021-11-04 14:23:16 PDT 
Created attachment 443339 [details]
Patch
Comment 21 Sergio Villar Senin 2021-11-16 00:34:41 PST 
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.
Comment 22 Rob Buis 2021-11-16 08:43:07 PST 
Created attachment 444394 [details]
Patch
Comment 23 Rob Buis 2021-11-17 01:15:44 PST 
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.
Comment 24 Rob Buis 2021-11-17 01:16:38 PST 
(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.
Comment 25 Rob Buis 2021-11-17 01:19:42 PST 
Created attachment 444494 [details]
Patch
Comment 26 Sergio Villar Senin 2021-12-02 01:57:49 PST 
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.
Comment 27 Rob Buis 2021-12-03 02:15:40 PST 
Created attachment 445824 [details]
Patch
Comment 28 Rob Buis 2021-12-03 03:16:08 PST 
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.
Comment 29 Rob Buis 2021-12-07 02:12:48 PST 
Created attachment 446140 [details]
Patch
Comment 30 Rob Buis 2022-01-14 01:38:51 PST 
Created attachment 449148 [details]
Patch
Comment 31 Rob Buis 2022-04-21 01:46:13 PDT 
Created attachment 458043 [details]
Patch
Comment 32 Rob Buis 2022-08-22 03:20:50 PDT 
Created attachment 461793 [details]
Patch
Comment 33 Rob Buis 2022-09-07 06:43:16 PDT 
Created attachment 462183 [details]
Patch
Comment 34 EWS 2022-09-09 13:53:53 PDT 
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].