Closed Bug 892926 Opened 11 years ago Closed 10 years ago

FindBar steals focus from contents when switching tab

Categories

(Firefox :: Tabbed Browser, defect)

25 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox24 --- unaffected
firefox25 - ---

People

(Reporter: alice0775, Assigned: unusualtears)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Build Identifier:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9a46657d49e2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130712 Firefox/25.0 ID:20130712001738

Steps To Reproduce:
1. Open 2 tabs, say[tab A][tab B]
2. Open FindBar on [tab A]
   --- observe focus, now FindBar get focused --- as expected
3. Click contents area or input/textarea of the contents[tab A]
   --- observe focus, now contents get focused --- as expected
4. Switch to [tab B] and switch back to [tab A]
   --- observe focus

Actual Results:
   FindBar steals focus at step4.

Expected Results:
   FindBar should not steal focus at step4.
   Focused item should be persisted per tab.

This problem does not happens Aurora24.0a2.
Summary: FindBar steals focus when switching tab from contents → FindBar steals focus from contents when switching tab
I suspect this is caused by bug 537013
(In reply to Cork from comment #1)
> I suspect this is caused by bug 537013

No, this is caused by Bug 892384
Assignee: nobody → unusualtears
I'm ambivalent about the tradeoff here.  

(Without patch) hurdle: you either close it immediately or focus another element.

(With patch) clutter: unfocused findbar sits open until you use it or clean it up.

Bug 565552, comment 1 lists wanted improvements for the findbar.  One of them, "close when you click outside it", would seemingly remove this tradeoff.  I couldn't find a bug for that, so I filed bug 893321.

In the naive implementation, the findbar would always be closed when switching to a tab, so focus is moot.  

A more complex approach might leave it open only if the focus didn't move to an element in the same tab.  In that case it could be safely focused if it's open (ie, the current behavior).

Dão, any thoughts on this bug?
Attachment #775093 - Flags: review?(dao)
Comment on attachment 775093 [details] [diff] [review]
Track/restore focus of findbar findfield on tab switch

I'm not convinced we should do this. Don't think this needs to be tracking-firefox25+ either.
Attachment #775093 - Flags: review?(dao)
Given the engineering opinion, untracking.
Is there a reason why in the patch for bug 892384 the .getAttribute('focused') == 'true' check was removed? Was this not a valid check anymore since bug 537013 landed?

An alternative to the proposed patch would be to save the state keeping in mind the per-tab basis. That is, when switching away from a tab with the findbar open and focused, save that in maybe a bool property or an attribute of that same findbar, then when switching back to that tab, check for that property and focus the findbar only if it's true.
Sounds like a Dao needinfo.
Flags: needinfo?(dao)
(In reply to Luís Miguel [:Quicksaver] from comment #6)
> Is there a reason why in the patch for bug 892384 the
> .getAttribute('focused') == 'true' check was removed? Was this not a valid
> check anymore since bug 537013 landed?

No, it wasn't, because whether one tab's find bar is focused is now irrelevant for other tabs with the find bar not being global anymore.
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #8)
> No, it wasn't, because whether one tab's find bar is focused is now
> irrelevant for other tabs with the find bar not being global anymore.

I'm sorry, but I honestly don't understand this. The focused attribute may not be relevant to other tabs, but it definitely is relevant to the current tab, which is what it would be checking for. So I'll rephrase.

Currently, the code is making these questions when/after switching to a new tab:

- Is the find bar in this tab (the one switched to) initialized?
- Is the initialized find bar in this tab not hidden?

And this question was dropped by the patch in bug 892384:

- Does the textbox in the not hidden, initialized find bar in this tab have the "focused" attribute?

My question is, is the "focused" attribute kept/restored to the textbox in the find bar when switching to a tab? Or is it always gone because the element was hidden (and so, automatically not focused, automatically removing the attribute), regardless of whether the user last left that tab with the find bar focused or not?

If it is kept, then the fix would be to keep the same three checks as before and call .focus() before break;ing out of that loop, to actually focus the textbox. And if not, second paragraph in comment 6.
(In reply to Luís Miguel [:Quicksaver] from comment #9)
> My question is, is the "focused" attribute kept/restored to the textbox in
> the find bar when switching to a tab?

No.
This bug needs to be fixed... it is annoying as hell for editing Wikipedia. The fact that you might one day change the behaviour of the find bar (automatically disappearing or whatever) is not an excuse.

Quicksaver's alternative sounds like the best to me: only focus the find bar on tab switch if a newly introduced variable says it was focused before.
(In reply to Adam [:hobophobe] from comment #3)
> I'm ambivalent about the tradeoff here.  
> 
> (Without patch) hurdle: you either close it immediately or focus another
> element.
> 
> (With patch) clutter: unfocused findbar sits open until you use it or clean
> it up.

I honestly don't see how there is any tradeoff.

(With patch) clutter: unfocused findbar sits open until you use it or clean it up.

(Without patch) hurdle and clutter: you have to focus another element to get the tab back to the way it was, and even when you do this, the findbar is still open until you use it or clean it up.

This patch seems like a good solution until more radical changes to the findbar are landed. It only differs from Quicksaver's idea by making the boolean _findBarFocused a property of the tab rather than the findbar itself.
(In reply to Connor Behan from comment #12)
> (Without patch) hurdle and clutter: you have to focus another element to get
> the tab back to the way it was, and even when you do this, the findbar is
> still open until you use it or clean it up.

You can just hit Esc. It will both close the find bar and get back to the previously focused element on the page.
Page problem by this bug

Steps To Reproduce:
1. Open tabs
2. Open https://developer.mozilla.org/en-US/Add-ons/Code_snippets in [tab A]
3. Open Findbar Ctrl+F
4. Type "something" to <input> field at the top left of the page
5. Switch to any tab
6. Switch to [tab A]

Actual Results:
  I cannot see typed text because the <input> field lost focus.
  I cannot continuously type to the <input> field. 

Expected results:
  <input> field at the top left of the page should be focused.
  I can continuously type to the <input> field
(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to Connor Behan from comment #12)
> > (Without patch) hurdle and clutter: you have to focus another element to get
> > the tab back to the way it was, and even when you do this, the findbar is
> > still open until you use it or clean it up.
> 
> You can just hit Esc. It will both close the find bar and get back to the
> previously focused element on the page.

Ok so it's only a minor hurdle. In any case, I will keep using the patch. I'm glad it can be applied to omni.ja without a rebuild.
(In reply to Dão Gottwald [:dao] (on vacation from March 21 to 28) from comment #13)
> You can just hit Esc. It will both close the find bar and get back to the
> previously focused element on the page.

This sounds like you want to WONTFIX this bug. If that is not the case, please suggest an alternative fix to this bug that you would consider. But to argue against WONTFIX:

You *can* just hit Esc. But having to do is very annoying because the browser is violating the rule that "the tab is just the way you left it". I run into this focus issue all the time on Bugzilla, because I often search for things and so almost all my bugzilla tabs have the findbar open. And then when I try to enter comments I need to look up stuff in other tabs, so I switch back and forth while entering a comment. And so often when I switch back to the tab I'm writing a comment in (and where I left the focus in the textarea) the focus is in the findbar and when I continue typing the text ends up there. Yes, I *can* just hit Esc but by the time I've noticed I've already typed a few words which I have to re-type, not to mention that it breaks flow.

That being said, I don't really care how the bug is fixed (and I don't know enough front-end code to understand exactly what the patch is doing), but I would like it so that the findbar doesn't steal focus when you switch to a tab that has it.
Flags: needinfo?(dao)
I think the best way to get this fixed is to involve other seasoned Mozilla devs. However I can't ask one of them to review the patch because "Request Review" only works from the submitter's account. It would be pretty stupid to submit duplicates but I don't see another option.
(In reply to Connor Behan from comment #17)
> I think the best way to get this fixed is to involve other seasoned Mozilla
> devs. However I can't ask one of them to review the patch because "Request
> Review" only works from the submitter's account. It would be pretty stupid
> to submit duplicates but I don't see another option.

I am able to to flag someone for review on the patch even though I am not the submitter. I am happy to flag someone for you if you tell me who.
(In reply to Botond Ballo [:botond] from comment #18)
> (In reply to Connor Behan from comment #17)
> > I think the best way to get this fixed is to involve other seasoned Mozilla
> > devs. However I can't ask one of them to review the patch because "Request
> > Review" only works from the submitter's account. It would be pretty stupid
> > to submit duplicates but I don't see another option.
> 
> I am able to to flag someone for review on the patch even though I am not
> the submitter. I am happy to flag someone for you if you tell me who.

How about Mike Hommey?
Comment on attachment 775093 [details] [diff] [review]
Track/restore focus of findbar findfield on tab switch

Flagging as per comment 19.
Attachment #775093 - Flags: review?(mh+mozilla)
Comment on attachment 775093 [details] [diff] [review]
Track/restore focus of findbar findfield on tab switch

Review of attachment 775093 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a frontend peer.
Attachment #775093 - Flags: review?(mh+mozilla)
Comment on attachment 775093 [details] [diff] [review]
Track/restore focus of findbar findfield on tab switch

Review of attachment 775093 [details] [diff] [review]:
-----------------------------------------------------------------

All right, let's try Mike Conley.
Attachment #775093 - Flags: review?(mconley)
I tend to agree with kats - not only does this add an extra step for users ("Esc", or keyboard/mousing to re-focus) but it seems to violate the principle of least surprise. The tab is taking the reigns away from the user here in terms of what needs to be focused.

So I'm on board with a patch to fix this. Unless there's a compelling argument for why the current state of things is better (and I don't think there is), I'll start reviewing this.
Comment on attachment 775093 [details] [diff] [review]
Track/restore focus of findbar findfield on tab switch

Review of attachment 775093 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with my nits fixed.

::: browser/base/content/tabbrowser.xml
@@ +1109,5 @@
>                      break;
>                    }
>                  }
>  
>                  // If the find bar is open, focus it.

This comment will need updating.

@@ +1110,5 @@
>                    }
>                  }
>  
>                  // If the find bar is open, focus it.
> +                if (this.selectedTab._findBarFocused) {

This could break if something like an add-on has removed or hidden the findbar since the last time we saw it, if this.selectedTab._findBarFocused is truthy. I would advise keeping the gFindBarInitialized and !gFindBar.hidden checks.
Attachment #775093 - Flags: review?(mconley) → review+
Thanks, Mike!

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=319846f719a4
Attachment #775093 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dd60c3189c8a
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Flags: needinfo?(dao)
Reproduced the issue on Firefox 25.0 (2013-10-25) buildID: 20131025151332, verified as fixed on: Firefox 31.0 (buildID: 20140714151536) under Windows 7 64bit.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.