638465 - We're not picking up the insertion into the newly created window of testcase for bug 637644

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Open the testcase attached to bug 637644.
2. Press the "Press me!" button and watch the window get opened.

Expected: The newly inserted "I'm here!" should be found.
Actual: The accessible tree stops at the iframe. No document accessible is created.

This is the case for both current nightly and the latest try-server build from bug 637644.

Comment 1

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

Can we get a regression range?

Comment 2

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

Note Marco's comment 0 is regarding tryserver build from bug 637644 comment 35.

Comment 3

[Marco Zehe (:MarcoZ)]

I don't believe it makes much sense to hunt for a regression range. First, this bug should wait for bug 637644 to be fixed. This will not happen for RC1, possibly RC2 according to Beltzner in #planning, but even that didn't sound definitive to me.
Second, that bug was around for months before it was discovered. The code from when bug 637644 was introduced to what we have in the a11y module today is so signifficantly different thhat hunting is prune to be very uncertain what results are concerned.
I suggest to properly debug this once bug 637644 is fixed.

In addition, since bug 637644 has been lingering for months before it was discovered, I suspect that not many sites are using this technique to insert content into a newly created window. We should definitely not block FX4 on that, possibly make it a fix for a point release.

Comment 4

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

Fair enough. We can reconsider if we need to when we're active here.

Comment 5

[alexander :surkov (:asurkov)]

The problem when AccService handles ContentRangeInserted notification for text node appended to the body, the text node's document is initial document and we do not create an accessible for it.

It sounds we shouldn't ignore all initial documents if its initial state in this case is not a bug. Boris, do you have ideas how I can distinguish permanent initial documents from temporary initial documents?

Comment 6

[Boris Zbarsky [:bzbarsky]]

What do you mean by "initial document"?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> What do you mean by "initial document"?

(In reply to comment #6)
> What do you mean by "initial document"?

nsIDocument::IsInitialDocument() == true

Comment 8

[Boris Zbarsky [:bzbarsky]]

In that case I'm not sure what you mean by "permanent initial documents".  Initial documents are just the first document loaded in the window; that's all it means.

Comment 9

[alexander :surkov (:asurkov)]

It sounds I've got the wrong impression that initial is equivalent to temporary, i.e. the document that is going to be replaced on another one. A11y doesn't need to handle temporary documents but it sounds it should handle initial documents. Do you have ideas how we could proceed this case?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> It sounds I've got the wrong impression that initial is equivalent to
> temporary, i.e. the document that is going to be replaced on another one.

That impression is definitely incorrect, yes.

Unless you can predict the future, you can't tell when looking at a document whether it's "temporary"....

Comment 11

[alexander :surkov (:asurkov)]

Marco, can you run the try server build for a while with different ATs (https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c38be1804c1b/). Does anybody get confused by temporary created documents for a short time?

Comment 13

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #11)
> Marco, can you run the try server build for a while with different ATs
> (https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.
> [e-mail]-c38be1804c1b/). Does anybody get confused by temporary
> created documents for a short time?

From the testing I did with NVDA, JAWS and Window-Eyes, none of them seem to get confused with anything in this try build. Moreover, it fixes the issue also for bug 986876.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 15

[Trevor Saunders (:tbsaunde)]

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

>   // Ignore temporary, hiding, resource documents and documents without
>   // docshell.
>-  if (aDocument->IsInitialDocument() ||
>-      !aDocument->IsVisibleConsideringAncestors() ||
>+  if (!aDocument->IsVisibleConsideringAncestors() ||
>       aDocument->IsResourceDoc() || !aDocument->IsActive())
>     return nullptr;

update comment to remove temporary?

>+      {
>+        // Reorder event might be duped because of temporary document creation.
>+        if (aEvent.accessible == getAccessible(currentBrowser())) {
>+          this.cnt++;

not actually checking the first one is an initial / temporary doc is kind of lame, but whatever

>+          return this.cnt != 2;

wait, so the first time through this matches, then if you get a dup event it doesn't match right? why is that correct? and if so could you just use a bool?

Comment 16

[alexander :surkov (:asurkov)]

the logic was
1) first event we handle (either case we are fine)
2) second one if fired is ignored (works for the case when temporary document was created and attached)
3) third and further if fired trigger dupe event error (it doesn't happen, just a test)

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f71655a15266

Comment 18

[Marco Zehe (:MarcoZ)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Indeterminable, see comment #3 for an explanation.
User impact if declined: I just learned that a much needed feature to be introduced into Google Docs is depending on this fix. The feature would allow blind users to read text in a Google Doc on a braille display, which is currently not possible. If this only gets into 31, it would mean that Google would have to delay publishing this feature to the general audience for 3 more months. Having it in 30 would at least soften this to a mere 6 weeks.
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): Low. Has been baking on m-c for 4 weeks, heavily tested by me and other Nightly users.
String or IDL/UUID changes made by this patch: None.

I realize this is late in the game for the Aurora 30 train, but I thought it worthwhile raising this anyway so blind users depending on a braille display for reading text could benefit from this feature addition earlier rather than later. This also heavily impacts the education sector.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

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

We don't need to track this because of the longstanding presence in code, but happy to take the uplift and help this feature ship sooner.

Comment 20

[Marco Zehe (:MarcoZ)]

Thanks Lukas!

Sorry, but because I'm currently re-doing my work notebook, I have no means to check this into Aurora myself ATM. Requesting this be checked in by sheriffs this time. Thanks!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

FWIW, you don't need to checkin-needed uplifts. We (I) query for them anyway :)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a470c97f3668