Closed Bug 1264988 Opened 8 years ago Closed 7 years ago

Scrollbar appears for a moment in the new Awesomebar Resultlist

Categories

(Firefox :: Address Bar, defect, P1)

48 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.4 - Aug 1
Tracking Status
firefox56 --- verified

People

(Reporter: mehmet.sahin, Assigned: mak)

References

Details

(Whiteboard: [fxsearch] [photon-performance])

Attachments

(2 files)

Attached video Bug.mov
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160415030231

Steps to reproduce:

Firefox Nightly 48.0a1 (2016-04-15)
Mac OS 10.11.4

STR:

1.) type something into the Location Bar, so that only one item is to see in the Resultlist
2.) Delete some letters quickly


Actual results:

The scrolbar appears for a moment.


Expected results:

The scrollbar should not appear.
Blocks: 1262507
OS: Unspecified → Mac OS X
yep, that's a little bit noisy... We may maybe use overflow: hidden... though users could still set browser.urlbar.maxRichResults and we should not break their urlbar.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Whiteboard: [fxsearch]
Component: Untriaged → Location Bar
Reproducible on Windows too
OS: Mac OS X → All
Hardware: Unspecified → All
Summary: [OSX] Scrollbar appears for a moment in the new Awesomebar Resultlist → Scrollbar appears for a moment in the new Awesomebar Resultlist
Marco's comment about users with a different value for browser.urlbar.maxRichResults is alluding to values which are high enough that the results can extend beyond the results pane's max-height.

Perhaps the solution is to set overflow: hidden during the resize and unset it afterwords.
In linux (Fedora) also happens. But the scrollbar it is permanent if the result box it is on his max size 
hiding a couple of items.
(In reply to jorrete from comment #5)
> In linux (Fedora) also happens. But the scrollbar it is permanent if the
> result box it is on his max size 
> hiding a couple of items.

did you change browser.urlbar.maxRichResults ?
Which version of Firefox?
Priority: P2 → P3
Could this be taken into account for Photon perceived performance ?
Flags: needinfo?(mak77)
yep.
Flags: needinfo?(mak77)
Whiteboard: [fxsearch] → [fxsearch][photon-performance]
Priority: P3 → P2
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
Priority: P2 → P3
Whiteboard: [fxsearch][photon-performance] → [fxsearch] [reserve-photon-performance]
Priority: P3 → P2
oh hm, sorry I didn't see Marco changed the priority.
Priority: P2 → P3
(In reply to Marco Bonardo [::mak] from comment #9)
> oh hm, sorry I didn't see Marco changed the priority.

Marco changed the priority (along with the priority of lots of other photon perf bugs) because we decided this bug goes on the 'reserve' for photon-perf. The reason for putting the bug in the photon reserve in this case is that we expect the fxsearch team to handle it... so feel free to set the priority to whatever makes sense for you (especially if there are chances you could fix it, which I would really appreciate ;-)).
Ok, I didn't want to cause issues to your tracking, I think we should fix this for the speed perception (and showing the scrollbar may even have a cost).
Priority: P3 → P2
(In reply to Marco Bonardo [::mak] from comment #11)

> (and showing the scrollbar may even have a cost).

Unless I missed a newer patch, we added a fast code path that skips a sync reflow only when there's no scrollbar, because we assumed it was always the case for the awesomebar... so I'm afraid the cost here is a sync reflow.
But the flickering perception is enough for this to be worth fixing anyway.
(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #12)
> Unless I missed a newer patch, we added a fast code path that skips a sync
> reflow only when there's no scrollbar, because we assumed it was always the
> case for the awesomebar... so I'm afraid the cost here is a sync reflow.

No, what was causing the reflow was a call to ensureItemIsVisible, and to avoid that we checked if we could never have a scrollbar in a theoretical way, by comparing maximum number of entries. Since we know that the urlbar doesn't want a scrollbar we always skip the reflow because we don't care.
Having a scrollbar or not *in reality* doesn't have any effect on that, the cost I was speaking about is that layout may have to do more computation to show a scrollbar rather than an overflow:hidden frame (but I don't know if that's true).
Whiteboard: [fxsearch] [reserve-photon-performance] → [fxsearch] [photon-performance]
I may look at this next week
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Priority: P2 → P1
there is another alternative, that is to tamporarily set overflow hidden when we modify the contents, and restore to auto once done. It should be done somewhere between _appendCurrentResult and adjustHeight in autocomplete.xml to completely avoid the scrollbar. I didn't try since it sounds far more work and complication for a small benefit of a single consumer. If we'd ever find the problem is also annoying in other consumers of the richlistbox autocomplete, we may have to look into that.
Comment on attachment 8889407 [details]
Bug 1264988 - Scrollbar flickers in the Awesomebar results list.

https://reviewboard.mozilla.org/r/160440/#review165724

A solution specific to this popup sounds good to me.

::: browser/base/content/urlbarBindings.xml:187
(Diff revision 1)
>  
>        <!--
> +        Since we never want scrollbars, we always use the maxResults value.
> +      -->
> +      <property name="maxRows"
> +                onget="return this.popup.maxResults || Services.prefs.getIntPref(`browser.urlbar.maxRichResults`);"/>

Doesn't popup.maxResults always have a value?
Attachment #8889407 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8889407 [details]
Bug 1264988 - Scrollbar flickers in the Awesomebar results list.

https://reviewboard.mozilla.org/r/160440/#review165784

::: browser/base/content/urlbarBindings.xml:187
(Diff revision 1)
>  
>        <!--
> +        Since we never want scrollbars, we always use the maxResults value.
> +      -->
> +      <property name="maxRows"
> +                onget="return this.popup.maxResults || Services.prefs.getIntPref(`browser.urlbar.maxRichResults`);"/>

Yes, it was just an overzealous check, but actually I should have checked .popup. It likely doesn't matter so I just removed the fallback.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/07052313bf75
Scrollbar flickers in the Awesomebar results list. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/07052313bf75
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Iteration: --- → 56.4 - Aug 1
Verified fix: 56.0a1 20170801100311 - Ubuntu 16.04, Windows 10 x64, Mac OSX 10.12.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.