use nsFocusManager::GetFocusManager() more

RESOLVED FIXED in mozilla13

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: jhk)

Tracking

(Blocks: 1 bug)

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
look for cases of nsCOMPtr<nsIFocusManager> fm = do_GetService(NS_FOCUSMANAGER_CONTRACTID) using grep or http://mxr.mozilla.org/mozilla-central/ and replace them with
nsFocusManager* fm = nsFocusManager::GetFocusManager();
(Assignee)

Updated

5 years ago
Assignee: nobody → jigneshhk1992
(Assignee)

Comment 1

5 years ago
Created attachment 595671 [details] [diff] [review]
Patch
Attachment #595671 - Flags: review?(trev.saunders)
(Reporter)

Comment 2

5 years ago
Comment on attachment 595671 [details] [diff] [review]
Patch

r=me for accessible/ part, but I'm not a peer for the rest of these things, so you need to find someone else to review that or only do the a11y part.
Attachment #595671 - Flags: review?(trev.saunders) → review+

Comment 3

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Comment on attachment 595671 [details] [diff] [review]
> Patch
> 
> r=me for accessible/ part, but I'm not a peer for the rest of these things,
> so you need to find someone else to review that or only do the a11y part.

I suspect everything outside of accessible part might be wrong because nsFocusManager::GetFocusManager() doesn't make sure the nsFocusManager instance is created so I assume one of do_GetService calls can do this. We are safe to assume on accessibility side that GetFocusManager is not null because DOM focus happens prior we can touch focus stuff in a11y.
(Assignee)

Comment 4

5 years ago
Created attachment 595997 [details] [diff] [review]
Patch modified for a11y.
Attachment #595671 - Attachment is obsolete: true
Attachment #595997 - Flags: review?(trev.saunders)

Comment 5

5 years ago
Comment on attachment 595997 [details] [diff] [review]
Patch modified for a11y.

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

moving review request to Hub (Trevor agreed to steal review request from him per irc)
Attachment #595997 - Flags: review?(trev.saunders) → review?(hub)
Comment on attachment 595997 [details] [diff] [review]
Patch modified for a11y.

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

r=me

Looks good for checkin
Attachment #595997 - Flags: review?(hub) → review+

Comment 7

5 years ago
Comment on attachment 595997 [details] [diff] [review]
Patch modified for a11y.

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

r=me with comment fixed

::: accessible/src/base/nsAccessNode.cpp
@@ +423,5 @@
>    nsIDOMWindow* win = doc->GetWindow();
>  
>    nsCOMPtr<nsIDOMWindow> focusedWindow;
>    nsCOMPtr<nsIDOMElement> focusedElement;
> +  nsFocusManager* fm = nsFocusManager::GetFocusManager();

please remove nsAccessNode::GetCurrentFocus at all since it's not used
Attachment #595997 - Flags: review+
(Assignee)

Comment 8

5 years ago
Created attachment 596066 [details] [diff] [review]
Removed nsAccessNode::GetCurrentFocus
Attachment #595997 - Attachment is obsolete: true
Comment on attachment 596066 [details] [diff] [review]
Removed nsAccessNode::GetCurrentFocus

Good catch from Alex. Looks good this way too.
Attachment #596066 - Flags: review+

Comment 10

5 years ago
inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/e47e25d0a6c1
https://hg.mozilla.org/mozilla-central/rev/e47e25d0a6c1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.