nsITypeAheadFind: get rid of aHasFocus argument

RESOLVED FIXED in mozilla1.9alpha3

Status

()

Toolkit
Find Toolbar
P2
normal
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

Trunk
mozilla1.9alpha3
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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.
Status: NEW → ASSIGNED
Priority: -- → P2
Created attachment 245380 [details] [diff] [review]
v1
Attachment #245380 - Flags: review?(masayuki)
Created attachment 245414 [details]
unit test
Attachment #245414 - Flags: review?(gavin.sharp)
Flags: in-testsuite?
Comment on attachment 245380 [details] [diff] [review]
v1

looks good. thank you for your work.
Attachment #245380 - Flags: review?(masayuki) → review+

Comment 4

11 years ago
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.
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

11 years ago
(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.
(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).
Created attachment 251592 [details] [diff] [review]
updated to tip
Attachment #245380 - Attachment is obsolete: true
Attachment #251592 - Flags: review?(mconnor)
Comment on attachment 251592 [details] [diff] [review]
updated to tip

(just for the findbar.xml changes).
Attachment #251592 - Flags: review?(mconnor) → review?(gavin.sharp)
Attachment #251592 - Flags: review?(gavin.sharp) → review+
Created attachment 253443 [details]
unit test

Unit test that also verifies that the findbar doesn't steal focus from chrome, as discussed on IRC.
Assignee: mano → gavin.sharp
Attachment #245414 - Attachment is obsolete: true
Attachment #253443 - Flags: review?
Attachment #245414 - Flags: review?(gavin.sharp)
Attachment #253443 - Flags: review? → review?(mano)
Assignee: gavin.sharp → mano
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Target Milestone: Firefox 3 alpha1 → Firefox 3 alpha2
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.
Attachment #253443 - Flags: review?(mano) → review-
(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 on attachment 253443 [details]
unit test

you win :)
Attachment #253443 - Flags: review- → review+
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
Attachment #251592 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [a3]
Target Milestone: Firefox 3 alpha2 → Firefox 3
Whiteboard: [a3]
Target Milestone: Firefox 3 → Firefox 3 alpha3
Created attachment 255980 [details] [diff] [review]
unit test for checkin
Attachment #253443 - Attachment is obsolete: true
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
Flags: in-testsuite? → in-testsuite+
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.