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

VERIFIED FIXED

Status

()

Core
Layout: View Rendering
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Mardak, Assigned: Eli Friedman)

Tracking

({regression})

Trunk
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
Created attachment 270399 [details]
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.)
(Reporter)

Comment 1

11 years ago
Created attachment 270400 [details]
screenshot of testcase (paste 12345 multiple times)
(Reporter)

Comment 2

11 years ago
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?
Assignee: nobody → roc
Component: Layout: Fonts and Text → Layout: View Rendering
Flags: blocking1.9?
QA Contact: layout.fonts-and-text → ian
Summary: Old padding covers new text when updating a div removed from flow and positioned from bottom → Old padding remains when updating bottom-positioned-div (gmail macros label selector)
(Reporter)

Comment 3

11 years ago
Created attachment 271229 [details]
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%";
This is marked with the "regression" keyword.  When did it regress?
Flags: blocking1.9? → blocking1.9+
(Reporter)

Comment 5

11 years ago
(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.
Blocks: 371536
(Assignee)

Comment 6

11 years ago
Created attachment 272254 [details] [diff] [review]
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.
Assignee: roc → sharparrow1
Status: NEW → ASSIGNED
Attachment #272254 - Flags: review?(roc)
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?
(Assignee)

Comment 8

11 years ago
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.
Attachment #272254 - Attachment is obsolete: true
Attachment #272254 - Flags: review?(roc)
(Assignee)

Comment 9

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

Comment 10

11 years ago
Created attachment 272448 [details]
Supplemental test

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

Comment 11

11 years ago
Created attachment 272452 [details] [diff] [review]
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...
Attachment #272452 - Flags: review?(roc)
Attachment #272452 - Flags: superreview+
Attachment #272452 - Flags: review?(roc)
Attachment #272452 - Flags: review+
(Assignee)

Comment 12

11 years ago
Checked in.  (We don't have any automated invalidation tests at the moment.)
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Blocks: 386395
(Reporter)

Comment 13

11 years ago
v. w/ minimized testcase. Thanks!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.