566103 - reorganize accessible document handling

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

We have several issues related with accessible document handling.

1) Sometimes accessible document has wrong role (I suppose because of earlier document creation)
2) Refresh document event has wrong value of event-from-user-input attribute (there is no way to distinguish if refresh was caused by user or automatically)
3) Some accessible events aren't fired (XUL document was loaded and its doc shell has busy state still): bug 382021
4) State busy=true isn't fired because OnStateChange start_loading reports temporary HTML document so that it's to early to fire busy state change event since right document wasn't created yet: bug 446469, bug 419626, bug 551385
5) Document is created for hidden content (for example if iframe is hidden)

Comment 1

[alexander :surkov (:asurkov)]

it might make sense to keep in mind bug 353519, especially for iframes if they were requested before they were loaded since we don't fire neither buys state change nor document load complete (because it leads to AT confusion). On windows we could use reorder event but we might want to have common solution.

Comment 2

[alexander :surkov (:asurkov)]

bugs that might be affected by reorg: bug 417249, bug 413778, bug 327918, bug 305813.

Comment 3

[alexander :surkov (:asurkov)]

bug 482932 should have been addressed during reorg

Comment 4

[alexander :surkov (:asurkov)]

bumping to major (perhaps it makes sense to bump it to critical) since
accessible document loading processing are cause of memory leaks "in law", i.e.
we don't destroy accessible document tree under certain circumstances (when tab
is closed bug 482932 and I figured our we may create document accessible after
pagehide event) so that we don't loose pointers to these accessibles but we
don't ever use them.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: wip1

Comment 6

[alexander :surkov (:asurkov)]

request blocking1.9.3 per comment #4

Comment 7

[alexander :surkov (:asurkov)]

try build for wip (http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-2d8f3f0be467/)

Marco, could you do some testing?

Comment 8

[Marco Zehe (:MarcoZ)]

I'm primarily seeing problems with content that's being added dynamically after the page loads, for example in a doc load JavaScript event, and when hovering the mouse over something.

For example on twitter.com, when hovering the mouse over one of the tweets, links to reply and retweet appear. With NVDA I do the following:
1. Go to http://twitter.com and sign in with my account.
2. Go to the home page and read the tweets from those I follow.
3. While positioned with the virtual cursor on one of the tweets or nick names, press NVDA+NUMPAD / (that's NUMPAD0 held down, and / pressed and released) to route the mouse pointer to that object under the virtual cursor.

Expected, and the case in a regular build: The dynamically added list below the tweet can now be found with the virtual cursor. That is in a regular build.
Actual in this try-server build: The trigger almost never fires, OR NVDA just doesn't see it.

Also, I've seen occasions where iFrames that normally contain Google Ads are loaded empty. So the frame is there, but no content. I have to forcefully refresh the NVDA or JAWS buffers for them to pick up these changes.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Also, I've seen occasions where iFrames that normally contain Google Ads are
> loaded empty. So the frame is there, but no content. I have to forcefully
> refresh the NVDA or JAWS buffers for them to pick up these changes.

That's ok, I just implemented AdBlock+ functionality natively ;)

I hope this one is better http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4865d7ffc43b/

Could you try it please?

Comment 10

[Damon Sicore (:damons)]

Blocking final as this is a leak.

Comment 11

[Marco Zehe (:MarcoZ)]

Alex, the new try-server build you sent me does not show any improvements in the points I mention. I still can't make the Twitter links appear when simulating a mouse hover with NVDA as described above. No change, this build is not good still.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> Alex, the new try-server build you sent me does not show any improvements in
> the points I mention. I still can't make the Twitter links appear when
> simulating a mouse hover with NVDA as described above. No change, this build is
> not good still.

Is behavior the same for Google Ads? I prepared several try sever builds and might confuse with them.

Comment 13

[Marco Zehe (:MarcoZ)]

Yes, those Google Ads were now always showing in this last try-server build.

Comment 14

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-9c94a1275943

Marco, please look.

Comment 15

[Marco Zehe (:MarcoZ)]

Still the same problems as in the previous one. The twitter actions like "reply" etc. still don't appear automatically when pulling NVDA's mouse emulation over a tweet.

I'm also observing on a site I have to log in that some items which appear without problems in a regular nightly, only appear in NVDA's virtual buffer after I refresh their virtual buffer, with this and all previous try-server builds. This is content that gets added dynamically after the document initially loads. And this is in an iframe also.

Other iframe stuff like google ads now appear (like in the previous try build).

Comment 16

[alexander :surkov (:asurkov)]

just an idea, next try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-2b4cc7b33180/. It'll be available soon.

Comment 17

[Marco Zehe (:MarcoZ)]

With this latest try-server build, all my testcases work! Twitter, the other site I only see the problem when logged in, and iframes.

Comment 18

[alexander :surkov (:asurkov)]

Maroc, you don't realize how I'm happy to hear this! Let's review the patch and test it carefully. Thank you, for really good news.

Comment 19

[alexander :surkov (:asurkov)]

Attachment: patch

let's go

Comment 20

[alexander :surkov (:asurkov)]

Marco, it's necessary to test blocking bugs plus bugs from comment #2 and comment #3.

Comment 21

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

(In reply to comment #19)
> Created an attachment (id=449067) [details]
> patch
> 
> let's go

Any guidance before I start my review?  (Please)

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)

> Any guidance before I start my review?  (Please)

1) Most of document handling happens in new nsAccDocManager class
1.1) it listens webprogress notifications
1.1.1) start document loading to fire reload/busy state change events on old document
1.1.2) stop document loading to fire complete/stop events on new document
1.2) listens DOMContentLoaded event for error pages while they are handled by special way (when they'll fire pageshow event then we can consider to stop webprogress stop notification usage and use pageshow notifications only).
1.3) listens "pagehide" event to shutdown documents
1.4) it creates document accessibles when they are ready to be created (when they're not dying, not temporary, having presshell and etc)
1.5) it keeps all document accessibles in cache and returns them when they were asked

