Last Comment Bug 360437 - nsITypeAheadFind: get rid of aHasFocus argument
: nsITypeAheadFind: get rid of aHasFocus argument
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.9alpha3
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
Mentors:
Depends on:
Blocks: 189039
  Show dependency treegraph
 
Reported: 2006-11-12 02:42 PST by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2008-07-31 04:30 PDT (History)
6 users (show)
asaf: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (11.53 KB, patch)
2006-11-12 04:04 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
masayuki: review+
Details | Diff | Splinter Review
unit test (3.86 KB, application/vnd.mozilla.xul+xml)
2006-11-12 15:20 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details
updated to tip (13.41 KB, patch)
2007-01-15 16:15 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
gavin.sharp: review+
Details | Diff | Splinter Review
unit test (4.45 KB, application/vnd.mozilla.xul+xml)
2007-01-30 20:26 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
asaf: review+
Details
as checked in (13.41 KB, patch)
2007-02-08 17:23 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
unit test for checkin (7.25 KB, patch)
2007-02-21 17:55 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review

Description Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-12 02:42:13 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-12 04:04:24 PST
Created attachment 245380 [details] [diff] [review]
v1
Comment 2 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-12 15:20:14 PST
Created attachment 245414 [details]
unit test
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-11-13 11:39:52 PST
Comment on attachment 245380 [details] [diff] [review]
v1

looks good. thank you for your work.
Comment 4 Peter Kasting 2006-11-13 11:46:07 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-13 12:08:44 PST
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 2006-11-13 12:55:32 PST
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-13 13:18:21 PST
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-01-15 16:15:37 PST
Created attachment 251592 [details] [diff] [review]
updated to tip
Comment 9 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-01-30 14:17:58 PST
Comment on attachment 251592 [details] [diff] [review]
updated to tip

(just for the findbar.xml changes).
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-01-30 20:26:27 PST
Created attachment 253443 [details]
unit test

Unit test that also verifies that the findbar doesn't steal focus from chrome, as discussed on IRC.
Comment 11 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-01-31 03:55:17 PST
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: gavin@gavinsharp.com] 2007-01-31 09:25:38 PST
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-01-31 09:44:17 PST
Comment on attachment 253443 [details]
unit test

you win :)
Comment 14 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-02-08 17:23:06 PST
Created attachment 254476 [details] [diff] [review]
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-02-21 17:55:53 PST
Created attachment 255980 [details] [diff] [review]
unit test for checkin
Comment 16 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-02-21 17:58:34 PST
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

Note You need to log in before you can comment on or make changes to this bug.