1456588 - Switch nsFocusManager to dealing with Element instead of nsIContent

Status: RESOLVED FIXED

Description

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Pretty sure in practice mFocusedContent is always an Element.  We should make that clearer.

Comment 1

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 1. Change nsIFocusManager::SetFocus to take Element

Comment 2

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 2. Change nsIFocusManager::MoveFocus to take Element

Comment 3

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 3. Change nsFocusManager::SetFocusInner to take Element

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 4. Change nsFocusManager guts to make it clearer that the focused thing is always an Element

I couldn't find a good way to make this incremental without adding QIs and
AsElement() in various places....

Comment 5

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 5. Store an Element as the focused content in the focus manager

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 6. Change nsXULCommandDispatcher::GetRootFocusedContentAndWindow to return Element

Comment 7

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: part 1. Change nsIFocusManager::SetFocus to take Element

Comment 8

[Neil Deakin]

Comment on attachment 8970806 [details] [diff] [review]
part 1.  Change nsIFocusManager::SetFocus to take Element

>     AutoHandlingUserInputStatePusher inputStatePusher(true, nullptr, focusContent->OwnerDoc());
>-    nsCOMPtr<nsIDOMElement> element(do_QueryInterface(focusContent));
>+    // XXXbz: Can we actually have a non-element content here?
>+    RefPtr<Element> element =
>+      focusContent->IsElement() ? focusContent->AsElement() : nullptr;
>     fm->SetFocus(element, 0);


Not sure about accessible code whether focusContent could be a non-element.
 
>-            nsCOMPtr<nsIDOMElement> elem = do_QueryInterface(this);
>-            fm->SetFocus(elem, nsIFocusManager::FLAG_BYMOUSE |
>-                               nsIFocusManager::FLAG_NOSCROLL);
>+            // XXXbz do we need the strong ref here?
>+            RefPtr<Element> kungFuDeathGrip(this);
>+            fm->SetFocus(kungFuDeathGrip, nsIFocusManager::FLAG_BYMOUSE |
>+                                          nsIFocusManager::FLAG_NOSCROLL);


While events are sent in scriptrunners if unsafe, I think we should leave the strong references in for now.


>   // the element within the document that is currently focused when this
>-  // window is active
>+  // window is active.
>   nsCOMPtr<nsIContent> mFocusedNode;

Doesn't matter too much, but this cosmetic change should go in the later patch that changes this file.


>diff --git a/toolkit/components/typeaheadfind/nsITypeAheadFind.idl b/toolkit/components/typeaheadfind/nsITypeAheadFind.idl
>--- a/toolkit/components/typeaheadfind/nsITypeAheadFind.idl
>+++ b/toolkit/components/typeaheadfind/nsITypeAheadFind.idl

The typeaheadfind changes seem like they are part of a different bug. There are some in part 2 as well.

Comment 9

[Neil Deakin]

Comment on attachment 8970768 [details] [diff] [review]
part 4.  Change nsFocusManager guts to make it clearer that the focused thing is always an Element

We should have a followup bug to rename references to currentContent, focusedContent and the like to refer to elements rather than content.

Similarly, followup to change nsPIDOMWindow::mFocusedNode to become mFocusedElement (and Get/SetFocusedElement)

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> Not sure about accessible code whether focusContent could be a non-element.

Yeah, me neither...

> While events are sent in scriptrunners if unsafe, I think we should leave the strong references in for now.

OK.  I will take out the XXX comment.

> Doesn't matter too much, but this cosmetic change should go in the later patch that changes this file.

Done.

> The typeaheadfind changes seem like they are part of a different bug.

They're here because nsTypeAheadFind does:

              fm->SetFocus(mFoundEditable, 0);

so mFoundEditable needed to become an Element when I changed SetFocus, which needed changes to the code touching mFoundEditable.  I could have split this up into more fine-grained changesets with temporary QIs introduced and then removed, but it seemed like a small enough change to not be worth the trouble.

> There are some in part 2 as well.

Right, nsTypeAheadFind passes getter_AddRefs(mFoundLink) to MoveFocus.

> We should have a followup bug to rename references

Filed bug 1457155 and bug 1457156.  Thank you for prodding me on this!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4ed3bc2cd85
part 1.  Change nsIFocusManager::SetFocus to take Element.  r=enndeakin
https://hg.mozilla.org/integration/mozilla-inbound/rev/73369777ac65
part 2.  Change nsIFocusManager::MoveFocus to take Element.  r=enndeakin
https://hg.mozilla.org/integration/mozilla-inbound/rev/927fe6220136
part 3.  Change nsFocusManager::SetFocusInner to take Element.  r=enndeakin
https://hg.mozilla.org/integration/mozilla-inbound/rev/e419e7f99479
part 4.  Change nsFocusManager guts to make it clearer that the focused thing is always an Element.  r=enndeakin
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64df38f5117
part 5.  Store an Element as the focused content in the focus manager.  r=enndeakin
https://hg.mozilla.org/integration/mozilla-inbound/rev/f240cf49fff8
part 6.  Change nsXULCommandDispatcher::GetRootFocusedContentAndWindow to return Element.  r=enndeakin

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f4ed3bc2cd85
https://hg.mozilla.org/mozilla-central/rev/73369777ac65
https://hg.mozilla.org/mozilla-central/rev/927fe6220136
https://hg.mozilla.org/mozilla-central/rev/e419e7f99479
https://hg.mozilla.org/mozilla-central/rev/b64df38f5117
https://hg.mozilla.org/mozilla-central/rev/f240cf49fff8