Closed Bug 820111 Opened 7 years ago Closed 7 years ago

Flexbox: Changing an element's box-shadow shrinks flex items with width:/height: auto

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: shorlander, Assigned: dholbert)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached file Example
Changing the box-shadow on an element shrinks the width or height of flex elements set to auto. It doesn't seem to matter if the item changed is inside or outside of a container with display: flex. 

Although its position does seem to affect whether it is the width or the height of various items is changed.

Attaching an example.
Thanks! I can repro in linux nightly. Taking & investigating.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
After you've hovered & un-hovered, you can restore the original layout by resizing the window horizontally.

I think we probably need to mark more things dirty, or something.
So this affects items with width/height:auto because those are the ones that we stretch in the cross axis.

The bug here is that we're failing to stretch those items in some cases.

Looks like it's because our "measuring" reflow (w/ an un-stretched cross-size) on a flex item clears the dirty bits on all its descendants. Then when we do the "actual" reflow, we skip reflowing some of the descendants, because they're no longer dirty.  So they're left with the size that they ended up with during the "measuring" reflow. (which wasn't stretched)
Attached file testcase 2 (obsolete) —
Here's a somewhat reduced testcase.  Expected behavior: hovering the "hover me" block should show a shadow, but have no other effects.
Attached patch wip (obsolete) — Splinter Review
Here's a WIP patch for this. It fixes "testcase 2", but it doesn't completely fix the original testcase ("Example").

Here's what's still broken in the original testcase:
 - If I hover the button, the first two pink blocks no longer paint. (but no positioning is changed)
 - If I resize the page vertically, a bunch of stuff collapses, as with the original hover issue. (true w/ or w/out the patch)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
I don't think we actually want to be forcibly setting mHResize -- we want to have it already set correctly, up-front.

Right now, it's _not_ set (which is why the wip helps).  This is because, when we do the "real" reflow of our item, we see that our stretch-imposed mComputedWidth is the same as what the item's mRect.width (from the last "real" reflow).  So for the item itself, it's correct that we're not doing a horizontal resize.  But from the perspective of its kids, we *are* doing a horizontal resize, because we did a "measuring" reflow with a smaller width, and the kids updated their own mRects according to that.  (We don't update the item's own mRect for that measuring reflow, because we know it's not a "real" reflow and we're trying to save some work, so we call DidReflow() directly instead of calling FinishReflowChild() on the flex item.  DidReflow() doesn't update mRect)

I think we can fix this by calling FinishReflowChild() instead of trying to skip work with the (more lightweight) DidReflow() call. As this bug demonstrates, we get into trouble when a flex-item's kids update their own sizes during the "measuring" reflow, without the item itself updating its size.
Summary: Flexbox: Changing an elements box-shadow shrinks flex items with width:/height: auto → Flexbox: Changing an element's box-shadow shrinks flex items with width:/height: auto
Attached patch part 1: call FinishReflowChild (obsolete) — Splinter Review
(In reply to Daniel Holbert [:dholbert] from comment #6)
> I think we can fix this by calling FinishReflowChild() instead of trying to
> skip work with the (more lightweight) DidReflow() call.

This patch makes us call FinishReflowChild instead of DidReflow.  This fixes testcase 2 but doesn't completely fix the original attachment (similar to my earlier WIP).
Attachment #690672 - Attachment is obsolete: true
Depends on: 821426
Comment on attachment 691159 [details] [diff] [review]
part 1: call FinishReflowChild

I ended up splitting "testcase 2", & its fix, "part 1", off into a helper-bug: bug 821426.

I may split off another helper-bug or two, to address the remaining issues with the original testcase here.
Attachment #691159 - Attachment is obsolete: true
Attachment #690670 - Attachment is obsolete: true
Depends on: 821449
Helper bug 821426 and bug 821449 are now both fixed, which fix all of the issues in this bug's testcase when the button is hovered (i.e. when box-shadow is set).

 --> Resolving as FIXED.

(Note that bug 821775 tracks a different issue in this bug's testcase, which happens when the window is resized.)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.