1120236 - Can't Click on Form Autofill Entry If Search Bar is Removed

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[rdz_bug]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150111030201

Steps to reproduce:

1. Open the Customize Firefox UI
2. Drag the Search Bar to the "Additional Tools and Features" area
3. Exit Customize
4. Open https://bugzilla.mozilla.org
5. Double click the search field at the top
6. Click on any entry


Actual results:

Nothing


Expected results:

Search field should be filled with the selected entry

Video: http://screencast-o-matic.com/watch/coVXqvewzc

Started with: https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=206205dd8bd1

Comment 1

[B.J. Herbison]

Seen in Linux as well.

Easy to test, no restart is needed. The behavior changes as soon as the search bar is added or removed from the toolbars.

Comment 2

[:Gijs (he/him)]

There should be a null check here, at a minimum, before accessing |searchBar|

Comment 3

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
serious functional regression

Comment 4

[:Gijs (he/him)]

Attachment: fix autocomplete in case search bar doesn't exist,

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8550400 [details] [diff] [review]
fix autocomplete in case search bar doesn't exist,

Review of attachment 8550400 [details] [diff] [review]:
-----------------------------------------------------------------

I'm hoping this will get cleaner once bug 1119250 is fixed.

I'm surprised we could break this without causing any test to fail. Could you please write a test or file a bug for it?

::: browser/base/content/urlbarBindings.xml
@@ +925,5 @@
>            var controller = this.view.QueryInterface(Components.interfaces.nsIAutoCompleteController);
>  
>            // Check for unmodified left-click, and use default behavior
>            var searchBar = BrowserSearch.searchBar;
> +          if (searchBar) {

I think you want to duplicate the whole |searchBar && searchBar.textbox == this.mInput| test from line 943. It doesn't seem right to set properties on the searchbar when the user clicks on the autocomplete panel in a web page.

Comment 6

[:Gijs (he/him)]

You really think it's worth making a test that:

a) removes the search bar
b) opens a web page
c) creates autocomplete results for some input on that page
d) triggers the autocomplete popup
e) triggers a click on the autocomplete entry and checks that it's filled in?

... because someone forgot to do a nullcheck when they added some telemetry code?

I don't think the effort required to write a test here is commensurate with the size of the problem and the likelihood that it'll recur.

Comment 7

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #6)

> ... because someone forgot to do a nullcheck when they added some telemetry
> code?

That's not the problem I'm seeing. The problem I see is that the search code and the autocomplete code are interdependent in ways that don't make sense, hence causing people (like Blake and me) to forget to think about edge cases that they need to care about. I would have liked a test ensuring that autocompletes in web pages work regardless of the state of the browser UI.

> I don't think the effort required to write a test here is commensurate with
> the size of the problem and the likelihood that it'll recur.

I'm hoping to separate the searchbar panel code from the generic autocomplete code in bug 1119250, so hopefully that will make it less likely to recur. You are right that it's probably not worth the effort. Sorry for the request I didn't fully think through.

By the way, is there a good reason for web page form autocomplete to use a binding implemented in urlbarBindings.xml? I don't see how that relates to the urlbar, I would have expected a binding directly from toolkit/content/widgets/autocomplete.xml.

Comment 8

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> 
> > ... because someone forgot to do a nullcheck when they added some telemetry
> > code?
> 
> That's not the problem I'm seeing. The problem I see is that the search code
> and the autocomplete code are interdependent in ways that don't make sense,
> hence causing people (like Blake and me) to forget to think about edge cases
> that they need to care about. I would have liked a test ensuring that
> autocompletes in web pages work regardless of the state of the browser UI.

A fine point, but "regardless of the state of the browser UI" is a big ask. That would include trying the panel and so on, or moving it to a different toolbar and collapsing that toolbar. :-)

I've lamented before that we have a large number of customizability options that often interact in surprising ways that are hard to test adequately (cf. http://www.gijsk.com/blog/2014/04/why-doing-visual-refreshes-of-firefox-is-hard/ ). That's not to say that we should get rid of all of them (although I do think we should push back against even more where possible), just that we need to be extra vigilant.

Perhaps we can write some kind of fuzzing test tool that checks common interactions in fuzzed UI states, or something. Kind of like MattN's screenshot tool, but for functional things. I can file a bug on that.

> > I don't think the effort required to write a test here is commensurate with
> > the size of the problem and the likelihood that it'll recur.
> 
> I'm hoping to separate the searchbar panel code from the generic
> autocomplete code in bug 1119250, so hopefully that will make it less likely
> to recur. You are right that it's probably not worth the effort. Sorry for
> the request I didn't fully think through.

OK. I will upload a new patch with correct checks and I will move the comment which has gotten displaced from the existing if condition, too.

> By the way, is there a good reason for web page form autocomplete to use a
> binding implemented in urlbarBindings.xml? I don't see how that relates to
> the urlbar, I would have expected a binding directly from
> toolkit/content/widgets/autocomplete.xml.

I don't know the answer to that question, Dão, Jared or Gavin might - probably best to ask in bug 1119250.

Comment 9

[:Gijs (he/him)]

Attachment: fix autocomplete in case search bar doesn't exist,

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> Perhaps we can write some kind of fuzzing test tool that checks common
> interactions in fuzzed UI states, or something. Kind of like MattN's
> screenshot tool, but for functional things. I can file a bug on that.

bug 1122825

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8550611 [details] [diff] [review]
fix autocomplete in case search bar doesn't exist,

Review of attachment 8550611 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 12

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/e161904aeeb9

Comment 13

[:Gijs (he/him)]

Comment on attachment 8550611 [details] [diff] [review]
fix autocomplete in case search bar doesn't exist,

Approval Request Comment
[Feature/regressing bug #]: bug 1102937 
[User impact if declined]: broken autocomplete if the search bar isn't in its default spot and hasn't been created
[Describe test coverage new/current, TBPL]: no :-(
[Risks and why]: very low, simple null check
[String/UUID change made/needed]: no

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e161904aeeb9

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8550611 [details] [diff] [review]
fix autocomplete in case search bar doesn't exist,

Aurora+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/939fc9a9173a