Fix abspos-020.html
Created attachment 441875 [details] Patch
Created attachment 442315 [details] Patch
Created attachment 442340 [details] Patch
Created attachment 442363 [details] Patch
Created attachment 442387 [details] Patch
Created attachment 442397 [details] Patch
Created attachment 442402 [details] Patch
Created attachment 442406 [details] Patch
Created attachment 442581 [details] Patch
Created attachment 442582 [details] Patch
<rdar://problem/84707554>
Created attachment 442593 [details] Patch
Created attachment 442605 [details] Patch
Created attachment 442936 [details] Patch
Created attachment 443233 [details] Patch
Created attachment 443242 [details] Patch
Created attachment 443282 [details] Patch
Created attachment 443291 [details] Patch
Created attachment 443295 [details] Patch
Created attachment 443339 [details] Patch
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.
Created attachment 444394 [details] Patch
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.
(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.
Created attachment 444494 [details] Patch
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.
Created attachment 445824 [details] Patch
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.
Created attachment 446140 [details] Patch
Created attachment 449148 [details] Patch
Created attachment 458043 [details] Patch
Created attachment 461793 [details] Patch
Created attachment 462183 [details] Patch
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].