424251 - pageproxystate set to valid when selecting an autocomplete result

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Simon Bünzli]

Steps to Reproduce:
1. Enable browser.urlbar.autoFill
2. Start entering h t t ...

Actual result:
As soon as a URL is auto-filled, the current page's favicon is displayed again (opposed to the faded out page indicating non-validated user input).

Expected result:
Don't display any favicon until you hit Go/Enter or Esc (and if a favicon was to be displayed, it should be the one which is also displayed for the auto-filled suggestion, not the currently visible page's).

Comment 1

[Simon Bünzli]

This regressed between the 2008031204 and the 2008031303 builds -> bug 419656.

Comment 2

[Simon Bünzli]

Attachment: minimal fix

This minimal change would fix it...

Comment 3

[Ed Lee :Mardak]

Yup, bug 419656 was the one that added aMustUseURI and uses it.

Comment 4

[Simon Bünzli]

Comment on attachment 310883 [details] [diff] [review]
minimal fix

Is this the right fix, then?

Comment 5

[Ed Lee :Mardak]

Comment on attachment 310883 [details] [diff] [review]
minimal fix

>+++ browser/base/content/browser.js
>@@ -1991,17 +1991,18 @@ function URLBarSetURI(aURI, aMustUseURI)
>-      state = "valid";
>+      if (!aMustUseURI)
>+        state = "valid";
That would fix the issue. Perhaps comment why it shouldn't be valid when forced but is okay when not forced. Potentially someone would want the forced uri to be valid? gavin?

Should we check instead..
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.1008&mark=1953,1957,1999#1953

- if (!value || aMustUseURI) {
+ if (!value || aMustUseURI != null)
treating aMustUseURI as a tri-state null(undefined), true, false?

Comment 6

[Ed Lee :Mardak]

Attachment: v1

Here's a version if we want to allow the forced uri to cause a valid proxy state.

Comment 7

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

Hrm, I'm kind of regretting using URLBarSetURI for bug 419656. I was worried about inconsistencies between the various ways we unescape URIs, but maybe misusing URLBarSetURI for setting the value for non-loads is worse.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #0)
You don't need autofill for this. Just arrow through the list.

(In reply to comment #7)
> I was worried about inconsistencies between the various ways we unescape URIs

That could be solved by letting a helper function do the unescaping.

Comment 9

[Dão Gottwald [:dao]]

Attachment: use a helper function

Comment 10

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

Comment on attachment 311121 [details] [diff] [review]
use a helper function

Yeah this is better, thanks!

Comment 11

[Mike Connor [:mconnor]]

Not a blocker, autofill is a hidden pref that is unsupported.

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> Not a blocker, autofill is a hidden pref that is unsupported.

see comment 8:
> You don't need autofill for this. Just arrow through the list.

Comment 13

[Reed Loden [:reed]]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.1013; previous revision: 1.1012
done
Checking in browser/base/content/urlbarBindings.xml;
/cvsroot/mozilla/browser/base/content/urlbarBindings.xml,v  <--  urlbarBindings.xml
new revision: 1.58; previous revision: 1.57
done

Comment 14

[Tracy Walker [:tracy]]

edited test cases https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=8858 to include this expected behavior.