Closed Bug 415190 Opened 14 years ago Closed 13 years ago

Autocomplete results can be incorrectly sized (clipped)

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: stephend, Assigned: Mardak)

References

Details

(Whiteboard: [fixes bug 424868])

Attachments

(3 files, 3 obsolete files)

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?
Attached image Screenshot of problem
Seems like there's font height issues.
Duplicate of this bug: 424868
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..?
OS: Mac OS X → All
Hardware: PC → All
Attached patch v1 (obsolete) — Splinter Review
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?
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #312837 - Flags: review?(gavin.sharp)
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..
^^ This would make us only do 2 batches of 6 updates on a full result of 12 instead of 3 batches of 5 updates.
Blocks: 424868
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?
Priority: -- → P2
Whiteboard: [has patch][need review gavin][fixes bug 424868]
Attached patch v1.1 (obsolete) — Splinter Review
Doh. Should have been more careful when converting the code to call on a timeout.
Attachment #312837 - Attachment is obsolete: true
Attachment #312855 - Flags: review?(gavin.sharp)
Attachment #312837 - Flags: review?(gavin.sharp)
stephend: Use this build instead of the other one I linked earlier.

https://build.mozilla.org/tryserver-builds/2008-03-31_23:12-edward.lee@engineering.uiuc.edu-multi.select.download/

Some reason my original build worked correctly, but I guess I never qrefreshed the this/self change into my patch.
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?
Attached patch v1.2 (obsolete) — Splinter Review
(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.
Attachment #312855 - Attachment is obsolete: true
Attachment #312903 - Flags: review?(gavin.sharp)
Attachment #312855 - Flags: review?(gavin.sharp)
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?
(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.
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?
Attached patch v2Splinter Review
(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.
Attachment #312903 - Attachment is obsolete: true
Attachment #312949 - Flags: review?(gavin.sharp)
Attachment #312903 - Flags: review?(gavin.sharp)
Attachment #312949 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][need review gavin][fixes bug 424868] → [has patch][has review][fixes bug 424868]
(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...
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][fixes bug 424868] → [fixes bug 424868]
Target Milestone: --- → Firefox 3
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.)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.