386391 - Old padding remains when updating bottom-positioned-div (gmail macros label selector)

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[Ed Lee :Mardak]

Attachment: minimized testcase

When dynamically changing the contents of a div that is "position: fixed/absolute" and based on "bottom:", the old padding covers the new content.

Attached is a minimal testcase based on the gmail macros greasemonkey script for choosing a label. (Pressing "g" to open the label selector and typing a label results in a parts of the text cut off - both while typing and when the full label is inserted.)

Comment 1

[Ed Lee :Mardak]

Attachment: screenshot of testcase (paste 12345 multiple times)

Comment 2

[Ed Lee :Mardak]

The padding covers up images as well (use <img src> in the testcase) -> component view rendering

More people (using greasemonkey + gmail macros) might be running into this bug as the beta approaches -> summary update, b1.9?

Comment 3

[Ed Lee :Mardak]

Attachment: screenshot: gmail macros label selector

The screenshot comes from typing "s" then "e" at which point the script will choose "Sent Mail", but the old padding was for displaying only "se".

For gmail macros users that want to avoid the problem, you can edit the script to position by top instead of bottom:

@@ -490,7 +490,7 @@ function getNodeSet() {
   with (boxNode.style) {
     display = "none";
     position = "fixed";
-    bottom = "20%";
+    top = "70%";
     left = "10%";
     margin = "0 10% 0 10%";
     width = "60%";

Comment 4

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

This is marked with the "regression" keyword.  When did it regress?

Comment 5

[Ed Lee :Mardak]

(In reply to comment #4)
> When did it regress?
Started showing up in 20070227 builds with bug 371536 being a likely candidate.

Fixing this might also fix bug 386395 which has a similar issue but with borders.

Comment 6

[Eli Friedman]

Attachment: Patch

Ugh, I really hate our invalidation model... I don't think we want to continue trying to smash the bugs in this for 1.9.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why does  nsAbsoluteContainingBlock::ReflowAbsoluteFrame need the aChildBounds parameter? the child's overflow area and rect are set correctly when ReflowAbsoluteFrame returns, so we can get them from the frame, right? Is this just simplifying the code to avoid AddFrameToChildBounds being needed?

-  } else if (oldRect.Size() != rect.Size()) {

Why are you eliminating this optimization? Is it unsafe?

Comment 8

[Eli Friedman]

Comment on attachment 272254 [details] [diff] [review]
Patch

Oops, I just realized my fix doesn't really even fix the problem :(  Real fix coming up.

Comment 9

[Eli Friedman]

(In reply to comment #7)
> Why does  nsAbsoluteContainingBlock::ReflowAbsoluteFrame need the aChildBounds
> parameter? the child's overflow area and rect are set correctly when
> ReflowAbsoluteFrame returns, so we can get them from the frame, right? Is this
> just simplifying the code to avoid AddFrameToChildBounds being needed?

It's a simplification, plus it avoids a call to GetOverflowRect() when we can get it off the reflow metrics. (The code currently looks up the overflow property for every frame.)

Comment 10

[Eli Friedman]

Attachment: Supplemental test

Test with a fixed width div, so the width, height, and overflow rect stay the same.

Comment 11

[Eli Friedman]

Attachment: Fixed patch

Blech, I'm not sure how this code managed to stick around so long.  It's been unnecessary for about seven years now...

Comment 12

[Eli Friedman]

Checked in.  (We don't have any automated invalidation tests at the moment.)

Comment 13

[Ed Lee :Mardak]

v. w/ minimized testcase. Thanks!