Issue 3129/3144 - Fixed: Findbar key handling conflicted with filter editing

Previous title: Issue 3129 - Ignore accessibility.typeaheadfind preference in filter settings dialog

[Wladimir Palant, 2015-10-05 14:17:21]

https://codereview.adblockplus.org/29328828/diff/29328829/chrome/content/ui/f...
File chrome/content/ui/filters-search.js (right):

https://codereview.adblockplus.org/29328828/diff/29328829/chrome/content/ui/f...
chrome/content/ui/filters-search.js:60: });
I don't think we need to disable the functionality altogether, it does make
sense. We should however make sure that keyboards events from our tree editor
are left alone.

The real problem is that we are passing events to the findbar without checking
them first. However, that's not how their content script works now - see
http://hg.mozilla.org/mozilla-central/file/45f01961ecd0/toolkit/content/brows....
So the real solution here and in issue 3144 would be checking for event.ctrlKey
|| event.altKey || event.metaKey || event.defaultPrevented in our code.

[Thomas Greiner, 2015-10-05 16:45:20]

https://codereview.adblockplus.org/29328828/diff/29328829/chrome/content/ui/f...
File chrome/content/ui/filters-search.js (right):

https://codereview.adblockplus.org/29328828/diff/29328829/chrome/content/ui/f...
chrome/content/ui/filters-search.js:60: });
On 2015/10/05 14:17:20, Wladimir Palant wrote:
> I don't think we need to disable the functionality altogether, it does make
> sense. We should however make sure that keyboards events from our tree editor
> are left alone.
> 
> The real problem is that we are passing events to the findbar without checking
> them first. However, that's not how their content script works now - see
>
http://hg.mozilla.org/mozilla-central/file/45f01961ecd0/toolkit/content/brows....
> So the real solution here and in issue 3144 would be checking for
event.ctrlKey
> || event.altKey || event.metaKey || event.defaultPrevented in our code.

Thanks for the link, I wasn't aware of this implementation. Seems like their
content script does a bit more than that check so I added all the relevant
parts.

BTW I love their note in the content script that says "Please keep in sync with
toolkit/content/widgets/findbar.xml". :D

[Wladimir Palant, 2015-10-05 17:56:43]

NOT LGTM

This is getting way overboard with relying on implementation details. Please
check whether the simple fix I suggested will already work. If not - I guess we
won't have any other choice than to fork it then.

[Thomas Greiner, 2015-10-05 18:15:40]

On 2015/10/05 17:56:43, Wladimir Palant wrote:
> NOT LGTM
> 
> This is getting way overboard with relying on implementation details. Please
> check whether the simple fix I suggested will already work. If not - I guess
we
> won't have any other choice than to fork it then.

I did check that and it doesn't work, unfortunately. The second check fixes
#3144 and the third check fixes #3129. Therefore I'll close this review and look
into forking it.