The default bug view has changed. See this FAQ.

decomtaminate GetFocusedChild()

RESOLVED FIXED in mozilla8

Status

()

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

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla8
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 546284 [details] [diff] [review]
patch
Attachment #546284 - Flags: review?(surkov.alexander)

Comment 2

6 years ago
Comment on attachment 546284 [details] [diff] [review]
patch

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

btw, please use new FocusedChild() on platform layer (linux, windows and mac)

::: accessible/src/base/nsAccessible.cpp
@@ +770,5 @@
> +nsAccessible*
> +nsAccessible::FocusedChild()
> +{ 
> +  if (!mContent)
> +    return nsnull;

why is that for?

@@ +775,5 @@
> +
> +  if (gLastFocusedNode == mContent)
> +    return this;
> +
> +    nsAccessible* focusedChild = GetAccService()->GetAccessible(gLastFocusedNode);

I think I prefer GetDocAccessible()->GetAccessible()

@@ +778,5 @@
> +
> +    nsAccessible* focusedChild = GetAccService()->GetAccessible(gLastFocusedNode);
> +
> +    // XXX this doesn't consider the focused child being a grand child of this
> +    // or focusedChild == this althought I'm not sure the second is actually possible

it appears AT happy with this, if they need a child from subtree then they call it on document accessible, otherwise doing this is perf problem. Having said that I think we should do exception for ARIA documents, please file bug on this

::: accessible/src/base/nsAccessible.h
@@ +127,5 @@
>  
>    /**
> +   * Return the focused child if any.
> +   */
> +  virtual nsAccessible* FocusedChild();

let's get an agreement how we group methods, I'd suggest to list properies on top (like role, name and etc) and then accessible getters (like focused, child from point)

::: accessible/src/base/nsDocAccessible.cpp
@@ +359,4 @@
>  
>    // Return an accessible for the current global focus, which does not have to
>    // be contained within the current document.
> +  return GetAccService()->GetAccessible(gLastFocusedNode);

maybe inline it? like

return gLastFocusedNode ? GetAcccessible(gLastFocusedNode) : nsnull;
Attachment #546284 - Flags: review?(surkov.alexander)
(Assignee)

Comment 3

6 years ago
Created attachment 547802 [details] [diff] [review]
patch v2
Attachment #546284 - Attachment is obsolete: true
Attachment #547802 - Flags: review?(surkov.alexander)
Comment on attachment 547802 [details] [diff] [review]
patch v2

>+nsAccessible*
>+nsAccessible::FocusedChild()
>+{ 
>+  if (gLastFocusedNode == mContent)
>+    return this;
>+
>+    nsAccessible* focusedChild = GetDocAccessible()->GetAccessible(gLastFocusedNode);
>+    if (!focusedChild || focusedChild->GetParent() != this)
>+      return nsnull;

Indentation is weird here.
(Assignee)

Comment 5

6 years ago
Created attachment 547811 [details] [diff] [review]
patch v2

thanks, didn't notice that.
Attachment #547802 - Attachment is obsolete: true
Attachment #547811 - Flags: review?(surkov.alexander)
Attachment #547802 - Flags: review?(surkov.alexander)

Comment 6

6 years ago
Comment on attachment 547811 [details] [diff] [review]
patch v2

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

::: accessible/src/base/nsAccessible.cpp
@@ +758,5 @@
>  NS_IMETHODIMP
> +nsAccessible::GetFocusedChild(nsIAccessible** aChild)
> +{
> +  NS_ENSURE_ARG_POINTER(aChild);
> +  *aChild = nsnull;

nit: new line please

@@ +772,5 @@
> +{ 
> +  if (gLastFocusedNode == mContent)
> +    return this;
> +
> +  nsAccessible* focusedChild = GetDocAccessible()->GetAccessible(gLastFocusedNode);

I'd prefer to have gLastFocusedNode null check before getting an accessible

::: accessible/src/base/nsDocAccessible.cpp
@@ +357,4 @@
>  
>    // Return an accessible for the current global focus, which does not have to
>    // be contained within the current document.
> +  return gLastFocusedNode ? GetAccessible(gLastFocusedNode) : nsnull;

I know I suggseted this but GetAccessible() contradicts with comment above, please get back to AccService() usage

::: accessible/src/xul/nsXULTreeGridAccessible.h
@@ +160,5 @@
>    virtual PRBool Init();
>    virtual bool IsPrimaryForNode() const;
>  
>    // nsAccessible
> +  virtual nsAccessible* FocusedChild();

please keep accessible getters after property getters
Attachment #547811 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 547894 [details] [diff] [review]
patch

Comment 8

6 years ago
Comment on attachment 547894 [details] [diff] [review]
patch

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

otherwise looks good

::: accessible/src/base/nsAccessible.cpp
@@ +773,5 @@
> +{ 
> +  if (gLastFocusedNode == mContent)
> +    return this;
> +  if (!gLastFocusedNode)
> +    return nsnull;

it's cleaner when you check !gLastFocusedNode prior to gLastFocusedNode == mContent check
(Assignee)

Comment 9

6 years ago
landed http://hg.mozilla.org/integration/mozilla-inbound/rev/2d6026252d03
http://hg.mozilla.org/mozilla-central/rev/2d6026252d03
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8

Updated

6 years ago
Depends on: 677467
You need to log in before you can comment on or make changes to this bug.