224607 - Dynamic change of CSS 'display' does not repaint view correctly

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[Mats Palmgren (inactive)]

Dynamic change of CSS 'display' does not repaint view correctly

STEPS TO REPRODUCE:
1. load http://www.trionica.com
2. click 'Links' in left nav bar (this should take you to the URL)
3. click 'Expand All'

ACTUAL RESULTS:
The old area where the headings used to be is not cleared properly and is
obscuring newly exposed content.  See first screenshot.

EXPECTED RESULTS:
After forcing a repaint by covering the window the view is correct (reflow not
needed).  See second screenshot.

BUILDS & PLATFORMS TESTED:
Bug occurs in Mozilla nightly trunk build 2003-11-03-05 on Linux
Bug occurs in Mozilla 1.5 on Linux
Bug does NOT occur in Mozilla 1.4 on Linux

Comment 1

[Mats Palmgren (inactive)]

Attachment: Screenshot #1 - bug

Comment 2

[Mats Palmgren (inactive)]

Attachment: Screenshot #2 - correct

Comment 3

[Mats Palmgren (inactive)]

Attachment: Standalone markup

Comment 4

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

Mats, can you get someone to minimize that testcase? :-)

Comment 5

[Boris Zbarsky [:bzbarsky]]

This broke between build 2003-08-18-05 and build 2003-08-19-22.  Looks like more
fallout from bug 194638 (backing out that patch fixes this bug)

Comment 6

[Boris Zbarsky [:bzbarsky]]

See also bug 220698

Comment 7

[Boris Zbarsky [:bzbarsky]]

I bet the problem is that when the Resize() call is made on the nsWindow, it
clobbers the existing mUpdateArea.  In particular, note that if and only if
aRepaint is true the mUpdateArea is stomped on with the new window area.  If
there was something under the old area it won't get repainted, looks like...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: This fixes things

Patch fixes this bug and bug 220698.  I'm not sure whether this is the right
thing to do -- should the window be keeping track of its pre-resize
mUpdateArea?  Or should something else be handling the invalidation of that on
window resize?

Comment 9

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

I think this should just be setting the update area to (0, 0, width, height).
The mUpdateArea seems to be relative to the window origin. Has this been broken
always?

Comment 10

[Christopher Blizzard (:blizzard)]

It's been like that for years, yeah.  I suspect for legitimate reasons, though.

Comment 11

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

I say we change it to (0,0). It's just not consistent with the rest of the code.

Comment 12

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

Comment on attachment 134725 [details] [diff] [review]
This fixes things

see my comments in the bug

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Yeah, this fixes things too

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134868 [details] [diff] [review]
Yeah, this fixes things too

sr=me (since this is roc's patch, really).  blizzard, can you think of any
reason _not_ to make this change?

Comment 15

[Christopher Blizzard (:blizzard)]

Comment on attachment 134868 [details] [diff] [review]
Yeah, this fixes things too

r=blizzard

I looked through the code and I don't think it's going to break anything.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Taking to make sure this lands.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Back to roc, since it's his patch.

Comment 18

[Boris Zbarsky [:bzbarsky]]

And checked in.