Closed Bug 904865 Opened 11 years ago Closed 9 years ago

Clear focus in child when focusing chrome

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1176239
Tracking Status
e10s + ---

People

(Reporter: evilpie, Assigned: billm)

References

Details

(Whiteboard: [e10s])

Attachments

(1 file, 2 obsolete files)

Attached patch clear-focus (obsolete) — Splinter Review
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.
Attachment #789845 - Flags: review?(enndeakin)
With this ClearFocus does that mean that the focus also go back to the first element when you tab again into content?
Yeah that is the basic idea.
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.
Summary: Cross chrome/content focus handling → Clear focus in child when focusing chrome
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.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
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 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?
Attachment #789845 - Flags: review?(enndeakin)
Neil, do you know how to implement this properly? I am still interested in fixing this.
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
I have only noticed that with tabbing, but there might be something else that I am not aware of.
Attached patch focus-fx (obsolete) — Splinter Review
This seem to have not the issue that the testcase tested for.
Attachment #789845 - Attachment is obsolete: true
Attachment #803189 - Flags: review?(enndeakin)
OS: Linux → All
Hardware: x86_64 → All
Attached patch v2Splinter Review
I just realized that tabbing backwards into the child would not work. We now have a MoveFocus method in both the parent and child.
Attachment #803189 - Attachment is obsolete: true
Attachment #803189 - Flags: review?(enndeakin)
Attachment #804586 - Flags: review?(enndeakin)
>     // 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.
>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.
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.
Assignee: evilpies → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(dvander)
Comment on attachment 804586 [details] [diff] [review]
v2

I hope David is going to take care of this.
Attachment #804586 - Flags: review?(enndeakin)
Flags: needinfo?(dvander)
I'll take this for now so it doesn't get lost.
Assignee: nobody → wmccloskey
No longer blocks: e10s-it1
Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".
tracking-e10s: --- → +
Think this is covered by 1132518
I meant 1176239.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: