Scrollbar appears for a moment in the new Awesomebar Resultlist

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Address Bar
P1
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: Mehmet, Assigned: mak)

Tracking

(Blocks: 2 bugs)

48 Branch
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8741829 [details]
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.
(Reporter)

Updated

2 years ago
Blocks: 1262507
OS: Unspecified → Mac OS X
(Assignee)

Comment 1

2 years ago
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]
(Reporter)

Updated

2 years ago
Component: Untriaged → Location Bar

Comment 2

2 years ago
Reproducible on Windows too

Updated

2 years ago
Duplicate of this bug: 1291950

Updated

2 years ago
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

Comment 4

2 years ago
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.

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
(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?
(Assignee)

Updated

a year ago
Priority: P2 → P3

Comment 7

a year ago
Could this be taken into account for Photon perceived performance ?
Flags: needinfo?(mak77)
(Assignee)

Comment 8

a year ago
yep.
Blocks: 1348289
Flags: needinfo?(mak77)
Whiteboard: [fxsearch] → [fxsearch][photon-performance]
Priority: P3 → P2
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu

Updated

a year ago
Blocks: 1363757
Priority: P2 → P3
Whiteboard: [fxsearch][photon-performance] → [fxsearch] [reserve-photon-performance]
(Assignee)

Updated

a year ago
Priority: P3 → P2
(Assignee)

Comment 9

a year ago
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 ;-)).
(Assignee)

Comment 11

a year ago
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.
(Assignee)

Comment 14

a year ago
(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]
(Assignee)

Comment 15

10 months ago
I may look at this next week
Assignee: nobody → mak77
Status: NEW → ASSIGNED

Updated

10 months ago
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 17

10 months ago
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 18

10 months ago
mozreview-review
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+
(Assignee)

Comment 19

10 months ago
mozreview-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.
Comment hidden (mozreview-request)

Comment 21

10 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/07052313bf75
Scrollbar flickers in the Awesomebar results list. r=Paolo

Comment 22

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/07052313bf75
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

10 months ago
Iteration: --- → 56.4 - Aug 1
Verified fix: 56.0a1 20170801100311 - Ubuntu 16.04, Windows 10 x64, Mac OSX 10.12.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.