893201 - Cross chrome/content focus handling

Status: RESOLVED FIXED

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

Description

[Tom S [:evilpie]]

When you tab from chrome until you end up in content, we highlight the first element there. On the next tab however we start from the first element in chrome, because the FocusManager doesn't know that <browser> has elements in it that are focused. We however already send the right events to content, so that this could in theory work. All this patch does is, it forces the chrome process to ignore the MoveFocus request so that content can do the right thing. The other case is when content is basically at the end of focus-able elements and clears focus. However we should actually go back into chrome and focus the next element there. I am not sure if this is right patch, but I find after typing this out, it does seem to make a certain amount of sense. Only thing that confuses me is that shift+tab from content up to chrome works.

Comment 1

[Tom S [:evilpie]]

Oh change-set: http://hg.mozilla.org/projects/larch/rev/ae47a8d24a63

Comment 2

[Tom S [:evilpie]]

Okay there is at least one remaining problem. When something in content is focused, but the user manually focuses something in chrome, they element in content stays focused. I believe the straight forward fix is to explicitly reset the elements that are focused in content in that case.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

(In reply to Tom Schuster [:evilpie] from comment #2)
> Okay there is at least one remaining problem. When something in content is
> focused, but the user manually focuses something in chrome, they element in
> content stays focused. I believe the straight forward fix is to explicitly
> reset the elements that are focused in content in that case.

This is supposed to already work, here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1563

Is that codepath not being hit for some reason?

Comment 4

[Tom S [:evilpie]]

Just deactivating is not enough. We still remember the last focused element and continue from there. Just the highlighting is removed.

Comment 5

[Neil Deakin]

(In reply to Tom Schuster [:evilpie] from comment #0)
> When you tab from chrome until you end up in content, we highlight the first
> element there. On the next tab however we start from the first element in
> chrome, because the FocusManager doesn't know that <browser> has elements in
> it that are focused.

When the content process has the focus, key events should be sent to the child process and MoveFocus called on its focus manager, so the chrome focus manager shouldn't be doing anything.

Are you sure that the content process is being activated properly? This is done at http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1785


> We however already send the right events to content, so
> that this could in theory work. All this patch does is, it forces the chrome
> process to ignore the MoveFocus request so that content can do the right
> thing. The other case is when content is basically at the end of focus-able
> elements and clears focus. However we should actually go back into chrome
> and focus the next element there.

The content process focus manager handles this by calling docShell->TabToTreeOwner().

Didn't all this work before? There shouldn't be any need to change anything in the focus manager.

Comment 6

[Tom S [:evilpie]]

The MoveFocus is obviously ends up in chrome, even when content is focused. Maybe this is related to bug 862519?

Comment 7

[Tom S [:evilpie]]

I now believe that this http://hg.mozilla.org/projects/larch/rev/ee111715a450 is a better fix. When we let content handle the tab event, we shouldn't also in chrome do anything with that event. This fixes the problem and nicely ties in with the existing code that Neil mentioned in comment 5.

Comment 8

[Tom S [:evilpie]]

Even with the last fix there is something that is bugging me.
1. Focus something in content (better not the last or first focusable item)
2. Click on the something in chrome, eg. the urlbar.
3. We call tabchild->deactivate()
4. Press tab until you end up in content.
5. Problem: we continue tabbing from the last focused item in content instead of the beginning.

I tried solving this by also calling ClearFocus(FocusManager->FocusedWindow()). The log also says <<blur>> Element a has been blurred. Sadly I still see the same behavior.

Comment 9

[Tom S [:evilpie]]

I hope you can help me out here. Your last hint was already quite helpful.

Comment 10

[Neil Deakin]

Is this something that used to work?

> I tried solving this by also calling ClearFocus(FocusManager->FocusedWindow()).

Where are you calling this from?

Comment 11

[Tom S [:evilpie]]

(In reply to Neil Deakin from comment #10)
> Is this something that used to work?
What do you mean? I actually don't understand how this ever worked, but I guess the code used to be different. Is there some branch or version I could check out that has this stuff in a working condition?
> 
> > I tried solving this by also calling ClearFocus(FocusManager->FocusedWindow()).
> 
> Where are you calling this from?

Before this.
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1564

I was basically sending a message to the child to do the ClearFocus(FocusManager->FocusedWindow()).

Comment 12

[Neil Deakin]

I would(In reply to Tom Schuster [:evilpie] from comment #11)
> (In reply to Neil Deakin from comment #10)
> > Is this something that used to work?
> What do you mean? I actually don't understand how this ever worked, but I
> guess the code used to be different. Is there some branch or version I could
> check out that has this stuff in a working condition?
> > 

I don't know. You'd have to ask someone who worked on on that.


> > > I tried solving this by also calling ClearFocus(FocusManager->FocusedWindow()).
> > 
> > Where are you calling this from?
> 
> Before this.
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.
> cpp#1564
> 
> I was basically sending a message to the child to do the
> ClearFocus(FocusManager->FocusedWindow()).

That should work, but you should clear it before activating the window, and only when moving the focus (for example, by tabbing). This can be determined with the flags/type passed to MoveFocus and elsewhere.

However, clearing the focus isn't what you want to do anyway. You want to, depending on the flags/type, move focus to the first or last element in the child frame.

Comment 13

[Tom S [:evilpie]]

I found a better fix for this problem. http://hg.mozilla.org/projects/larch/rev/ee111715a450
We don't handle the tab event when we handled it in content. This works without any other code. The problem with not clearing the focus when clicking the urlbar however isn't fixed yet.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Minor nit: please give a more descriptive name to the `dispatched` var as dispatched might have various different meanings in that context. e.g. handledByRemoteProcess, or dispatchedToContentProcess, sentToContentProcess, etc... Take your pick!

Comment 15

[Tom S [:evilpie]]

Attachment: v1 - basic patch

This is basically the commit that I pushed to large, with better comments and names. I still need to fix the previously mentioned issue, but at least tabbing is usable now.

Comment 16

[:Felipe Gomes (needinfo for replies!)]

I think the check+return should be inside the TAB/F6 case, and not unconditional for all keypresses. Unless you found other things that need this?

Comment 17

[Tom S [:evilpie]]

There is no other code in that case, so I doubt that this a problem.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Yeah I know, but this fix is specifically for the focus handling. If someone adds code afterwards in the keypress case then it might not be applicable anymore

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ebc3baa2b9cb