388374 - [FIXr]"ASSERTION: How did our kid's height change if nothing was dirty?" with tables, padding, <select>

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers:

###!!! ASSERTION: How did our kid's height change if nothing was dirty?: 'oldVisibleHeight == GetScrolledFrame()->GetSize().height', file /Users/jruderman/trunk/mozilla/layout/forms/nsListControlFrame.cpp, line 665

The testcase is fragile: changing the amount of text in the testcase affects whether the assertion is triggered :(

Comment 1

[Boris Zbarsky [:bzbarsky]]

Might depend on the exact fonts too...  I can't reproduce.  :(

Comment 2

[Jesse Ruderman]

Attachment: testcase without text

Can you reproduce with this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Indeed, I can.

David, the issue here is just that heights do depend on widths, so if the select width changed we should be prepared for the heights of the options to change.  In this case that's due to percentage vertical padding, but allowing options to wrap would also have this effect, I bet.  It's really a silly thing to have overlooked...

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 273034 [details] [diff] [review]
Proposed fix

r+sr=dbaron.  (Seems like checking ShouldReflowAllKids() rather than just mHResize might be useful for some vertical resize cases, although I can't actually think of any that can happen when height is auto.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273034 [details] [diff] [review]
Proposed fix

Make the optimization not be so optimized that it optimizes away needed reflows.  No correctness risk, and I doubt this will affect performance much, if at all.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 273034 [details] [diff] [review]
Proposed fix

a19=dbaron

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 8

[Jesse Ruderman]

Crashtests checked in.