Closed
Bug 1456588
Opened 3 years ago
Closed 3 years ago
Switch nsFocusManager to dealing with Element instead of nsIContent
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(6 files, 1 obsolete file)
18.23 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
3.52 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
20.92 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
5.10 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
3.32 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
45.35 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
Pretty sure in practice mFocusedContent is always an Element. We should make that clearer.
![]() |
Assignee | |
Comment 1•3 years ago
|
||
Attachment #8970765 -
Flags: review?(enndeakin)
![]() |
Assignee | |
Updated•3 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 2•3 years ago
|
||
Attachment #8970766 -
Flags: review?(enndeakin)
![]() |
Assignee | |
Comment 3•3 years ago
|
||
Attachment #8970767 -
Flags: review?(enndeakin)
![]() |
Assignee | |
Comment 4•3 years ago
|
||
I couldn't find a good way to make this incremental without adding QIs and AsElement() in various places....
Attachment #8970768 -
Flags: review?(enndeakin)
![]() |
Assignee | |
Comment 5•3 years ago
|
||
Attachment #8970769 -
Flags: review?(enndeakin)
![]() |
Assignee | |
Comment 6•3 years ago
|
||
Attachment #8970770 -
Flags: review?(enndeakin)
![]() |
Assignee | |
Comment 7•3 years ago
|
||
Attachment #8970806 -
Flags: review?(enndeakin)
![]() |
Assignee | |
Updated•3 years ago
|
Attachment #8970765 -
Attachment is obsolete: true
Attachment #8970765 -
Flags: review?(enndeakin)
Updated•3 years ago
|
Priority: -- → P2
Comment 8•3 years ago
|
||
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•3 years ago
|
Attachment #8970766 -
Flags: review?(enndeakin) → review+
Updated•3 years ago
|
Attachment #8970767 -
Flags: review?(enndeakin) → review+
Comment 9•3 years ago
|
||
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•3 years ago
|
Attachment #8970769 -
Flags: review?(enndeakin) → review+
Updated•3 years ago
|
Attachment #8970770 -
Flags: review?(enndeakin) → review+
![]() |
Assignee | |
Comment 10•3 years ago
|
||
> 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!
![]() |
Assignee | |
Updated•3 years ago
|
Comment 11•3 years ago
|
||
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
Comment 12•3 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•2 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•