415190 - Autocomplete results can be incorrectly sized (clipped)

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Marcia's seen this twice, now; sometimes she'll wrongly sized (clipped) autocomplete results in the awesomebar.

I'll attach a screenshot and her history.sqlite; we haven't yet been able to get a good repro case.

I have her places.sqlite and history.sqlite files; which should I attach, if any?

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Screenshot of clipped autocomplete results

Comment 2

[Ed Lee :Mardak]

Attachment: Screenshot of problem

Seems like there's font height issues.

Comment 4

[Ed Lee :Mardak]

Some reason the richlistitem's boxobject height is reporting the wrong value..? Perhaps related to why we need a timeout to update the ellipsis because the text doesn't update right away.

So the title starts out empty and gets filled in, but by then it already grabbed the height of the row..?

Comment 5

[Ed Lee :Mardak]

Attachment: v1

Compute the height by taking the y position of the first row and last row shown as well as setting the height on a timeout of 0.

This removes the existing code that computes the height by first row's height * numRows that executes before items are added.

This would make sure the heights are computed /after/ the texts have been updated.

But potentially we will show the old number of rows momentarily when there's fewer rows after an invalidate -- i.e., starting a new search and the timeout hasn't updated the height. Perhaps we should update the height on call and on timeout0?

Comment 6

[Ed Lee :Mardak]

Oh, I suppose on a completely unrelated note, perhaps we should change the richlistitem chunksize to 3 or 6 now that we show 6 visible and at most 12..

Comment 7

[Ed Lee :Mardak]

^^ This would make us only do 2 batches of 6 updates on a full result of 12 instead of 3 batches of 5 updates.

Comment 8

[Ed Lee :Mardak]

Preemptively marking as fixing blocking bug 424868.

stephend: Can you reproduce this issue consistently? If so, can you try the build from bug 424868 comment 9?

Comment 9

[Ed Lee :Mardak]

Attachment: v1.1

Doh. Should have been more careful when converting the code to call on a timeout.

Comment 10

[Ed Lee :Mardak]

stephend: Use this build instead of the other one I linked earlier.

https://build.mozilla.org/tryserver-builds/2008-03-31_23:[e-mail]/

Some reason my original build worked correctly, but I guess I never qrefreshed the this/self change into my patch.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 312855 [details] [diff] [review]
v1.1

>diff --git a/toolkit/content/widgets/autocomplete.xml 

>-          // until we have support for "rows" on richlistbox,
>-          // determine the height dynamically.  (see bug #401939)

Keep this comment?

>+            // Grab the last row that we can show (which might be nothing)
>+            let lastRow = self.richlistbox.childNodes.length >= rows ?
>+              self.richlistbox.childNodes[rows - 1] : self.richlistbox.lastChild;

Won't this throw NS_ERROR_DOM_INDEX_SIZE_ERR if rows and childNodes.length are both 0?

Comment 12

[Ed Lee :Mardak]

Attachment: v1.2

(In reply to comment #11)
> >-          // until we have support for "rows" on richlistbox,
> >-          // determine the height dynamically.  (see bug #401939)
> Keep this comment?
Undeleted.

> >+            // Grab the last row that we can show (which might be nothing)
> >+            let lastRow = self.richlistbox.childNodes.length >= rows ?
> >+              self.richlistbox.childNodes[rows - 1] : self.richlistbox.lastChild;
> Won't this throw NS_ERROR_DOM_INDEX_SIZE_ERR if rows and childNodes.length are
> both 0?
Ah indeed, or whenever rows is 0.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 312903 [details] [diff] [review]
v1.2

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets

>+            let lastRow = rows && self.richlistbox.childNodes.length >= rows ?
>+              self.richlistbox.childNodes[rows - 1] : self.richlistbox.lastChild;
>+
>+            // Set the height to have the first row to the last row shown
>+            self.richlistbox.height = !lastRow ? 0 : lastRow.boxObject.y +
>+              lastRow.boxObject.height - self.richlistbox.firstChild.boxObject.y;

This depends on the fact that lastChild is null if rows==0? That wouldn't be the case if someone adjusts maxRows between search, would it?

Comment 14

[Ed Lee :Mardak]

(In reply to comment #13)
> >+            // Set the height to have the first row to the last row shown
> >+            self.richlistbox.height = !lastRow ? 0 : lastRow.boxObject.y +
> >+              lastRow.boxObject.height - self.richlistbox.firstChild.boxObject.y;
> This depends on the fact that lastChild is null if rows==0? That wouldn't be
> the case if someone adjusts maxRows between search, would it?
All the rows are collapsed when it's invalidated, and that causes the rows to have a height of 0 with the same y value.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

I'm saying that if maxRows is 0, but lastChild is non-null (which could happen if maxRows was previously non-zero), then we'll set the richlistbox height to something non-zero. What am I misunderstanding?

Comment 16

[Ed Lee :Mardak]

Attachment: v2

(In reply to comment #15)
> I'm saying that if maxRows is 0, but lastChild is non-null (which could happen
> if maxRows was previously non-zero), then we'll set the richlistbox height to
> something non-zero.
Ah, I see. We can just take the min of match count, max rows, num rows and if any is 0, we set the height to 0.

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #8)
> Preemptively marking as fixing blocking bug 424868.
> 
> stephend: Can you reproduce this issue consistently? If so, can you try the
> build from bug 424868 comment 9?

No, I can't, which is why I was hoping Reed would comment, since he "just started seeing it recently," and perhaps he sees it now...

Comment 18

[Ed Lee :Mardak]

Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.134; previous revision: 1.133
done

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre)
Gecko/2008050804 Minefield/3.0pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050806
Minefield/3.0pre

-and-

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008050804
Minefield/3.0pre

(I've asked around and tested quite a bit-none of us has seen this since.)