Closed
Bug 638465
Opened 13 years ago
Closed 10 years ago
We're not picking up the insertion into the newly created window of testcase for bug 637644
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: MarcoZ, Assigned: surkov)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
3.04 KB,
patch
|
tbsaunde
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Can we get a regression range?
Comment 2•13 years ago
|
||
Note Marco's comment 0 is regarding tryserver build from bug 637644 comment 35.
Reporter | ||
Comment 3•13 years ago
|
||
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•13 years ago
|
||
Fair enough. We can reconsider if we need to when we're active here.
Assignee | ||
Comment 5•13 years ago
|
||
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?
Version: Trunk → Other Branch
Assignee | ||
Updated•13 years ago
|
Blocks: treeupdatea11y
Comment 6•13 years ago
|
||
What do you mean by "initial document"?
Assignee | ||
Comment 7•13 years ago
|
||
(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•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
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•13 years ago
|
||
> 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"....
Assignee | ||
Comment 11•10 years ago
|
||
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.alexander@gmail.com-c38be1804c1b/). Does anybody get confused by temporary created documents for a short time?
Flags: needinfo?(marco.zehe)
Reporter | ||
Comment 13•10 years ago
|
||
(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. > alexander@gmail.com-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.
Flags: needinfo?(marco.zehe)
Assignee | ||
Comment 14•10 years ago
|
||
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #8400196 -
Flags: review?(trev.saunders)
Comment 15•10 years ago
|
||
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?
Attachment #8400196 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 16•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f71655a15266
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Reporter | ||
Updated•10 years ago
|
Version: Other Branch → Trunk
Reporter | ||
Updated•10 years ago
|
status-firefox29:
--- → wontfix
status-firefox30:
--- → affected
status-firefox31:
--- → verified
tracking-firefox30:
--- → ?
Reporter | ||
Comment 18•10 years ago
|
||
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.
Attachment #8400196 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Comment 19•10 years ago
|
||
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.
Attachment #8400196 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter | ||
Comment 20•10 years ago
|
||
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!
Keywords: checkin-needed
Comment 21•10 years ago
|
||
FWIW, you don't need to checkin-needed uplifts. We (I) query for them anyway :)
Updated•10 years ago
|
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•