Closed Bug 661831 Opened 9 years ago Closed 4 years ago

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

Categories

(Firefox :: Preferences, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
Firefox 43
Tracking Status
firefox43 --- verified

People

(Reporter: mardeg, Assigned: steffen.wilberg)

References

()

Details

(Whiteboard: [testday-20110603])

Attachments

(1 file, 1 obsolete file)

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?
If possible, this might also be good for about:addons.
OS: Windows XP → All
Hardware: x86 → All
Summary: Doing Ctrl F in about:permissions should focus the "Search Sites" box → Doing Ctrl+F in about:permissions (and about:addons?) should focus the "Search Sites" box
Severity: normal → enhancement
Blocks: 821049
I filed bug 1195060 to fix the Add-ons manager, so this bug should only be about about:permissions.
Assignee: nobody → steffen.wilberg
Summary: Doing Ctrl+F in about:permissions (and about:addons?) should focus the "Search Sites" box → In about:permissions, Ctrl+f should 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.
Attachment #8648458 - Flags: review?(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.
Attachment #8648458 - Flags: review?(margaret.leibovic) → review?(jaws)
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?
Attachment #8648458 - Flags: review?(jaws)
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.
address review comments.
Attachment #8651943 - Flags: review?(jaws)
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.
Flags: needinfo?(steffen.wilberg)
url:        https://hg.mozilla.org/integration/fx-team/rev/5b6fdfc822e9d18614035d0ce4962ac63eb842b6
changeset:  5b6fdfc822e9d18614035d0ce4962ac63eb842b6
user:       Steffen Wilberg <steffen.wilberg@web.de>
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
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.
Flags: needinfo?(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?
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
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.
Attachment #8651943 - Attachment is obsolete: true
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 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.
Attachment #8648458 - Flags: review?(jaws)
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
Attachment #8648458 - Flags: review?(jaws) → review+
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
https://hg.mozilla.org/mozilla-central/rev/d6ea1920cbe2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
QA Whiteboard: [good first verify]
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
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
QA Whiteboard: [good first verify] → [good first verify][bugday-20151118]
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!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.