2) Some things are handled outside nsAccDocManager class, when iframe is hidden (presshell is destroyed but DOM document is alive) then outerdocAccessible shutdowns its document accessible by nsAccDocManager notification (later we could consider to change this like bz suggested to make presshell notify us it's going away).

Shortly that's all. Please let me know if you need any additional details.

Comment 23

[alexander :surkov (:asurkov)]

try-server build for the patch - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-6822af10cacb/

Comment 24

[alexander :surkov (:asurkov)]

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

Boris, I really find useful to get you to look at things how we use content and webprogress notifications and things around.

Comment 25

[Marco Zehe (:MarcoZ)]

One difference I notice on Linux is that, when opening a new tab with CTRL+T, I get the following spoken:

Navigation toolbar toolbar
Search bookmarks and history edit combo
Finished loading about:blank document
about:blank HTML content

In a regular nightly, everything from "finished loading..." up to the end is NOT spoken. So for some reason, Orca tells the user that an about:blank document has been loaded. From the last line, it would appear that focus shifts from the awesome bar to the content area, which is NOT actually the case.

And here's some initial feedback from the bugs Alex asked me to look at:
First the dependencies:
bug 308560 - Testcase is 5 years old, no longer reproducible. Current bt.com page loads fine. Suggest to close WFM.
Bug 382021 - Can someone test this with Accerciser?
Bug 419626 - no way to test with AT really. Aaron even says that in the bug description. Can someone tell through a debugger or the like?
Bug 446469 - Orca's announcements in both cases are identical (both cases being the try-server build versus the regular nightly of Minefield).
bug 482932 - Ginn, can you test this with your debugging magic?
Bug 551385 - no testcase, am not sure how to test.

And now the bugs from comments:
Bug 353519 - Not sure what to do about this.
Bug 417249 - Ginn, since you were able to repro this, can you see whether the try-server build regresses the specific scenarios again?
Bug 413778 - I don't see a reproducible testcase there and am not sure this can be tested without debugging.
Bug 327918 - Couldn't dig up an equivalent testcase up-front yet, might be worth to create a mochitest for this one.
Bug 305813 - still works fine for me.

Joanie and Ginn, the try-server builds for this bug are here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-6822af10cacb/

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> One difference I notice on Linux is that, when opening a new tab with CTRL+T, I
> get the following spoken:
> 
> Navigation toolbar toolbar
> Search bookmarks and history edit combo
> Finished loading about:blank document
> about:blank HTML content
> 
> In a regular nightly, everything from "finished loading..." up to the end is
> NOT spoken. So for some reason, Orca tells the user that an about:blank
> document has been loaded.

Ok, I think I can add about:blank ignorance like we have now. This ignorance was introduced because about:blank are often temporary. This patch filters temporary about:blank so I thought it might make sense to fire events for not temporary about:blanks. However is it ok if you type about:blank into awsomebar and you don't get document load events?

Comment 27

[Marco Zehe (:MarcoZ)]

When typing about:blank and pressing ENTER, the focus actually gets set to the document, that's OK. The one problem is the case where we open a blank tab or window where we now get this announcement. I am wondering, since this is not a problem on Windows, whether this filtering could be done on Orca's side instead.

Joanie, any thoughts?

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> When typing about:blank and pressing ENTER, the focus actually gets set to the
> document, that's OK.

If it's ok on the current build with Orca and Windows screen readers then we could not fire docload events for about:blank at all. However this is likely a hack on our side.

Comment 29

[Marco Zehe (:MarcoZ)]

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

>+nsAccDocManager::ShutdownDocAccessiblesInTree(nsIDocument *aDocument)
>+{
>+  nsCOMPtr<nsISupports> supports = aDocument->GetContainer();

Can we find a better name for this local var than "supports"?

>+  nsCOMPtr<nsIDocShellTreeItem> treeItem = do_QueryInterface(supports);
>+  ShutdownDocAccessiblesInTree(treeItem, aDocument);
>+}

Because that other variable has a useful name as well.

>+  // Fire reload and state busy events on existing document accessible while
>+  // event from user input flag can be calculated properly and accessible
>+  // is alive. When new document gets loading then this one is destroyed.

Nit: "When new document gets loaded ..."

>+    // Fire reload event.
>+    nsRefPtr<nsAccEvent> reorderEvent =
>+      new nsAccEvent(nsIAccessibleEvent::EVENT_DOCUMENT_RELOAD, docAcc);

Nit: Call this variable "reloadEvent" please.

>+  // Document accessible can be created before we notified the DOM document was
>+  // loaded completely. However if it's not created yet then create it.

Nit: "...we were notified ..."

>+   *Shutdown the document accessible.

Nit: Missing space after the *.

Left off at nsAccEvent.cpp

Comment 30

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

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


>+void
>+nsAccDocManager::ShutdownDocAccessiblesInTree(nsIDocument *aDocument)
>+{

ShutdownDocumentAndSubdocumentAccessibles


>+    // If end consumer has been retargeted for loaded content then do not fire
>+    // any event because it means no new document has been loaded, for example,
>+    // it happens when user clicks on file link.
>+    if (aRequest) {
>+      PRUint32 loadFlags = 0;
>+      aRequest->GetLoadFlags(&loadFlags);
>+      if (loadFlags & nsIChannel::LOAD_RETARGETED_DOCUMENT_URI)
>+        eventType = 0;
>+    }

Please ensure we have a test for this.

>+  // Document loading was started.
>+  NS_LOG_ACCDOCLOAD("start document loading", aWebProgress, aRequest,
>+                    aStateFlags)
>+
>+  if (!IsEventTargetDocument(document))

When does this happen?


>+NS_IMETHODIMP
>+nsAccDocManager::HandleEvent(nsIDOMEvent *aEvent)
>+{
>+  nsAutoString type;
>+  aEvent->GetType(type);
>+
>+  nsCOMPtr<nsIDOMEventTarget> target;
>+  aEvent->GetTarget(getter_AddRefs(target));
>+
>+  nsCOMPtr<nsIDocument> document(do_QueryInterface(target));
>+  NS_ASSERTION(document, "pagehide or DOMContentLoaded for non document!");
>+  if (!document)
>+    return NS_OK;
>+
>+  if (type.EqualsLiteral("pagehide")) {
>+    // 'pagehide' event is registered on every DOM document we create an
>+    // accessible for, process the event for the target, this document

please change "target, this" to "target. This" - maybe start "This" on a new line.

>+    // accessible and all its sub document accessible are shutdown as result of
>+    // processing.
>+
>+    NS_LOG_ACCDOCDESTROY("handled 'pagehide' event", document)

Please change "handled" to "received".

>+void
>+nsAccDocManager::HandleDOMDocumentLoad(nsIDocument *aDocument,
>+                                       PRUint32 aLoadEventType,
>+                                       PRBool aMarkAsLoaded)

[left off here]

Comment 31

[Marco Zehe (:MarcoZ)]

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

>+        try {
>+          var event = aEvent.QueryInterface(nsIAccessibleStateChangeEvent);
>+          is

What happened there? :)

Other than that and the nits from before, r=me.

Comment 32

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

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


>+  // Document accessible can be created before we notified the DOM document was

'are' notified.

>+  // Do not fire document complete/stop events for root chrome document
>+  // accessibles and for frame/iframe documents because
>+  // a) screen readers start working on focus event in the case of root chrome
>+  // documents
>+  // b) document load event on sub documents causes screen readers to act is if
>+  // entire page is reloaded.
>+  if (!IsEventTargetDocument(aDocument)) {
>+    // XXX: AT doesn't update their virtual buffer once frame is loaded and it
>+    // has dynamic content added after frame load. There's something wrong how
>+    // we handle this changes.

Please file and reference the bug #.

>+  // Fire complete/load stopped if the load event type is given.
>+  nsCOMPtr<nsIDOMNode> DOMNode = do_QueryInterface(aDocument);
>+  if (aLoadEventType) {
>+    nsRefPtr<nsAccEvent> loadEvent = new nsAccEvent(aLoadEventType, DOMNode);
>+    docAcc->FireDelayedAccessibleEvent(loadEvent);
>+  }
>+

Why do we always file the busy event after this? (below)

>+  // Fire busy state change event.
>+  nsRefPtr<nsAccEvent> stateEvent =
>+    new nsAccStateChangeEvent(DOMNode, nsIAccessibleStates::STATE_BUSY,
>+                              PR_FALSE, PR_FALSE);
>+  docAcc->FireDelayedAccessibleEvent(stateEvent);

>+  NS_LOG_ACCDOCCREATE_TEXT("  add 'pagehide' listener")

>+    NS_LOG_ACCDOCCREATE_TEXT("  add 'DOMContentLoaded' listener")

Should probable use past tense here. 'added'

>+void
>+nsAccDocManager::RemoveListeners(nsIDocument *aDocument)
>+{
>+  // Document hasn't window when application shutdowns. Eventually we didn't
>+  // receive "pagehide" event from it and therefore its accessible wasn't
>+  // shutdown.

"Document has no window when the application shuts down. The document can still exist because we didn't receive a "pagehide" event."

>+  NS_LOG_ACCDOCDESTROY("remove 'pagehide' listener", aDocument)

>+    NS_LOG_ACCDOCDESTROY("remove 'DOMContentLoaded' listener", aDocument)

'removed'

>+    // XXXaaronl: ideally we would traverse the presshell chain. Since there's
>+    // no easy way to do that, we cheat and use the document hierarchy.
>+    // GetAccessible() is bad because it doesn't support our concept of multiple
>+    // presshells per doc. It should be changed to use
>+    // GetAccessibleInWeakShell().

We should reference a bug # here I think.

>+PLDHashOperator
>+nsAccDocManager::ClearDocCacheEntry(const void* aKey,
>+                                    nsRefPtr<nsDocAccessible>& aDocAccessible,
>+                                    void* aUserArg)
>+{
>+  nsAccDocManager *accDocMgr = static_cast<nsAccDocManager*>(aUserArg);
>+
>+  NS_ASSERTION(aDocAccessible,
>+               "Calling ClearDocCacheEntry with a NULL pointer!");

Why not just bail?

>+/**
>+ * Manage the document accessible creation process.
>+ */

Or maybe "Manage the document accessible life cycle"

>+  /**
>+   * Search an accessible by the given unique id through all document
>+   * accessibles.
>+   */

"Search through all document accessibles for an accessible with the given unique id."

>+   * XXX: in general AT expect events for document accessible loading into
>+   * tabbrowser, events from other document accessibles may break AT. We need to
>+   * figure out what AT wants to know about loading page (for example, some of
>+   * them have separate processing of iframe documents on the page and therefore
>+   * they need a way to distinguish sub documents from page document). Ideally
>+   * we should make events firing for any loaded document and provide additional
>+   * info AT are needing.
>+   */
>+  PRBool IsEventTargetDocument(nsIDocument *aDocument) const;

Ref that bug # here too please.

>+  /**
>+   * Create document or root accessible.
>+   */
>+  nsDocAccessible *CreateDocOrRootAccessible(nsIDocument *aDocument);

Please change spacing to: nsDocAccessible* (to make return type explicit)

>+  /**
>+   * Shutdown and removes the document accessible from cache.
>+   */

please change to either: "remove" or "Shuts down"

> nsDocAccessible*
> nsAccEvent::GetDocAccessible()
> {
>   nsINode *node = GetNode();
>   if (node)
>-    return nsAccessNode::GetDocAccessibleFor(node->GetOwnerDoc());
>+    return GetAccService()->GetDocAccessible(node->GetOwnerDoc());

Do we still need the node check?

>+void
>+nsAccessibilityService::Shutdown()
>+{
>+  // Remove observers.
>+  nsCOMPtr<nsIObserverService> observerService =
>+      mozilla::services::GetObserverService();
>+  if (observerService)
>+    observerService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
>+
>+  // Stop accessible document loader.
>+  nsAccDocManager::Shutdown();
>+
>+  // Application is going to be closed, shutdown accessibility and mark
>+  // accessibility service as shutdown to prevent calls of its methods.
>+  // Don't null accessibility service static member at this point to be safe
>+  // if someone will try to operate with it.
>+
>+  NS_ASSERTION(!gIsShutdown, "Accessibility was shutdown already");
>+
>+  gIsShutdown = PR_TRUE;

I wonder if you can set this at the top of this method? It is sort of a guard for this method right?

>+  /**
>+   * Reference on accessibility service.
>+   */

change "on" to "for".

>+  /**
>+   * Append/remove a child. Alternative approach of children handling than
>+   * CacheChildren/InvalidateChildren.
>+   *
>+   * @param  aAccessible  [in] child to append/remove
>+   * @return true          if child was successfully appended/removed
>+   */
>+  virtual PRBool AppendChild(nsAccessible *aAccessible) { return PR_FALSE; }
>+  virtual PRBool RemoveChild(nsAccessible *aAccessible) { return PR_FALSE; }

It could probably be "AddChild" since 'append' is an implementation detail.

>+PRBool
>+nsCoreUtils::IsErrorPage(nsIDocument *aDocument)
>+{

Regarding your implementation of this... isn't there API we could use instead?

>+nsresult
>+nsOuterDocAccessible::Shutdown()

[left off here]

Comment 33

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

BTW the patch has a lot of good stuff :)

Comment 34

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

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

OK finished everything except the tests. Conditional r=me if you can answer my questions to my happiness :)

Comment 35

[Ginn Chen]

(In reply to comment #25)

> bug 482932 - Ginn, can you test this with your debugging magic?
My testing shows bug 482932 is solved by the patch here.

> Bug 417249 - Ginn, since you were able to repro this, can you see whether the
> try-server build regresses the specific scenarios again?
No, it didn't regress.


> Bug 413778 - I don't see a reproducible testcase there and am not sure this can
> be tested without debugging.
I verified this bug, it didn't regress.

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> (In reply to comment #25)
> > Bug 413778 - I don't see a reproducible testcase there and am not sure this can
> > be tested without debugging.
> I verified this bug, it didn't regress.

Ginn, thank you for feedback and testing! Actually it can be tested if you turn on debug output for accessible document handling what was introduced by this patch. But I haven't seen anything similar so it might be not worth to waste your time for this.

However I would like you to test bug 382021 since you were working on it and got steps to reproduce. I'm not sure it's fixed but it should be because we document is loaded (proper flag is set on) with the patch a bit earlier than now.

Comment 37

[Ginn Chen]

No, it didn't solve bug 382021.
But it did solve bug 494802. Great!

Comment 38

[alexander :surkov (:asurkov)]

(In reply to comment #37)
> No, it didn't solve bug 382021.
> But it did solve bug 494802. Great!

Ginn, I again was confused with bug numbers. Sorry. I think I'm tired. Of course this bug shouldn't solve bug 382021. I meant bug 494802. And I'm happy to hear it's solved.

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #30)
> (From update of attachment 449067 [details] [diff] [review])
> 
> >+void
> >+nsAccDocManager::ShutdownDocAccessiblesInTree(nsIDocument *aDocument)
> >+{
> 
> ShutdownDocumentAndSubdocumentAccessibles
> 

The used one is very long but suggested one is more long. I'm happy to change the name but on something that is shorter.

> >+      PRUint32 loadFlags = 0;
> >+      aRequest->GetLoadFlags(&loadFlags);
> >+      if (loadFlags & nsIChannel::LOAD_RETARGETED_DOCUMENT_URI)
> >+        eventType = 0;
> >+    }
> 
> Please ensure we have a test for this.

Ok, you kill me though :)

> >+  if (!IsEventTargetDocument(document))
> 
> When does this happen?

see description of this method.

Comment 40

[Ginn Chen]

BTW:
+  return IsDefunct() ? nsnull : mParent;
+}

please use
return IsDefunct() ? nsnull : mParent.get();

missing feature of Sun compiler. Thanks!

Comment 41

[alexander :surkov (:asurkov)]

(In reply to comment #32)

> >+    // XXX: AT doesn't update their virtual buffer once frame is loaded and it
> Please file and reference the bug #.

> >+    // XXXaaronl: ideally we would traverse the presshell chain. Since > 
> We should reference a bug # here I think.
> 

> >+    docAcc->FireDelayedAccessibleEvent(loadEvent);
> >+  }
> >+
> 
> Why do we always file the busy event after this? (below)

for historical reasons.

> >+  NS_ASSERTION(aDocAccessible,
> >+               "Calling ClearDocCacheEntry with a NULL pointer!");
> 
> Why not just bail?

We don't expect this.
> Ref that bug # here too please.


> >+  /**
> >+   * Create document or root accessible.
> >+   */
> >+  nsDocAccessible *CreateDocOrRootAccessible(nsIDocument *aDocument);
> 
> Please change spacing to: nsDocAccessible* (to make return type explicit)

I would prefer to keep this styling. Usually '*' is separated from type because '*' doesn't mean new type.

> >   if (node)
> >-    return nsAccessNode::GetDocAccessibleFor(node->GetOwnerDoc());
> >+    return GetAccService()->GetDocAccessible(node->GetOwnerDoc());
> 
> Do we still need the node check?

Yes, will be crashed on application accessilbe

> >+  gIsShutdown = PR_TRUE;
> 
> I wonder if you can set this at the top of this method? It is sort of a guard
> for this method right?

That's how we do currently. I'm not sure how it may affect on accessible shutdown process, I think nohow but who knows. If you like then I can add XXX comment to get back it once. I'm not sure I want to burden this patch by unrelated bug fixes.

> >+  virtual PRBool AppendChild(nsAccessible *aAccessible) { return PR_FALSE; }
> >+  virtual PRBool RemoveChild(nsAccessible *aAccessible) { return PR_FALSE; }
> 
> It could probably be "AddChild" since 'append' is an implementation detail.

I like AppendChild because it appends into the end of array. It's internal method so the caller should care about implementation.

> Regarding your implementation of this... isn't there API we could use instead?

bz said this way.

Comment 42

[alexander :surkov (:asurkov)]

(In reply to comment #41)
> (In reply to comment #32)
> 
> > >+    // XXX: AT doesn't update their virtual buffer once frame is loaded and it
> > Please file and reference the bug #.
> 
> > >+    // XXXaaronl: ideally we would traverse the presshell chain. Since > 
> > We should reference a bug # here I think.
> > 

I'm not sure I share an idea to add bug for every XXX comment. XXX comment means some things that developer should keep an eye on while he works with this code, it means here's bug likely. The bug in bugzilla is different thing because it must include good description, steps to reproduce if you want it won't hang forever. Also bugs should be organized well otherwise there's no chance to keep them on track. Technically we work much more with the code than with bugs, so I prefer to see potential problem in the code than look for bugs. Also let's say I see XXX comment with bug number, open the bug and see absolutely the same wording like in code and silence in discussion. It doesn't make sense. So I would say let's be silent until we really have what to say.

Comment 43

[alexander :surkov (:asurkov)]

Attachment: patch2

We probably doesn't want to summon bz since he must be very busy, a11y is not his area really, the patch is big and it was tested well by Marco and Ginn, has reviews.

Comment 44

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/a2ba3a7cec03 with Marco, David and Ginn's addressed comments