Open Bug 1315950 Opened 4 years ago Updated 2 years ago

It should not be possible to put focus into background browsers/tabs

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P3)

defect

Tracking

()

Tracking Status
firefox52 --- wontfix

People

(Reporter: Gijs, Unassigned)

References

(Depends on 1 open bug)

Details

STR:

1. open about:home in one tab
2. in browser console, eval b = gBrowser.selectedBrowser
3. open another tab, and focus the search field in the new tab page
4. in browser console, run b.focus()
5. start typing

ER:
text goes into the new tab search field

AR:
no text appears, if you go to the previous about:home tab, text is shown in the search box there.


This... really shouldn't be possible. Neil, do you know what code is responsible for translating "browser.focus()" into "focus content in the content of this browser", and/or why we don't stop dead when asked to focus something that's presumably display:none or collapsed or whatever?
Flags: needinfo?(enndeakin)
(In reply to :Gijs Kruitbosch from comment #0)
> STR:
> 
> 1. open about:home in one tab
> 2. in browser console, eval b = gBrowser.selectedBrowser
> 3. open another tab, and focus the search field in the new tab page
> 4. in browser console, run b.focus()

actually, I guess you might need to do this on a timeout to ensure you don't manipulate focus by switching between windows.
Blocks: 1313403
I don't see this at all. Can you make some detailed working steps that show this bug?
Flags: needinfo?(enndeakin)
Also, is this with e10s or and/or off?
e10s on.

On current nightly, you can follow the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1313403#c6 - but I hope to land the fix there soon.

I tried to get separate str that don't rely on the still-extant .focus() call in the code I'm fixing there, but I was unsuccessful. It looks like the timing of the focus() call might matter quite a bit, so it feels like a race condition.
Flags: needinfo?(enndeakin)
So in e10s the tab switching code doesn't update the actual selected index on the <tabpanels> element until tab switching is complete. At the point you're calling focus() this hasn't happened yet. For focus, the underlying deck's selected page is used to determine visibility for tab panels.

Note also that the focus call you added by the patch in bug 1313403 is doing nothing since the new browser is not visible. I would assume that_adjustFocusAfterTabSwitch probably sets the focus correctly later.
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #5)
> So in e10s the tab switching code doesn't update the actual selected index
> on the <tabpanels> element until tab switching is complete. At the point
> you're calling focus() this hasn't happened yet. For focus, the underlying
> deck's selected page is used to determine visibility for tab panels.

Can we avoid this, if we know a different tab is going to be selected anyway (ie can we use the tabbrowser's selected state instead of the deck's ) ?

> Note also that the focus call you added by the patch in bug 1313403 is doing
> nothing since the new browser is not visible. I would assume
> that_adjustFocusAfterTabSwitch probably sets the focus correctly later.

It'll do nothing if the tab isn't already selected, which may or may not be the case (ie loads in the same tab will still have a functioning focus() call). In any case, this isn't a change from what happened before 1000458, so I think I'm OK with that.
Flags: needinfo?(enndeakin)
> Can we avoid this, if we know a different tab is going to be selected anyway
> (ie can we use the tabbrowser's selected state instead of the deck's ) ?
> 

Since this could be fragile, I think it would be better if the code here waited for the visibility of the browser to actually change before trying to focus it. But since that already happens, is there actually an issue here?
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #7)
> > Can we avoid this, if we know a different tab is going to be selected anyway
> > (ie can we use the tabbrowser's selected state instead of the deck's ) ?
> > 
> 
> Since this could be fragile, I think it would be better if the code here
> waited for the visibility of the browser to actually change before trying to
> focus it. But since that already happens, is there actually an issue here?

Right, I guess maybe this can be closed. Dão, you originally brought this up in bug 1313403, is there anything else you think we should do in this bug?
Flags: needinfo?(dao+bmo)
(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Neil Deakin from comment #7)
> > > Can we avoid this, if we know a different tab is going to be selected anyway
> > > (ie can we use the tabbrowser's selected state instead of the deck's ) ?
> > > 
> > 
> > Since this could be fragile, I think it would be better if the code here
> > waited for the visibility of the browser to actually change before trying to
> > focus it. But since that already happens, is there actually an issue here?
> 
> Right, I guess maybe this can be closed.

I think Neil is saying that we should remove the seemingly redundant focus call, not that we should just close this bug? Either way this doesn't really seem to address the matter you filed this bug for. I admit the discussion between you and Neil somewhat confuses me.

> Dão, you originally brought this up
> in bug 1313403, is there anything else you think we should do in this bug?

Um, yes ... it shouldn't be possible to type in a background browser even if front-end code focuses the browser at the wrong time.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #9)
> Um, yes ... it shouldn't be possible to type in a background browser even if
> front-end code focuses the browser at the wrong time.

Can we make the deck switching selected panels force focus out of the now-hidden browser? If not, is there some other safety mechanism we can use to fix this issue?
Flags: needinfo?(enndeakin)
Apart from bug fixing 570835 I'm not clear what there is to fix here.
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #11)
> Apart from bug fixing 570835 I'm not clear what there is to fix here.

Thanks for pointing out that bug, I was not aware of it. Fixing that bug would have web impact, from reading the comments it might also break compat with blink/webkit, and it hasn't happened in all the years since the bug was filed. I'm also a bit worried that, if the typing still actually affects the input element and JS in the background tab, clearly it isn't display: none and is still a valid event target, so would it even be covered by a fix for 570835?

Is there something more light-touch that we could do to address this specific problem? If not, it sounds like we should mark this wontfix or dupe it to 570835.
Flags: needinfo?(enndeakin)
(In reply to :Gijs Kruitbosch from comment #12)
> I'm also a bit worried that, if the typing still actually affects the
> input element and JS in the background tab, clearly it isn't display: none
> and is still a valid event target, so would it even be covered by a fix for
> 570835?

The focus would be removed as it is in a deck pane that is no longer visible.

> Is there something more light-touch that we could do to address this
> specific problem? If not, it sounds like we should mark this wontfix or dupe
> it to 570835.

Possibly, but since you have fixed 1313403 I'm still not clear why this is important right now. The tab switching code always changes the focus to the right tab when the tab switch is complete.
Flags: needinfo?(enndeakin)
That said, adding something to handle removing the focus from a node within a deck wouldn't be particularly difficult.
(In reply to Neil Deakin from comment #13)
> > Is there something more light-touch that we could do to address this
> > specific problem? If not, it sounds like we should mark this wontfix or dupe
> > it to 570835.
> 
> Possibly, but since you have fixed 1313403 I'm still not clear why this is
> important right now. The tab switching code always changes the focus to the
> right tab when the tab switch is complete.

It would be an additional safeguard against our code being fragile in ways we may not know yet, against add-ons acting up, etc. That said, I wouldn't call it particularly urgent, and focusing on fixing bug 570835 seems preferable to me. Bug 570835 comment 5 suggests that there would be problems if this were implemented the wrong way, but if done right it might be viable.
Depends on: 570835
I'll see if I can get some traction on bug 570835 and mark this bug as a P3.
Priority: -- → P3
FWIW, bz told me he doesn't think bug 570835 is related here.
Mass wontfix for bugs affecting firefox 52.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.