661831 - In about:permissions, Ctrl+f should focus the filter box instead of invoking the find bar

Status: VERIFIED FIXED

(Firefox :: Settings UI, enhancement)

Description

[Mardeg]

Aleksej suggested this be filed as a new bug dependent on bug 661823, and better to do this than nothing. The main issue will be when the Find bar already exists, should Ctrl F close it and focus the "Search Sites" box in the one go?

Comment 1

[[:Aleksej]]

If possible, this might also be good for about:addons.

Comment 2

[Steffen Wilberg]

I filed bug 1195060 to fix the Add-ons manager, so this bug should only be about about:permissions.

Comment 3

[Steffen Wilberg]

Attachment: MozReview Request: Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar.

Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8648458 [details]
MozReview Request: Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar.

Redirecting to a desktop engineer.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8648458 [details]
MozReview Request: Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar.

https://reviewboard.mozilla.org/r/16221/#review14549

::: browser/components/preferences/aboutPermissions.js:889
(Diff revision 1)
> +    let filterBox = document.getElementById("sites-filter");

Can you add a sitesFilter member variable and initialize it in init(), similar to what is done for sitesList ?

Then update the other two places that call getElementById("sites-filter") to use the member variable?

Comment 6

[Steffen Wilberg]

Comment on attachment 8648458 [details]
MozReview Request: Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar.

Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar.

Comment 7

[Steffen Wilberg]

Attachment: MozReview Request: address review comments.

address review comments.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8651943 [details]
MozReview Request: address review comments.

https://reviewboard.mozilla.org/r/17035/#review15173

Ship It!

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Hey Steffen,

It looks like there was separate commits for the reviews here, rather than appending to the previous review request.

Can you post a patch that merges the two commits here? We can mark that one for checkin-needed.

Comment 10

[Steffen Wilberg]

url:        https://hg.mozilla.org/integration/fx-team/rev/5b6fdfc822e9d18614035d0ce4962ac63eb842b6
changeset:  5b6fdfc822e9d18614035d0ce4962ac63eb842b6
user:       Steffen Wilberg <[e-mail]>
date:       Mon Aug 24 22:18:54 2015 +0200
description:
Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar. r=jaws

Comment 11

[Steffen Wilberg]

Yeah, I just started using hg bookmarks and MozReview and am not sure about the best workflow yet.
Anyway, I rolled the two commits into one and pushed that, see comment 10.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_zbug569342.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=4354991&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/979bb826f040

Comment 13

[Steffen Wilberg]

uncaught exception - TypeError: this.sitesFilter is null at chrome://browser/content/preferences/aboutPermissions.js:891

That's this line:
  this.sitesFilter.focus();

Looks like a race between the init function of the page, which sets this.sitesFilter, and the test, which synthesizes ctrl+f.

Should I add
  if (!this.sitesFilter)
    this.sitesFilter = document.getElementById("sites-filter");
to focusFilterBox, to make that stupid test happy?

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Steffen Wilberg from comment #13)
> uncaught exception - TypeError: this.sitesFilter is null at
> chrome://browser/content/preferences/aboutPermissions.js:891
> 
> That's this line:
>   this.sitesFilter.focus();
> 
> Looks like a race between the init function of the page, which sets
> this.sitesFilter, and the test, which synthesizes ctrl+f.
> 
> Should I add
>   if (!this.sitesFilter)
>     this.sitesFilter = document.getElementById("sites-filter");
> to focusFilterBox, to make that stupid test happy?

It would be better to make siteFilter a getter that calls getElementById and overwrites the getter with the result of the getElementById call.

Comment 15

[Steffen Wilberg]

Comment on attachment 8648458 [details]
MozReview Request: Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar.

Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar.

Comment 16

[Steffen Wilberg]

https://reviewboard.mozilla.org/r/16221/#review15203

::: browser/components/preferences/aboutPermissions.js:361
(Diff revision 3)
> +  get sitesFilter () {
> +    let sitesFilter = document.getElementById("sites-filter");
> +
> +    // Cache this value, overwrite the getter.
> +    Object.defineProperty(this, "sitesFilter", {value: sitesFilter, enumerable: true});
> +
> +    return sitesFilter;
> +  },

stolen from http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/cells.js#37

Comment 17

[Steffen Wilberg]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f0af8f1f854

Comment 18

[Steffen Wilberg]

Comment on attachment 8648458 [details]
MozReview Request: Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar.

Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8648458 [details]
MozReview Request: Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar.

https://reviewboard.mozilla.org/r/16221/#review15249

::: browser/components/preferences/aboutPermissions.js:365
(Diff revision 3)
> +    Object.defineProperty(this, "sitesFilter", {value: sitesFilter, enumerable: true});

This can be simplified to:

get sitesFilter() {
  delete this.sitesFilter;
  return this.sitesFilter = document.getElementById("sites-filter");
}

This is similar to http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#1208

Comment 20

[Steffen Wilberg]

https://hg.mozilla.org/integration/fx-team/rev/d6ea1920cbe2512c9b03ae3f5dd6846badd3f858
Bug 661831 - In about:permissions, make Ctrl+f focus the filter box instead of invoking the find bar. r=jaws

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ashickur Rahman]

Successfully reproduce the bug on 
Name 	Firefox
Version 	42.0
Build ID 	20151029151421
User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:42.0) Gecko/20100101 Firefox/42.0

The fix works for me on 

Name 	Firefox
Version 	43.0b1
Build ID 	20151103023037
User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:43.0) Gecko/20100101 Firefox/43.0

Comment 23

[Ashickur Rahman]

My bad, I forgot to update to latest beta. Fix also works in 

Name 	Firefox
Version 	43.0b4
Build ID 	20151116155110
User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:43.0) Gecko/20100101 Firefox/43.0

Comment 24

[Hossain Al Ikram [:ikram] (QA Contact)]

I have successfully reproduced this bug on Firefox Nightly 7.0a1 (2011-06-03) on Windows 7, 64 bit!

This bug's fix is now verified on Latest 43.0 Beta 4.

Build ID 	20151116155110
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0

As it has been verified on MAC as per comment 23, Changing the status!