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

RESOLVED FIXED

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: shorlander, Assigned: dholbert)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Thanks! I can repro in linux nightly. Taking & investigating.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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)
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 7

6 years ago
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).
Attachment #690672 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 821426
(Assignee)

Comment 8

6 years ago
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
(Assignee)

Updated

6 years ago
Attachment #690670 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 821449
(Assignee)

Comment 9

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.