923801 - Case-sensitive still affects all find bars globally

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

Bug 537013 never took care of the match case button, as per bug 537013 comment 25, bug 537013 comment 26 and bug 537013 comment 27.

Right now, clicking the match case button will set the match case on all find bars, because they all still observe accessibility.typeaheadfind.casesensitive for changes. This goes against the per-tab nature of the new find bar (and can get a bit annoying at times when you don't expect the button state to change on all tabs when you just want it in a specific tab).

My proposal: accessibility.typeaheadfind.casesensitive should be considered only the default state of the button:

- when the find bar is initialized (and in other situations where it should assume default values, see bug 913536, bug 920031 and bug 920972) the button state would be set to the pref's value
- when the find bar is in use, the button won't change state every time the pref is changed, it only changes state upon user command
- clicking the match case button won't change the value of the pref, the pref can only be changed explicitly (i.e. in about:config)

Also, accessibility.typeaheadfind.casesensitive is currently an int pref, which can also have the value of 2, this triggers something like an "auto" mode, which uses a case sensitive search only if it finds any caps character in the find string. This is a very specific case which I doubt is used that much, plus if I wanted a case sensitive search only with non-caps chars, I'd still need to toggle the sensitivity mode anyway, so that system fails there. (If I'm failing to see the point of the "auto" mode please let me know)

Because of this, I would suggest dropping the "auto" mode as well, turning accessibility.typeaheadfind.casesensitive into a bool pref which would better fit the per-tab model IMO.

Comment 1

[Philip Chee]

