360437 - nsITypeAheadFind: get rid of aHasFocus argument

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P2)

Description

[Asaf Romano (gone)]

nsITypeAheadFind::find and nsITypeAheadFind::findAgain have an [in]aHasFocus argument which is supposed to be |true| if the find textfield is focused.

When |false| is passed, find/findAgain would focus a text/textarea if a search result was found in there.

I believe this is the wrong approach for at least two reasons:
 - UI-wise, we should never take focus out of chrome elements when (Shift)F3 is pressed.
 - Clients of this interface are not necessarily find-toolbars.

I suggest removing this argument and check the focused DOM window instead.

Comment 1

[Asaf Romano (gone)]

Attachment: v1

Comment 2

[Asaf Romano (gone)]

Attachment: unit test

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 245380 [details] [diff] [review]
v1

looks good. thank you for your work.

Comment 4

[Peter Kasting]

Without looking too closely at the patch, this is roughly the approach I took in my original patch to enable find-in-textfields.  It lived on in the 1.8 branch, where I couldn't change the interface, so make sure you look at the code there and ways in which this behaves similarly or differently.  (Things weren't done this way on the trunk because masayuki suggested the method on trunk was preferable.)

The main difference here I see is that you (purposefully) don't ever change focus when finding in a window that doesn't have focus, instead of just not changing when the findbar has focus.  I'm not sure whether this is correct or not.  The way all this stuff feels to users is extremely subtle.  Make sure to test as many scenarios as possible to see whether finding a match in a textfield and then immediately typing a few characters does the thing you'd expect.

Comment 5

[Asaf Romano (gone)]

masayuki: thanks for review.

Peter: Your branch check checked the focused element by its id (FindFieldIsFocused or something like that) IIRC.

As for the behavior, I don't think we should ever move the focus from chrome to content in this case (e.g. when pressing F3 when the location bar is focused).

Comment 6

[Peter Kasting]

(In reply to comment #5)
> As for the behavior, I don't think we should ever move the focus from chrome to
> content in this case (e.g. when pressing F3 when the location bar is focused).

I don't think that's right.  If the Match Case checkbox in the find bar has focus, for example, I think you _should_ change focus.  In general, it seems like you should change focus unless the focus is on a text-entry chrome element, not just any chrome element.

Comment 7

[Asaf Romano (gone)]

(In reply to comment #6)
> (In reply to comment #5)
> > As for the behavior, I don't think we should ever move the focus from chrome to
> > content in this case (e.g. when pressing F3 when the location bar is focused).
> 
> I don't think that's right.  If the Match Case checkbox in the find bar has
> focus, for example, I think you _should_ change focus.  In general, it seems
> like you should change focus unless the focus is on a text-entry chrome
> element, not just any chrome element.
> 

I cannot see why. If you focused the match-case checkbox on purpose, there's no sense in taking focus out of it. With that said, I do see this as a special case, and the solution might be to focus back the find textfield whenever the checkbox is ticked (Actually, that might be the right thing event without the behavior change introduced here).

Comment 8

[Asaf Romano (gone)]

Attachment: updated to tip

Comment 9

[Asaf Romano (gone)]

Comment on attachment 251592 [details] [diff] [review]
updated to tip

(just for the findbar.xml changes).

Comment 10

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

Attachment: unit test

Unit test that also verifies that the findbar doesn't steal focus from chrome, as discussed on IRC.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 253443 [details]
unit test

    var textbox = document.getElementById("textbox");
    textbox.focus();
    gFindBar.close();
    gFindBar.onFindAgainCommand(false);
    ASSERT(textbox.hasAttribute("focused"),
           "Focus was stolen from a chrome element");

The order here is wrong, I think. closing the findbar may change focus, thus the textbox should be focused right before onFindAgain is called.

Comment 12

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

(In reply to comment #11)
> The order here is wrong, I think. closing the findbar may change focus, thus
> the textbox should be focused right before onFindAgain is called.

It only changes focus if something in the binding itself is focused, so this isn't actually a problem, right? The test gives me the expected results, with and without your patch.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 253443 [details]
unit test

you win :)

Comment 14

[Asaf Romano (gone)]

Attachment: as checked in

mozilla/toolkit/components/typeaheadfind/public/nsITypeAheadFind.idl 1.10
mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp 1.31
mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.h 1.10
mozilla/toolkit/content/widgets/findbar.xml 1.8

Comment 15

[Asaf Romano (gone)]

Attachment: unit test for checkin

Comment 16

[Asaf Romano (gone)]

mozilla/toolkit/content/tests/chrome/Makefile.in 1.2
mozilla/toolkit/content/tests/chrome/bug360437_window.xul initial revision: 1.1
mozilla/toolkit/content/tests/chrome/test_bug360437.xul initial revision: 1.1