363247 - absolutely positioned table does not reflow properly after stylesheet change

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Andrew Schultz]

With linux seamonkey build 2006-12-08-10-trunk, a table with position:absolute does not reflow properly after a change in the stylesheet.  Changing the font size via the DOM directly (style.fontSize in the testcase I'll attach) does not cause any problems.

Comment 1

[Andrew Schultz]

Attachment: testcase

With current trunk the table cells to not resize after the font size change, so "foo" and "bar" overlap.

Comment 2

[Bernd]

This was already wrong before the reflow branch landed

Comment 3

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

This is a bug in our enqueueing of style change reflows; we need to do the same work for out-of-flows outside the subtree.

Comment 4

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

Attachment: patch

This fixes this bug; I'd like to look into why it doesn't fix bug 476357 a drop more before requesting review.

And actually I might want to make it a loop rather than recursive...

Comment 5

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

Attachment: patch

Comment 6

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

Attachment: diff -w of nsPresShell.cpp

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 360402 [details] [diff] [review]
patch

>+++ nsPresShell.cpp    2009-02-03 16:11:12.000000000 -0800

>+  nsTArray<nsIFrame*> subtrees;

Maybe nsAutoTArray?  Of size at least 1, but maybe pick a random number that should work in "most" cases?

And change the other nsTArray (for the descendants) to nsAutoTArray too, maybe?

>+          nsPlaceholderFrame *ph = static_cast<nsPlaceholderFrame*>(f);
>+          nsIFrame *oof = ph->GetOutOfFlowFrame();

  nsIFrame *oof = nsPlaceholderFrame::GetRealFrameForPlaceholder(f);

r+sr=bzbarsky with that.

Comment 8

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

Comment on attachment 360402 [details] [diff] [review]
patch

This is a pretty nasty dynamic change handling bug that I'm surprised we haven't gotten more reports of (or maybe we have, and just haven't connected them to it).  I think we should get this on 1.9.1.

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/cbe86aabf921

Comment 10

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

So my inclination is to just land it on 1.9.1 either without the assertion, or with the assertion changed to a warning, given that:
 * we've hit the assertion in two separate bugs
 * the assertion is really only saying that the caller is doing something that didn't especially make sense, and as a result we're doing a little extra work or not quite enough

Comment 11

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

[2009-02-12 12:58:40] <dbaron> bz, does https://bugzilla.mozilla.org/show_bug.cgi?id=363247#c10 make sense to you?
[2009-02-12 12:59:56] <bz> dbaron: yes
[2009-02-12 13:00:07] <bz> dbaron: no assert on 1.9.1 sounds ok to me

Comment 12

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/be61f000938e

Comment 13

[Aakash Desai [:aakashd]]

verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090413 Minefield/3.6a1pre ID:20090413031052

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090413 Minefield/3.6a1pre ID:20090413031052

Comment 14

[Aakash Desai [:aakashd]]

ack, here's the build ID for Shiretoko Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090413 Shiretoko/3.5b4pre ID:20090413031313

Comment 15

[Daniel Veditz [:dveditz]]

dbaron: does this patch apply to 1.9.0? we need this to fix bug 430569

Comment 16

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

Attachment: 1.9.0 patch

It applies cleanly to CVS (the only merging required was the reftest.list), and fixes this bug in CVS (by testing attachment 248051 [details] with and without the patch).  I think it should be safe.

However, I can't reproduce bug 430569 even without the patch, so I can't tell if it fixes that on CVS.

Comment 17

[Samuel Sidler (old account; do not CC)]

Comment on attachment 393417 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.14. a=ss

Comment 18

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

Checked in to CVS HEAD for 1.9.0.14, 2009-08-10 10:29/30 -0700.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.14 using attached testcase and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.14pre) Gecko/2009081305 GranParadiso/3.0.14pre (.NET CLR 3.5.30729).