(In reply to Luís Miguel [:Quicksaver] from comment #0)

> Also, accessibility.typeaheadfind.casesensitive is currently an int pref,
> which can also have the value of 2, this triggers something like an "auto"
> mode, which uses a case sensitive search only if it finds any caps character
> in the find string. This is a very specific case which I doubt is used that
> much, plus if I wanted a case sensitive search only with non-caps chars, I'd
> still need to toggle the sensitivity mode anyway, so that system fails
> there. (If I'm failing to see the point of the "auto" mode please let me
> know)
> 
> Because of this, I would suggest dropping the "auto" mode as well, turning
> accessibility.typeaheadfind.casesensitive into a bool pref which would
> better fit the per-tab model IMO.

How would this affect the findbar in view source and other non-tabbed sessions?

Comment 2

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Philip Chee from comment #1)
> How would this affect the findbar in view source and other non-tabbed
> sessions?

As far as I can tell, it wouldn't affect those any more than expected for the normal firefox find bar.

I ran a search in mozilla-central for "casesensitive", all the results relevant to this preference can also be retrieved from searching for "typeaheadfind.casesensitive" (I only ran the broader search to confirm).

Except for tests, I haven't found any occasion for the specific use of a value of (int) 2; except for the findbar._shouldBeCaseSensitive() (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#731) and findbar._updateCaseSensitivity() (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#463) methods present in all find bars, which implement the auto-mode: _shouldBeCaseSensitive() could easily be by-passed entirely when it is called, or instead just reflect the case-sensitive checkbox check state; _updateCaseSensitivity() hides the checkbox when the pref is value (int) 2, removing this bit should be fairly direct.

As long as any other accompanying gets/sets of the preference are properly changed from (int) to (bool) values, I don't see a reason why any find bar in any context should misbehave.

Plus, I didn't find any way to toggle the pref to value (int) 2, it would have to be done explicitly in about:config; this isn't even documented in http://kb.mozillazine.org/Accessibility.typeaheadfind.casesensitive.

BTW, in http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#449:
> // casesensitive: controls the find bar's case-sensitivity
> //     0 - "never"  (case-insensitive)
> //     1 - "always" (case-sensitive)
> // other - "auto"   (case-sensitive for mixed-case input, insensitive otherwise)
The last is false, it doesn't check for mixed-case input, it checks if there are any upper-case chars in the string, i.e. it would also return true if all chars are upper-case ( _shouldBeCaseSensitive() ):
> return aString != aString.toLowerCase();

Comment 3

[Luís Miguel [:quicksaver] (not reading bugmail)]

Of course, I don't know if any add-ons use/set this pref. I know mine don't, adapting the code from (int) to (bool) values shouldn't be hard on the add-ons developer side, and to keep the "auto" mode should they choose to, as I've shown, is just a matter of adding a single line of code and their own preference to go with it.

Comment 4

[Tomasz Kołodziejski [:tomasz]]

I'll work on this. I don't think, however, that there is much sense in removing that "auto" option as we don't know whether anyone's using it. And yes, I think there's no UI to change that value and I only know about it since I saw the source code. Personally, I don't find it useful at all.

Comment 5

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-tabwise-casesensitivity.patch

This is a proposed patch. It simply gets the pref value as the starting point. Then I don't touch prefs at all.

We can also consider pushing the current state to prefs so that the new search start with that default.

Comment 6

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-tabwise-casesensitivity.patch

I changed the logic back a little bit to pass the tests.

So: casesensitive pref gives just the default state for the findbar but if it is changed (for example set to auto) I update the current state. It seems quite natural because when someone changes that he changes the global state of the browser.

This patch comes now with a simple test making sure the state of one button does not influence the other one.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8480756 [details] [diff] [review]
find-tabwise-casesensitivity.patch

>diff --git a/toolkit/content/tests/browser/browser_findbar.js b/toolkit/content/tests/browser/browser_findbar.js

It might be clearer to have a separate sub-test for this, rather than incidentally testing this patch as part of a different basic test. 

>diff --git a/toolkit/content/widgets/findbar.xml b/toolkit/content/widgets/findbar.xml

>       <method name="_setCaseSensitivity">

>+          this._typeAheadCaseSensitive = aCaseSensitive;

This should convert to 1/0.

Some people who always want case sensitivity might be upset that there is no longer an easy way to toggle it on globally. These people can still manually adjust the pref using about:config, but we might investigate exposing that default in the preferences (or writing an addon to easily toggle it globally).

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #7)
> It might be clearer to have a separate sub-test for this, rather than
> incidentally testing this patch as part of a different basic test. 

Looks like you can convert this to use add_task relatively easily (since bug 872229).

Comment 9

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-tabwise-casesensitivity.patch

I addressed the issues. Will post try run later.

Comment 10

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-tabwise-casesensitivity.patch

Now I made it clear how _setCaseSensitivity behaves.

https://tbpl.mozilla.org/?tree=Try&rev=4b052f95256c

Final look, gavin?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8481538 [details] [diff] [review]
find-tabwise-casesensitivity.patch

>diff --git a/toolkit/content/tests/browser/browser_findbar.js b/toolkit/content/tests/browser/browser_findbar.js

>+function* test_found() {
>+  // Create second tab.
>+  yield promiseTestPageLoad();

It's no longer really a "second tab" - I would just remove the comment.

>+// Setting first findbar to case-sensitive mode should not affect
>+// new tab find bar.
>+add_task(function* test_tabwise_case_sensitive() {
>+  findbar.getElement("find-case-sensitive").click();
>+  yield Task.spawn(test_found);

It's a little sub-optimal that this test depends on the first "test task" in a somewhat non-obvious way. It's probably not worth changing this all around, but indicating the dependency with a comment could be useful. Something like "findbar still refers to the findbar opened in test_found(), compare its 'match case' state to our new findbar"

>diff --git a/toolkit/content/widgets/findbar.xml b/toolkit/content/widgets/findbar.xml

>-        - @param aCaseSensitive (boolean)
>+        - @param aCaseSensitive (int)

While you're at it, I would rename this to "aCaseSentivity" to reflect that it isn't a boolean.

Comment 12

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-tabwise-casesensitivity.patch

Good point with the tests. They are now independent just as they should be.

I updated the argument name but it's still messy in the whole file (and global pref is also called accessibility.typeaheadfind.casesensitive so I didn't try to update it everywhere.

Will you have another look :gavin?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8483798 [details] [diff] [review]
find-tabwise-casesensitivity.patch

Sorry, guess I didn't realize the naming issue affected things more broadly - it's probably best to avoid introducing an inconsistency with the rest of the code and just stick with "aCaseSensitive".

r=me with that change - no need to re-request review!

Comment 14

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-tabwise-casesensitivity.patch

I just updated the patch to the current tip.

I didn't change it back to case sensitive it's mixed all over the place. Ping me if you want to undo it.

https://tbpl.mozilla.org/?tree=Try&rev=83553a48c2ac

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Petruta Horea [:phorea], Desktop QA]

Verified that "Match Case" works per tab now using Nightly 35.0a1 20140914030209 under Win 7 64-bit, Ubuntu 12.10 32-bit and Mac OSX 10.8.5.