Switch nsFocusManager to dealing with Element instead of nsIContent

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
3 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Pretty sure in practice mFocusedContent is always an Element.  We should make that clearer.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
I couldn't find a good way to make this incremental without adding QIs and
AsElement() in various places....
Attachment #8970768 - Flags: review?(enndeakin)
Attachment #8970765 - Attachment is obsolete: true
Attachment #8970765 - Flags: review?(enndeakin)
Priority: -- → P2

Comment 8

Last year
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.
Attachment #8970806 - Flags: review?(enndeakin) → review+

Updated

Last year
Attachment #8970766 - Flags: review?(enndeakin) → review+

Updated

Last year
Attachment #8970767 - Flags: review?(enndeakin) → review+

Comment 9

Last year
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)
Attachment #8970768 - Flags: review?(enndeakin) → review+

Updated

Last year
Attachment #8970769 - Flags: review?(enndeakin) → review+

Updated

Last year
Attachment #8970770 - Flags: review?(enndeakin) → review+
> 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

Last year
Pushed by bzbarsky@mozilla.com:
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.