331880 - [FIX]nsStackLayout::Layout(..) 's redraw logic is not entirely correct.

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Lih Chen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

Inside nsStackLayout::Layout(nsIBox* aBox, nsBoxLayoutState &aState) method in layout/xul/base/src/nsStackLayout.cpp, the box is redrawn if and only if childRect's top coordinate(y) is not equal to its original top position or the similar discrepancy appears in it's left coordinate(x). 

If a box is resized in the S, SE, or E direction, the affected region is not invalidated; hence, you will see a trail of the resizing activity.

My Fix: 
change the line 
if(childRect.x != oldRect.x || childRect.y != oldRect.y) 
to
if(childRect!=oldRect)

I would love to disclose my xul/js source that would reproduce this problem everytime. However, this is an enterprise sw. 


Reproducible: Always

Steps to Reproduce:
1. Write a xul document with a stack. Inside the stack, have a box. 
2. Inside a js associated with a stack, capture mousedown, mousemove, mouseup event for moving and resize of a box.
3. drag to resize the south or east edge of the box and you shall see.

Actual Results:  
You will see the trace of your resizing activity if you reduce the size of the box. 

Expected Results:  
If you resize the box in the north direction, you will see that gecko invalidate the affected region correctly. We should expect the same for all the other edges as well.

I just download the most recent source tarball off  from ftp://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/latest-trunk/. I am still seeing the same line of code, though I haven't recompile the source and test against that source. 

Free feel to contact me at [e-mail]. Thank you.

Comment 1

[Martijn Wargers (dead)]

Could you attach a minimal testcase that shows the bug?

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

Testcase that shows the bug, you should see no red in the testcase.
Thanks for showing me the extension, Lih. That allowed me to create this testcase.

Comment 3

[Martijn Wargers (dead)]

This is a regression between 2004-05-20 and 2004-05-22:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-05-20+14&maxdate=2004-05-22+11&cvsroot=%2Fcvsroot

I very much suspect bug 243882 to be the culprit.

Comment 4

[Martijn Wargers (dead)]

Sorry, mid-air collision at work.

Comment 5

[Boris Zbarsky [:bzbarsky]]

roc, it looks liks nsSprocketLayout calls SetBounds() in a bunch of places but never calls Invalidate().  And nsBox::SetBounds() also doesn't invalidate.

If I recall correctly, invalidating a child that gets resized  (or rather the difference between new and old areas) is the responsibility of the thing doing the resizing, right?

Comment 6

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

yes.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed, and on branch.

Comment 9

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

I'm a little surprised that this added 174 bytes of codesize on prometheus; that seems like a lot for such a small patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Two things got added, basically:

  childRectNoMargin = childRect

which ends up compiler-generated; presumably 4 assignments inlined?

  childRectNoMargin != oldRect

instead of two compares.  This inlines the following:

138   PRBool  operator==(const nsRect& aRect) const {
139     return (PRBool) ((IsEmpty() && aRect.IsEmpty()) ||
140                      ((x == aRect.x) && (y == aRect.y) &&
141                       (width == aRect.width) && (height == aRect.height)));
142   }

(with a "!" tossed on the whole thing).  IsEmpty() is also inlined.  So instead of two compares we now have 8, plus more logic connecting them....

Not sure whether that's enough to give that big an increase, of course.

Comment 11

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

Might be interesting to try un-inlining nsRect::== globally to see what that does to codesize.