Closed Bug 552368 Opened 15 years ago Closed 15 years ago

fire focus event on document accessible whenever the root or body element is focused

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file)

the part of the bug 550338 (see 1-4 steps to reproduce).
Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #433080 - Flags: review?(marco.zehe)
Attachment #433080 - Flags: review?(bolterbugz)
Attachment #433080 - Flags: review?(bolterbugz) → review+
Comment on attachment 433080 [details] [diff] [review]
patch

r=me if Marco and tryserver give it a thumbs up. One small nit:

>diff --git a/accessible/src/base/nsRootAccessible.cpp b/accessible/src/base/nsRootAccessible.cpp
>--- a/accessible/src/base/nsRootAccessible.cpp
>+++ b/accessible/src/base/nsRootAccessible.cpp
>@@ -545,17 +545,31 @@ nsRootAccessible::FireCurrentFocusEvent(
>                                            getter_AddRefs(event))) &&
>         NS_SUCCEEDED(event->InitEvent(NS_LITERAL_STRING("focus"), PR_TRUE, PR_TRUE))) {
>       // Get the target node we really want for the event.
>       nsIAccessibilityService* accService = GetAccService();
>       if (accService) {
>         nsCOMPtr<nsIDOMNode> targetNode;
>         accService->GetRelevantContentNodeFor(focusedNode,
>                                             getter_AddRefs(targetNode));
>+
>         if (targetNode) {
>+          // If the focused element is document element or HTML body element
>+          // then simulate the focus event for the document.
>+          nsCOMPtr<nsIContent> targetContent(do_QueryInterface(targetNode));
>+          if (targetContent) {
>+            nsCOMPtr<nsIDOMNode> document =
>+              do_QueryInterface(targetContent->GetOwnerDoc());

Might as well write:
nsCOMPtr<nsIDOMNode> document(do_QueryInterface(targetContent->GetOwnerDoc());

To match the style you use for intializing targetContent.

>+            if (targetContent == nsCoreUtils::GetRoleContent(document)) {
>+              HandleEventWithTarget(event, document);
>+              return;
>+            }
>+          }
>+
>+          // Otherwise simulate the focus event for currently focused node.
>           HandleEventWithTarget(event, targetNode);
>         }
>       }
>     }
>   }
> }
> 
> ////////////////////////////////////////////////////////////////////////////////
>diff --git a/accessible/tests/mochitest/events.js b/accessible/tests/mochitest/events.js
>--- a/accessible/tests/mochitest/events.js
>+++ b/accessible/tests/mochitest/events.js
>@@ -594,28 +594,48 @@ function sequence()
>  */
> function synthClick(aNodeOrID, aChecker, aEventType)
> {
>   this.__proto__ = new synthAction(aNodeOrID, aChecker, aEventType);
> 
>   this.invoke = function synthClick_invoke()
>   {
>     // Scroll the node into view, otherwise synth click may fail.
>-    this.DOMNode.scrollIntoView(true);
>+    if (this.DOMNode instanceof nsIDOMNSHTMLElement)
>+      this.DOMNode.scrollIntoView(true);

What happens if not an instanceof nsIDOMNSHTMLElement?

> /**
>+ * Mouse move invoker.
>+ */
>+function synthMouseMove(aNodeOrID, aChecker, aEventType)
>+{
>+  this.__proto__ = new synthAction(aNodeOrID, aChecker, aEventType);
>+
>+  this.invoke = function synthMouseMove_invoke()
>+  {
>+    synthesizeMouse(this.DOMNode, 1, 1, { type: "mousemove" });
>+    synthesizeMouse(this.DOMNode, 2, 2, { type: "mousemove" });

Is that always enough to trigger the move handler?

>+function synthFocusOnFrame(aNodeOrID, aChecker, aEventType)

Please add a small comment for this function, maybe mention it focuses the body. Maybe rename this to synthFocusOnBody?

The rest looks good to me.
(In reply to comment #2)
> (From update of attachment 433080 [details] [diff] [review])

> Might as well write:
> nsCOMPtr<nsIDOMNode> document(do_QueryInterface(targetContent->GetOwnerDoc());

> To match the style you use for intializing targetContent.

I think this is because of 80 chars per line restriction.

> >   this.invoke = function synthClick_invoke()
> >   {
> >     // Scroll the node into view, otherwise synth click may fail.
> >-    this.DOMNode.scrollIntoView(true);
> >+    if (this.DOMNode instanceof nsIDOMNSHTMLElement)
> >+      this.DOMNode.scrollIntoView(true);
> 
> What happens if not an instanceof nsIDOMNSHTMLElement?

no scroll into view :) Actually we don't have a case in mochitests where it's needed for XUL elements. Therefore I didn't support the XUL case by special code.

> >+    synthesizeMouse(this.DOMNode, 1, 1, { type: "mousemove" });
> >+    synthesizeMouse(this.DOMNode, 2, 2, { type: "mousemove" });
> 
> Is that always enough to trigger the move handler?

I hope so, at least mochitests pass.

> 
> >+function synthFocusOnFrame(aNodeOrID, aChecker, aEventType)
> 
> Please add a small comment for this function, maybe mention it focuses the
> body.

sure, thanks for the catch.

> Maybe rename this to synthFocusOnBody?

I would like if name says the frame element is expected as argument. Probably synthFocusOnFrameDocument? Too long?
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 433080 [details] [diff] [review] [details])
> 
> > Might as well write:
> > nsCOMPtr<nsIDOMNode> document(do_QueryInterface(targetContent->GetOwnerDoc());
> 
> > To match the style you use for intializing targetContent.
> 
> I think this is because of 80 chars per line restriction.

Heh.. okay...


> > Is that always enough to trigger the move handler?
> 
> I hope so, at least mochitests pass.

OK. we can fix if it breaks.

> sure, thanks for the catch.

yep

> 
> > Maybe rename this to synthFocusOnBody?
> 
> I would like if name says the frame element is expected as argument. Probably
> synthFocusOnFrameDocument? Too long?

I guess with the comment I don't mind what we call it. I don't think synthFocusOnFrameDocument is too long (these are just tests afterall).
Attachment #433080 - Flags: review?(marco.zehe) → review-
Comment on attachment 433080 [details] [diff] [review]
patch

If I read this correctly, this is supposed to fix steps 1 through 4 of bug 550338, right? Well, it doesn't. Opening Jamie's testcase, navigating to the editable frame between the two buttons and pressing ENTER to set focus to it, then hitting Alt to go to the menu bar, then hitting Alt again to leave the menu bar, still doesn't result in NVDA speaking the now again focused document. I don't see a change in behaviour whatsoever.
Comment on attachment 433080 [details] [diff] [review]
patch

OK after explanation on IRC that this patch *might not* fix Jamie's bug and that that one has wider implications, r=me for this one.
Attachment #433080 - Flags: review- → review+
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/ce72ac91f1ad
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: