904865 - Clear focus in child when focusing chrome

Status: RESOLVED DUPLICATE of bug 1176239

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

Description

[Tom S [:evilpie]]

Attachment: clear-focus

This bug fixes the issue remaining from Bug #893201. When something is focused in content and we then focus something in chrome, we don't blur the focus of the focused content element, but just deactivate the browser.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

With this ClearFocus does that mean that the focus also go back to the first element when you tab again into content?

Comment 2

[Tom S [:evilpie]]

Yeah that is the basic idea.

Comment 3

[Neil Deakin]

The current behaviour is to reset the tab navigation when one tabs into the frame, but this patch looks to reset it when one leaves.

Comment 4

[Tom S [:evilpie]]

Interesting, but I understand it differently. When we focus something with the mouse, the old element is blurred. With this code I basically want to blur the focused element in the child, when an element in chrome gets focused. I think that is actually quite similar to non-e10s behavior.

Comment 5

[Neil Deakin]

No, the focused element within the child frame is not changed when a parent is focused. This means that childframe.document.activeElement maintains the same value.

You can set this with a simple test:

<script>
setInterval(() => dump(frames[0].document.activeElement + '\n'), 1000);
</script>
<iframe width="300" height="300" src="http://www.mozilla.org"/>

Comment 6

[Tom S [:evilpie]]

Comment on attachment 789845 [details] [diff] [review]
clear-focus

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

Okay, I see what you mean now. I will try to find a solution, but it would be interesting to now if you already have an idea on how to fix this?

Comment 7

[Tom S [:evilpie]]

Neil, do you know how to implement this properly? I am still interested in fixing this.

Comment 8

[Neil Deakin]

Is this only needed when tabbing into/out of the parent into the child, or other times as well?

For the former, you may need to implement nsIWebBrowserFocus::setFocusAtFirstElement and  nsIWebBrowserFocus::setFocusAtLastElement) or something similar.

Comment 9

[Tom S [:evilpie]]

I have only noticed that with tabbing, but there might be something else that I am not aware of.

Comment 10

[Tom S [:evilpie]]

Attachment: focus-fx

This seem to have not the issue that the testcase tested for.

Comment 11

[Tom S [:evilpie]]

Attachment: v2

I just realized that tabbing backwards into the child would not work. We now have a MoveFocus method in both the parent and child.

Comment 12

[Neil Deakin]

>     // for caret movement, pass false for the aFocusChanged argument,
>     // otherwise the caret will end up moving to the focus position. This
>     // would be a problem because the caret would move to the beginning of the
>     // focused link making it impossible to navigate the caret over a link.
>     SetFocusInner(newFocus, aFlags, aType != MOVEFOCUS_CARET, true);
>+    if (TabParent* remote = TabParent::GetFrom(newFocus)) {
>+      remote->SendMoveFocus(aType == MOVEFOCUS_BACKWARD ? MOVEFOCUS_LAST
>+                                                        : MOVEFOCUS_ROOT);

MOVEFOCUS_ROOT is subtly different that what the tabbing behaviour does. The former does nothing if the root isn't focusable, such as a non-visible tab or a <frameset> page (see GetRootForFocus), whereas the latter moves on to the next element in sequence.

I think what you actually want to do is change the code at http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#2745 (Within nsFocusManager::GetNextTabbableContent, at the line that calls GetSubDocumentFor). A subdocument won't be returned, but if currentContent is a remote browser frame, call into the remote child to perform navigation, which would effectively have to do the same thing as the code that handles a subdocument there. If it doesn't have anything to tab to, it needs to indicate this to the caller so that it can continue on to the next element.

The other issue is that you need to handle document navigation as well (F6 / Shift+F6), but that could be a followup bug.

>+
>+bool
>+TabChild::RecvMoveFocus(const uint32_t& aType)
>+{
>+  nsCOMPtr<nsIFocusManager> fm = do_GetService(FOCUSMANAGER_CONTRACTID);

You can just use nsFocusManager::GetFocusManager() in a file where nsFocusManager.h is included.


>-TabParent::RecvMoveFocus(const bool& aForward)
>+TabParent::RecvMoveFocus(const uint32_t& aType)
> {
>   nsCOMPtr<nsIFocusManager> fm = do_GetService(FOCUSMANAGER_CONTRACTID);

You could change this to do the same here as well.

Comment 13

[Tom S [:evilpie]]

>If it doesn't have anything to tab to, it needs to indicate this to the caller so that it can continue on to the next >element.
This sounds like you want to make a synchronous call into content? I think what we can do is, if we have a remote browser, we always tab into it. But if there is nothing to focus we immediately focus the next thing in chrome.

Comment 14

[Tom S [:evilpie]]

It's very hard to tell what needs to be done, so I rather not work on this from home. Maybe dvander can work on this after he already finished some other focus bug.

Comment 15

[Tom S [:evilpie]]

Comment on attachment 804586 [details] [diff] [review]
v2

I hope David is going to take care of this.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'll take this for now so it doesn't get lost.

Comment 17

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 18

[Neil Deakin]

Think this is covered by 1132518

Comment 19

[Neil Deakin]

I meant 1176239.