Summary: | Wrong static position for out-of-flow positioned element with different writing-mode than its containing block | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Oriol Brufau <obrufau> | ||||||||||||||||||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | changseok, clopez, esprehn+autocc, ews-watchlist, glenn, jfernandez, kondapallykalyan, obrufau, pdr, rego, simon.fraser, svillar, webkit-bug-importer, zalan, zsun | ||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Oriol Brufau
2018-09-11 12:09:43 PDT
Chromium was fixed in M76 according to http://crbug.com/883574 This is tested by the following WPT tests: css/css-grid/abspos/positioned-grid-descendants-???.html css/css-grid/abspos/orthogonal-positioned-grid-descendants-???.html Created attachment 427332 [details]
Patch
Comment on attachment 427332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427332&action=review > Source/WebCore/rendering/RenderBox.cpp:3997 > + staticLogicalTop += child->isHorizontalWritingMode() ? staticLogicalTop : renderBox.logicalLeft(); Shouldn't it be "? renderBox.logicalTop() : renderBox.logicalLeft()" ? Created attachment 427339 [details]
Patch
(In reply to Sergio Villar Senin from comment #3) > Comment on attachment 427332 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427332&action=review > > > Source/WebCore/rendering/RenderBox.cpp:3997 > > + staticLogicalTop += child->isHorizontalWritingMode() ? staticLogicalTop : renderBox.logicalLeft(); > > Shouldn't it be "? renderBox.logicalTop() : renderBox.logicalLeft()" ? Yes. It should be. I must have pressed wrong button while view my changes locally... Thank you! Created attachment 427653 [details]
Patch
Created attachment 427657 [details]
Patch
Created attachment 427766 [details]
Patch
Created attachment 427795 [details]
Patch
Created attachment 427861 [details]
Patch
Testing. Ignore. Created attachment 427863 [details]
Patch
Comment on attachment 427863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427863&action=review > Source/WebCore/rendering/RenderBox.cpp:3573 > + if (!child->isHorizontalWritingMode() && parent->isHorizontalWritingMode() && child->style().isFlippedLinesWritingMode() && !parent->style().isFlippedBlocksWritingMode()) We should refactor this condition in a new inline method with a more descriptive name so we could use it in both this method and in the one bellow as well (in computeBlockStaticDistance() you'll still have to add && parentDirection == TextDirection::LTR but that's fine). I was thinking about a very descriptive name like childIsVLRinHTBParent(). WDTY? Created attachment 428364 [details]
Patch
Created attachment 428366 [details]
Patch
Comment on attachment 428366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428366&action=review > Source/WebCore/rendering/RenderBox.cpp:3556 > +static inline bool childIsVLRParentInHTB(const RenderBox* child) The name seems confusing to me, sounds like "child is parent". Maybe isVerticalLrChildInHorizontalTbParent? > Source/WebCore/rendering/RenderBox.cpp:3561 > + if (!child->isHorizontalWritingMode() && parent->isHorizontalWritingMode() && child->style().isFlippedLinesWritingMode() && !parent->style().isFlippedBlocksWritingMode()) > + return true; > + return false; Instead of if (cond) return true; return false; this could just be return cond; > Source/WebCore/rendering/RenderBox.cpp:3598 > staticPosition += renderBox.logicalLeft(); Missing indentation. Created attachment 428387 [details]
Patch
Created attachment 428389 [details]
Patch
Comment on attachment 428389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428389&action=review Looking better, I have a few other comments. > Source/WebCore/rendering/RenderBox.cpp:3564 > +static inline bool childIsVLRParentInHTB(const RenderBox* child) I think the name should be isVLRChildInHTBParent() Also we know that the child parameter cannot be nullptr, so use a reference instead of a pointer. Last but not least, I think we should pass a reference to the parent as well, that way the caller is responsible of providing non null values > Source/WebCore/rendering/RenderBox.cpp:3568 > +} supernit: it might be easier to understand what this condition is doing if we put the child checks first and the two parent ones later (that way we'd be matching the method's name). > Source/WebCore/rendering/RenderBox.cpp:3604 > + staticPosition += renderBox.logicalLeft(); We can use a ternary operator here staticPosition += isVLRChildInHTBParent(child) ? renderBox.logicalTop() : renderBox.logicalLeft() > Source/WebCore/rendering/RenderBox.cpp:4017 > + TextDirection parentDirection = parent->style().direction(); We only use this to compare to TextDirection::LTR. Maybe we can just do bool isParentDirectionLTR = parent->style().direction() == TextDirection::LTR; > Source/WebCore/rendering/RenderBox.cpp:4033 > + staticLogicalTop += renderBox.logicalTop(); Same here with regard to using a ternary operator Created attachment 428477 [details]
Patch
Created attachment 428515 [details]
Patch
Committed r277497 (237729@main): <https://commits.webkit.org/237729@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428515 [details]. |