1195060 - In the Add-ons manager, Ctrl+f doesn't focus the search box directly, but first invokes the findbar

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Steffen Wilberg]

In the Add-ons manager, Ctrl+f first invokes the find bar. The search box of the Add-ons manager doesn't get focussed until the find bar is closed.

The Add-ons manager has two keyboard shortcuts to focus the search box on its top right:
Ctrl+f (Cmd+f on Mac), and "/".

"/" works fine.
However, Ctrl+f invokes the find bar. After you close the find bar by pressing Esc or clicking its close button, the search box gets focussed finally.


Bug 948888 messed with inlining scripts and introduced a hack because of bug 371900 - xul <key> handling doesn't fire a command event if @oncommand is missed.
But that hack doesn't work here (on Windows 7) for some reason.

I have a simpler workaround by adding oncommand=";". That is done in other places already; bug 978115 is on file for cleaning that up once bug 371900 is fixed.

I wonder why http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_bug570760.js doesn't fail here. Perhaps the test environment doesn't have a find bar?

Comment 1

[Steffen Wilberg]

Attachment: MozReview Request: Bug 1195060 - In the Add-ons manager, make Ctrl+f focus the search box again instead of invoking the findbar first.

add preventDefault() to stop the find bar from showing up

Comment 2

[Steffen Wilberg]

Attachment: MozReview Request: better hack

better hack

Comment 3

[Steffen Wilberg]

Attachment: MozReview Request: Bug 1195060 - In the Add-ons manager, make Ctrl+f focus the search box again instead of invoking the findbar first. r?mossop

Bug 1195060 - In the Add-ons manager, make Ctrl+f focus the search box again instead of invoking the findbar first. r?mossop

Comment 4

[Steffen Wilberg]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=352ca43f3e8e

Comment 5

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/16211/#review14807

This looks good and I suspect this is caused by the change in event dispatch to content by e10s. Sure enough this only reproduces in an e10s window and browser_bug570760.js is disabled in e10s runs which is why we're not seeing a failure. The note about why it fails talks about synthesizeMouse not being e10s friendly, but it shouldn't have to be since the add-ons manager always runs in the main process. I bet it's failing precisely because of this bug. Can you try re-enabling it and running that test in e10s mode, it might work with your patch.

Comment 6

[Steffen Wilberg]

Indeed.

WITHOUT THIS PATCH, running the test in a normal (non e10s) profile succeeds:
"mach mochitest toolkit/mozapps/extensions/test/browser/browser_bug570760.js"

But running it with --e10s like this:
"mach mochitest --e10s toolkit/mozapps/extensions/test/browser/browser_bug570760.js"
fails:
TEST-PASS | toolkit/mozapps/extensions/test/browser/browser_bug570760.js | Search box should have been focused due to the f key
TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_bug570760.js | Search box should have been focused due to the / key

This is surprising, because in manual testing accel+f never works, whereas "/" does work!?


WITH THIS PATCH, the test succeeds both with and without --e10s.

Comment 7

[Steffen Wilberg]

Comment on attachment 8648443 [details]
MozReview Request: Bug 1195060 - In the Add-ons manager, make Ctrl+f focus the search box again instead of invoking the findbar first.

add preventDefault() to stop the find bar from showing up

Comment 8

[Steffen Wilberg]

Comment on attachment 8648444 [details]
MozReview Request: better hack

better hack

Comment 9

[Steffen Wilberg]

Comment on attachment 8648445 [details]
MozReview Request: Bug 1195060 - In the Add-ons manager, make Ctrl+f focus the search box again instead of invoking the findbar first. r?mossop

Bug 1195060 - In the Add-ons manager, make Ctrl+f focus the search box again instead of invoking the findbar first. r?mossop

Comment 10

[Steffen Wilberg]

Attachment: MozReview Request: reenable test browser_bug570760.js

reenable test browser_bug570760.js

Comment 11

[Steffen Wilberg]

Comment on attachment 8648443 [details]
MozReview Request: Bug 1195060 - In the Add-ons manager, make Ctrl+f focus the search box again instead of invoking the findbar first.

Bug 1195060 - In the Add-ons manager, make Ctrl+f focus the search box again instead of invoking the findbar first.

Comment 12

[Steffen Wilberg]

(I folded the commits into one.)

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8648443 [details]
MozReview Request: Bug 1195060 - In the Add-ons manager, make Ctrl+f focus the search box again instead of invoking the findbar first.

https://reviewboard.mozilla.org/r/16213/#review15619

Wonderful!

Comment 14

[Steffen Wilberg]

https://hg.mozilla.org/integration/fx-team/rev/611e7966ec27acc4452a9b976ace56e1be481700
Bug 1195060 - In the Add-ons manager, make Ctrl+f focus the search box again instead of invoking the findbar first. r=mossop

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/611e7966ec27