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

Status: RESOLVED FIXED

Description

[Stephen Horlander [:shorlander]]

Created attachment 690525 [details]
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.

Comment 1

[Daniel Holbert [:dholbert]]

Thanks! I can repro in linux nightly. Taking & investigating.

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Daniel Holbert [:dholbert]]

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)

Comment 4

[Daniel Holbert [:dholbert]]

Created attachment 690670 [details]
testcase 2

Here's a somewhat reduced testcase.  Expected behavior: hovering the "hover me" block should show a shadow, but have no other effects.

Comment 5

[Daniel Holbert [:dholbert]]

Created attachment 690672 [details] [diff] [review]
wip

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)

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Daniel Holbert [:dholbert]]

Created attachment 691159 [details] [diff] [review]
part 1: call FinishReflowChild

(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).

Comment 8

[Daniel Holbert [:dholbert]]

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.

Comment 9

[Daniel Holbert [:dholbert]]

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.)