352220 - Inconsistent focus events when returning to a document frame

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[parente]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060911 Minefield/3.0a1

Assume an item in a FF document frame (e.g. a link) has focus. If the user switches away from the FF window and then back again, the following events are fired:

window:activate (on the FF window)
focus: (on the document frame)
focus: (on the link)

If, instead, the user switches focus to a menu within FF and then escapes out of the menu, the following events are fired:

focus: (on the link only)

It seems like these two cases should be consistent. Since we wouldn't want a focus on the document frame after every focus change within the document, it probably makes sense to to remove the focus on the document frame when reactivating a FF window. This approach will also make FF consistent with gtk which only fires one focus event when an application is reactivated. If an AT wants to determine if the element receiving focus is inside a document, it can either 1) traverse parent pointers or 2) use the getDocument call.

Comment 1

[Aaron Leventhal]

I'm not sure whether to prioritize this for Firefox 3 or not.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Mass un-assigning bugs assigned to Aaron.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[alexander :surkov (:asurkov)]

try build will be available here - http://build.mozilla.org/tryserver-builds/[e-mail]-try-9aa410316650

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 432877 [details] [diff] [review]
patch

r=me; with a small bit of whining, and a compliment.

As long as Marco's whirling doesn't find any problems.

>+
>+    // eCoalesceFromSameDocument : For events of the same type from the same
>+    //    document, only the newest event will be emitted.
>+    eCoalesceFromSameDocument,
>+

OK. Is this different from dupes?

>+    case nsAccEvent::eCoalesceFromSameDocument:
>+    {
>+      for (PRInt32 index = 0; index < tail; index ++) {
>+        nsAccEvent* thisEvent = mEvents[index];
>+        if (thisEvent->mEventType == tailEvent->mEventType &&
>+            thisEvent->mEventRule == tailEvent->mEventRule &&
>+            thisEvent->GetDocAccessible() == tailEvent->GetDocAccessible()) {
>+          thisEvent->mEventRule = nsAccEvent::eDoNotEmit;
>+        }
>+      }
>+    } break; // case eCoalesceFromSameDocument
>+

The pain here is that we are traversing the list of events yet again (performance). We really need some talos tests.


>-  nsCOMPtr<nsIAccessibleDocument> docAccessible = do_QueryInterface(finalFocusAccessible);
>-  if (docAccessible) {
>-    // Doc is gaining focus, but actual focus may be on an element within document
>-    nsCOMPtr<nsIDOMNode> realFocusedNode = GetCurrentFocus();
>-    if ((realFocusedNode != aNode || realFocusedNode == mDOMNode) &&
>-        !(nsAccUtils::ExtendedState(finalFocusAccessible) &
>-                    nsIAccessibleStates::EXT_STATE_EDITABLE)) {
>-      // Suppress document focus, because real DOM focus will be fired next,
>-      // except in the case of editable documents because we can't rely on a
>-      // followup focus event for an element in an editable document.
>-      // Make sure we never fire focus for the nsRootAccessible (mDOMNode)
>-
>-      // XXX todo dig deeper on editor focus inconsistency in bug 526313
>-
>-      return PR_FALSE;
>-    }
>-  }

This on its own is worth a steak dinner.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> >+    eCoalesceFromSameDocument,
> >+
> 
> OK. Is this different from dupes?

Dupes, if nodes are the same. Here is if they are in the same document.

> >+    case nsAccEvent::eCoalesceFromSameDocument:
> >+    {
> >+      for (PRInt32 index = 0; index < tail; index ++) {
> >+        nsAccEvent* thisEvent = mEvents[index];
> >+        if (thisEvent->mEventType == tailEvent->mEventType &&
> >+            thisEvent->mEventRule == tailEvent->mEventRule &&
> >+            thisEvent->GetDocAccessible() == tailEvent->GetDocAccessible()) {
> >+          thisEvent->mEventRule = nsAccEvent::eDoNotEmit;
> >+        }
> >+      }
> >+    } break; // case eCoalesceFromSameDocument
> >+
> 
> The pain here is that we are traversing the list of events yet again
> (performance). We really need some talos tests.

No pain and no performance. We fired focus event with eRemoveDupes. So we traverse them any way. Additionally I compare documents but since I don't expect more than two focus events in the queue then I don't expect any perf regressions.

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 432877 [details] [diff] [review]
patch

I don't see any change in behaviour that would be negatively noticeable. Also, r=me for the test and the patch with manual testing of the try-server build.

Comment 8

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/11798edae90d

Comment 9

[alexander :surkov (:asurkov)]

in-testsuite+ since patch have a mochitest