Closed
Bug 1363621
Opened 8 years ago
Closed 7 years ago
Favicon in the first location bar suggestion flickers when typing url
Categories
(Toolkit :: Places, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox53 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | verified |
People
(Reporter: 684sigma, Assigned: mak)
References
Details
(Keywords: regression, Whiteboard: [fxsearch])
Attachments
(2 files, 1 obsolete file)
While testing Bug 1363620, I have noticed a bug in Firefox Nightly 55 (2017-05-09). It doesn't happen in Beta 53.
Favicon in the first location bar suggestion flickers when typing url.
Here's how to reproduce the bug:
1. Visit google.com
2. Open new tab, type in location bar "google.com/helloworld"
Result: Favicon in the first suggestion flickers
Expected: Favicon should be displayed without flickering
Mozregression-gui 0.9.6 generated this pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b01f44c896b2472b154a48c68cf343ae2289d6bd&tochange=d80c517658f5048491b7bf27da25246a297ab50b
->
977177 - move favicons blobs out of places.sqlite to their own database
https://bugzilla.mozilla.org/show_bug.cgi?id=977177
Updated•8 years ago
|
Component: Untriaged → Places
Product: Firefox → Toolkit
Assignee | ||
Comment 2•8 years ago
|
||
this is similar to bug 1360279, but here you're typing a url. it may be a bit more complex to solve because the url effectively changes. Though, we should know if the url is known or not.
It could be just matter of specifying a fixed icon in _matchUnknownUrl like we did in _processHostRow.
Blocks: PlacesHiresFavicons
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Whiteboard: [fxsearch]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 3•8 years ago
|
||
So, here we have an url that we don't know if it's known or not to history and figuring that out may be expensive. So we don't know if we have an icon for it.
The reason for the flickr is exactly that at every new typed char we try to fetch the icon for the newly typed url.
There are 2 ways we could "fix" this:
1. if the uri has an host, we could fetch the icon for the host. This would only partially solve the flicker since until we start typing the path, we'll still try to fetch an icon at every typed char. The advantage is that we could show some more icons.
2. just always use the globe icon. This won't flicker, but if we have the icon for a specific url it won't be shown.
I'm prone to do 2, to completely avoid the flicker, and it will also be cheaper (since the user is typing that matters). The likely to have one icon out of many is very low, so it should not be terrible from a UI point of view.
Comment hidden (mozreview-request) |
Comment 5•8 years ago
|
||
Does this patch cause us to show the globe icon for completions? (eg. when I type 'b' and the first awesomebar item says 'bugzilla.mozilla.org - Visit')
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #5)
> Does this patch cause us to show the globe icon for completions? (eg. when I
> type 'b' and the first awesomebar item says 'bugzilla.mozilla.org - Visit')
it shows the globe if what you are typing is _not_ autofilled and it looks like a url.
It doesn't touch autofilled entries (b[ugzilla.org] or bugzilla.org/s[omething]).
For you example, if we show bugzilla.mozilla.org it means it's autofilled, or we'd show "b - Search with Google".
Comment 7•8 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #6)
> For you example, if we show bugzilla.mozilla.org it means it's autofilled,
> or we'd show "b - Search with Google".
We autofill inside the input field only after I type "bu", when I only type "b", there's no autofill in the field, but the first awesomebar item offers to visit bugzilla. And actually... it turns out this is because I have 'b' as a keyword for bugzilla search, and pressing enter doesn't send me to the home page, but makes me search for an empty string in bugzilla; that's misleading :-/ (but probably off topic for this bug).
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #7)
> We autofill inside the input field only after I type "bu", when I only type
> "b", there's no autofill in the field, but the first awesomebar item offers
> to visit bugzilla. And actually... it turns out this is because I have 'b'
> as a keyword for bugzilla search, and pressing enter doesn't send me to the
> home page, but makes me search for an empty string in bugzilla; that's
> misleading :-/ (but probably off topic for this bug).
Ah that's bug 1124238, I should really find some time to properly test and land that.
Assignee | ||
Comment 9•7 years ago
|
||
Florian, do you need further answers for this review?
Comment 10•7 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #9)
> Florian, do you need further answers for this review?
Not really, the reason I haven't r+'ed yet is that I wanted to test the patch locally to see if I find cases where we lose the icon and it feels like an actual loss.
Assignee | ||
Comment 11•7 years ago
|
||
I don't think anyone will even notice this. You should basically type exactly a url that you have in history, that has a favicon and that is not just a host, at a minimum "somedomain/somethingelse" when you have it in history and it also has a favicon. and it's likely as soon as you have finished typing it you'll just Enter :)
Comment 12•7 years ago
|
||
Would it be reasonably easy to display the globe (without flickering) until the user types '/', and then search for an icon for the hostname before the '/'? (this is a compromise between the 2 solutions you proposed in comment 3).
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #12)
> Would it be reasonably easy to display the globe (without flickering) until
> the user types '/', and then search for an icon for the hostname before the
> '/'? (this is a compromise between the 2 solutions you proposed in comment
> 3).
Potentially it may be possible, we should look for a /, then look if the url has a host (with a try catch since urls without a host throw) finally use one icon or the other one.
It would be more work while the user is typing, with a not much higher probability to have the icon for the host. But maybe it will save some icons.
I'll try and see how it works, tomorrow probably.
Comment 14•7 years ago
|
||
There's already a check for the host a few lines above: http://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/toolkit/components/places/UnifiedComplete.js#1598
We could put hostExpected.has(uri.scheme) in a variable there, and always use the globe when it's false.
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #14)
> There's already a check for the host a few lines above:
> http://searchfox.org/mozilla-central/rev/
> 31eec6f59eb303adf3318058cb30bd6a883115a5/toolkit/components/places/
> UnifiedComplete.js#1598
>
> We could put hostExpected.has(uri.scheme) in a variable there, and always
> use the globe when it's false.
well yes, it would only work for those schemes, but probably we don't care.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8871299 [details]
Bug 1363621 - Favicon in the first location bar suggestion flickers when typing url.
https://reviewboard.mozilla.org/r/142786/#review148334
::: toolkit/components/places/UnifiedComplete.js:1603
(Diff revisions 1 - 2)
> // Check the host, as "http:///" is a valid nsIURI, but not useful to us.
> // But, some schemes are expected to have no host. So we check just against
> // schemes we know should have a host. This allows new schemes to be
> // implemented without us accidentally blocking access to them.
> let hostExpected = new Set(["http", "https", "ftp", "chrome"]);
> if (hostExpected.has(uri.scheme) && !uri.host)
I think here we should save the result of hostExpected.has(uri.scheme) in a variable.
And given the early return if !uri.host, we would need to check 'uri.host' again later.
::: toolkit/components/places/UnifiedComplete.js:1632
(Diff revisions 1 - 2)
> - // be expensive. Thus we also don't know if we may have an icon for it.
> + // be expensive. Thus we also don't know if we may have an icon.
> // If we'd just try to fetch the icon for the typed string, we'd cause icon
> - // flicker, since the string keeps growing while the user types. Even if
> - // we'd return the icon for the url host, we'd cause flicker until the user
> - // starts typing the url path.
> - // To sum up, we won't provide any icon and fallback to the default one.
> + // flicker, since the url keeps changing while the user types.
> + // By default we won't provide an icon, but for the subset of urls with a
> + // host we'll check for a typed slash and set favicon for the host part.
> + if (hostExpected && uri.host &&
hostExpected is a set, not a boolean.
Attachment #8871299 -
Flags: review?(florian) → review-
Assignee | ||
Comment 20•7 years ago
|
||
oh lol, I read this really wrong. To my excuse, the variable name is horrible.
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8871299 [details]
Bug 1363621 - Favicon in the first location bar suggestion flickers when typing url.
https://reviewboard.mozilla.org/r/142786/#review148350
Looks good to me (note: I haven't tested the patch locally).
Attachment #8871299 -
Flags: review?(florian) → review+
Assignee | ||
Comment 23•7 years ago
|
||
I did test the patch locally, it works fine (well the previous one was working too because the Set was always defined!).
There are some incoherencies here and there, like for example for some pages we have icon for the https version but not for the http one, so it may look strange that an entry in the popup has an icon and the other one doesn't, but that's not fault of this patch. We likely should just relax the favicons service to fetch more icons in more cases (even if the situation already improved compared to the previous store).
Comment 24•7 years ago
|
||
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/5df9b21d7a40
Favicon in the first location bar suggestion flickers when typing url. r=florian
Comment 25•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
status-firefox53:
--- → unaffected
status-firefox54:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Comment 26•7 years ago
|
||
Build ID: 20170612224034
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Verified as fixed on Firefox Beta 55.0b1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•