Closed Bug 1280551 Opened 4 years ago Closed 3 years ago

Documents sometimes load with no child accessibles but all text merged into document

Categories

(Core :: Disability Access APIs, defect)

48 Branch
x86
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: Jamie, Assigned: surkov)

References

Details

(Keywords: regression)

Attachments

(4 files, 6 obsolete files)

Sometimes, documents load with no child accessibles except text leaf accessibles.
* All the text is merged into the parent; i.e. retrieving the text on the document returns all of the text with no embedded object characters.
* All of the text content is present as you would expect; there is just no semantic information whatsoever.
* If you tab to something which should be focusable, focus does move in Firefox (I can confirm this by using copy link location in the context menu or similar), but no accessible event is fired.
* The only way to fix this is to refresh the document, and even then, it might (or might not) occur again.

Unfortunately, this is rather intermittent. I currently can't get it to happen in Firefox, though I've definitely seen it happen there. It happens far more frequently in Thunderbird. It's a fairly recent regression; I'd say some time in the last month or so.

Sorry for filing a bug with such little info. I really don't have any more info to give, but I figured filing it with little info was better than never filing it. :(
I can reproduce it with git manpages. So in Windows, if I have Git installed from the official distribution, open a Git bash, then type

$ git help grep

Firefox comes up with the man page for the grep documentation.

If I do this a couple of times, eventually the man page will come up as a big blob of text without headings or other accessibles. It may require 3, 4, or 5 attempts.
I'd be helpful to know a regression bug.
I just (painstakingly :)) tracked this down with mozregression. The regressing bug is bug 1261170.

26:59.64 INFO: Last good revision: 336759fb7df024ec7fe456df8337316e8361c4d5
26:59.64 INFO: First bad revision: 296675bfe330a157e8b3054befee29273e3ac3fb
26:59.64 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=336759fb7df024ec7fe456df8337316e8361c4d5&tochange=296675bfe330a157e8b3054befee29273e3ac3fb

27:00.87 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1261170
Wow, thanks, Jamie, for sticking with it and finding the regressing bug!

Alex, any ideas?
Blocks: 1261170
Has Regression Range: --- → yes
Flags: needinfo?(surkov.alexander)
(In reply to James Teh [:Jamie] from comment #3)
> I just (painstakingly :)) tracked this down with mozregression. The
> regressing bug is bug 1261170.

thank you, Jamie. 

The finding is interesting because bug 1292627 largely reverted that change, and text node insertions and element node insertions have common processing path again.

what are known reliable steps to reproduce?

Marco, what git version do you use or what package did you use to install it? any change to see that on mac?
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #5)
> what are known reliable steps to reproduce?

I managed to do it with calling git help commit or such a command, which on Windows, brings up the default browser (or the currently running instance of Firefox that is running), with the HTML manual page. That is the standard Git for Windows install from https://git-scm.com/. I usually had to bring up the same man page two or three times before the bug became visible.

> Marco, what git version do you use or what package did you use to install
> it? any change to see that on mac?

I never saw Git for Mac bring up the man page in a browser like that. It always outputs to the console, and I've never bothered to check whether that can be changed.Unix command line and I... ;)
I tried 8 times git help grep and NVDA managed to read all headings every time. Marco, may I ask you to send logs with export A11YLOG=tree,verbose; variable set?
Marco are you able to get logs as per comment 7?
No luck so far. :-(
Flags: needinfo?(mzehe)
Attached file Verbose a11y tree log. (obsolete) —
I managed to get this to happen on the first try in this session.
(In reply to James Teh [:Jamie] from comment #10)
> Created attachment 8784708 [details]
> Verbose a11y tree log.
> 
> I managed to get this to happen on the first try in this session.

thank you, Jamie. what a page that was?
btw, it would be useful to have a normal log too to compare
Jamie, are you sure 'verbose' is set too? All verbose logs are missed. I'd be good to have one for 'bad case', but otherwise it looks like layout issue. In a bad case we get tons of text change notifications, which are under document accessible, which means 'initial tree creation' failed to create a tree. Also no other notifications from layout. It looks like frames are not accessible when we build the initial tree, so we skip them.
(In reply to alexander :surkov from comment #15)
> Jamie, are you sure 'verbose' is set too? All verbose logs are missed.

Hmm. I have the A11YLOG environment variable set to "tree,verbose;". Is that correct? Were both logs missing verbose info or just one of them?
(In reply to James Teh [:Jamie] from comment #16)
> (In reply to alexander :surkov from comment #15)
> > Jamie, are you sure 'verbose' is set too? All verbose logs are missed.
> 
> Hmm. I have the A11YLOG environment variable set to "tree,verbose;". 

it's strange, any chance for misspelling?

> Is that
> correct? 

yes

> Were both logs missing verbose info or just one of them?

both of them

here's a try build that contains some noisy extra logging ("tree") that may reveal a problem, would you please give me a log from that build too?
> > Hmm. I have the A11YLOG environment variable set to "tree,verbose;". 
> 
> it's strange, any chance for misspelling?
I copied and pasted in both cases, so I don't think so. Does verbose logging work for normal nightly builds? I'm just using a nightly, not a local build.

> here's a try build that contains some noisy extra logging ("tree") that may
> reveal a problem, would you please give me a log from that build too?
I don't see a link to a try build...?
(In reply to James Teh [:Jamie] from comment #18)
> > > Hmm. I have the A11YLOG environment variable set to "tree,verbose;". 
> > 
> > it's strange, any chance for misspelling?
> I copied and pasted in both cases, so I don't think so.

hm, "export A11YLOG=tree,verbose;" should do a trick. Once verbose logging is enabled, then the log should contain messages like "Initial subtree;" and "children after insertion"

> Does verbose logging
> work for normal nightly builds? I'm just using a nightly, not a local build.

it should be no difference, same module, just different flags

> > here's a try build that contains some noisy extra logging ("tree") that may
> > reveal a problem, would you please give me a log from that build too?
> I don't see a link to a try build...?

forgot to paste it, sorry, https://archive.mozilla.org/pub/firefox/try-builds/surkov.alexander@gmail.com-95ad8317c194efcc72c3744bbb9253e5a28029f1/
In Windows, it's "set", not "export", but the effect is the same.

I just had a thought: is verbose logging perhaps output to stderr, not stdout? I was only redirecting stdout (and because it's a GUI mode app, stdout/tstderr go nowhere if not redirected).
jamie, thanks for the logs, I started a new build with forced tree and verbose logging [1], would you please share a bad log when the build is complete?

[1]  https://archive.mozilla.org/pub/firefox/try-builds/surkov.alexander@gmail.com-080f67a1259365ea553f759829e7d8fe68f14fd2/
(In reply to James Teh [:Jamie] from comment #20)
> In Windows, it's "set", not "export", but the effect is the same.

I think I do 'export' on Windows (in mozbuild console I guess), I might be wrong but I thought that the difference is 'export' lives during the session, while 'set' is active in scope of a command.

> I just had a thought: is verbose logging perhaps output to stderr, not
> stdout? I was only redirecting stdout (and because it's a GUI mode app,
> stdout/tstderr go nowhere if not redirected).

they are quite same, they are just different modules. When build is started and logging is enabled, then it outputs what modules was enabled, in your logs it always says 'module enabled: tree' and that's it. Not sure why.
Attachment #8784708 - Attachment is obsolete: true
Attachment #8787011 - Attachment is obsolete: true
Attachment #8785011 - Attachment is obsolete: true
Attachment #8787012 - Attachment is obsolete: true
(In reply to alexander :surkov from comment #24)
> > In Windows, it's "set", not "export", but the effect is the same.
> I think I do 'export' on Windows (in mozbuild console I guess)

Oh! I'm not using the mozbuild console, just the normal Windows command interpreter, which is very different to bash.

This made me realise that I assumed you wanted a literal semi-colon at the end; i.e. so Firefox would see "tree,verbose;". Actually, the semi-colon would have been interpreted as a command separator in bash, so for "export A11YLOG=tree,verbose;", Firefox would never see the semi-colon. In contrast, semi-colon isn't special for the Windows command interpreter, so Firefox would see it. That's probably why this wasn't working; it would have been ignoring the invalid token "verbose;". Anyway, I generated these latest logs with the try build you provided just to be sure.

It's worth noting that this bug has made it into Firefox 48 and it's hurting more users than me now. I'm also seeing it a lot lately on GitHub and in other places. It'd be great if this could be given some priority for a fix in Firefox 49. I'm having to switch browsers from time to time as a result of this.
(In reply to James Teh [:Jamie] from comment #27)
> (In reply to alexander :surkov from comment #24)
> > > In Windows, it's "set", not "export", but the effect is the same.
> > I think I do 'export' on Windows (in mozbuild console I guess)
> 
> Oh! I'm not using the mozbuild console, just the normal Windows command
> interpreter, which is very different to bash.
> 
> This made me realise that I assumed you wanted a literal semi-colon at the
> end; i.e. so Firefox would see "tree,verbose;". Actually, the semi-colon
> would have been interpreted as a command separator in bash, so for "export
> A11YLOG=tree,verbose;", Firefox would never see the semi-colon. In contrast,
> semi-colon isn't special for the Windows command interpreter, so Firefox
> would see it. That's probably why this wasn't working; it would have been
> ignoring the invalid token "verbose;". 

sounds true

> Anyway, I generated these latest logs
> with the try build you provided just to be sure.

thank you, again

> It's worth noting that this bug has made it into Firefox 48 and it's hurting
> more users than me now. I'm also seeing it a lot lately on GitHub and in
> other places. It'd be great if this could be given some priority for a fix
> in Firefox 49. I'm having to switch browsers from time to time as a result
> of this.

I wish I was able to debug it myself.

Due to some reason, we don't find any accessibles under a document, I'll start over a new build with even more extended logging.

https://archive.mozilla.org/pub/firefox/try-builds/surkov.alexander@gmail.com-da3340dbff11701aa6c12d7f99a168da552d9def/
Flags: needinfo?(jamie)
I had to zip this, as the fail log was too big to attach uncompressed. :(
Attachment #8791834 - Attachment is obsolete: true
Attachment #8791835 - Attachment is obsolete: true
Flags: needinfo?(jamie)
(In reply to James Teh [:Jamie] from comment #29)
> Created attachment 8792763 [details]
> Verbose a11y tree logs for failed and successful renders.
> 
> I had to zip this, as the fail log was too big to attach uncompressed. :(

thanks, logging files had names were confused, bad was good, good was bad :)

Would you please give a try to this build [1], no logging, it should fix the issue.

[1] https://archive.mozilla.org/pub/firefox/try-builds/surkov.alexander@gmail.com-78927f3618acf9b02bb689e016cf4099d9ded169/
(In reply to alexander :surkov from comment #31)
> thanks, logging files had names were confused, bad was good, good was bad :)

Oops. Sorry. I originally named them badlog.txt and goodlog.txt and then renamed them to make more sense, but clearly, I renamed the wrong way. :(

> Would you please give a try to this build [1], no logging, it should fix the
> issue.

And it does! Thank you! We got there! :)

Ironically, now that you've solved this, we've just been presented with a test case that reproduces this issue 100%, at least for me. (Your try build does fix this.) In case it's helpful, STR:
1. Open this URL: http://codepen.io/PetrashkoEV/pen/BLzRjE?editors=1010
2. Press the "Open popup button" button.
3. Press the "Close popup button".
4. Examine the accessibility tree for the test document.
Expected: The first child should be a form.
Actual: There are only text leaf children.
(In reply to James Teh [:Jamie] from comment #32)
> (In reply to alexander :surkov from comment #31)
> > thanks, logging files had names were confused, bad was good, good was bad :)
> 
> Oops. Sorry. I originally named them badlog.txt and goodlog.txt and then
> renamed them to make more sense, but clearly, I renamed the wrong way. :(

no worries, it took almost no time to find the right one 

> > Would you please give a try to this build [1], no logging, it should fix the
> > issue.
> 
> And it does! Thank you! We got there! :)

awesome!

> Ironically, now that you've solved this, we've just been presented with a
> test case that reproduces this issue 100%, at least for me. (Your try build
> does fix this.) In case it's helpful, STR:

thanks, I've got one in mochitest (btw, our mochitests were very close to cover the case, but still not there)
Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Attachment #8793099 - Flags: review?(yzenevich)
Comment on attachment 8793099 [details] [diff] [review]
patch

Review of attachment 8793099 [details] [diff] [review]:
-----------------------------------------------------------------

Fix seems trivial, thanks for the test
Attachment #8793099 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff755d190b023901788a288ae2b1c404eb9ef7d3
Bug 1280551 - Documents sometimes load with no child accessibles but all text merged into document, r=yzen
https://hg.mozilla.org/mozilla-central/rev/ff755d190b02
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Alex, please request approval for aurora (51) and Beta (50). This problem is wide-spread, and it also fixes bug 1281828. David, do you know if there's a 49.0.1 or so planned which could take this as a ride-along as well?
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(dbolter)
I'm not aware of a planned dot release, but Liz is tracking all possible ride-alongs in case there is.
Flags: needinfo?(dbolter)
Comment on attachment 8793099 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: 1261170
[User impact if declined]: web pages and browser UI intermittently inaccessilbe for screen reader users
[Describe test coverage new/current, TreeHerder]:mochitest, manual testing
[Risks and why]: low, a small change
[String/UUID change made/needed]:no
Flags: needinfo?(surkov.alexander)
Attachment #8793099 - Flags: approval-mozilla-aurora?
Comment on attachment 8793099 [details] [diff] [review]
patch

Review of attachment 8793099 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes a regression. Take it in 51 aurora.
Attachment #8793099 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Alex, can you also prepare a patch that can land on 50 (Beta) and possibly 49 dot release? The current patch is based on bug 1274381 and thus cannot be backported, but it fixes a regression from an older bug, so we need a patch based on the code predating bug 1274381. Thanks!
Flags: needinfo?(surkov.alexander)
Attached patch beta patchSplinter Review
here's a beta patch, hopefully it works ;)
Flags: needinfo?(surkov.alexander)
Attachment #8794876 - Flags: feedback?(mzehe)
Comment on attachment 8794876 [details] [diff] [review]
beta patch

Yes, this is correct.
Attachment #8794876 - Flags: feedback?(mzehe) → feedback+
Comment on attachment 8794876 [details] [diff] [review]
beta patch

Approval Request Comment
[Feature/regressing bug #]: 1261170
[User impact if declined]: web pages and browser UI intermittently inaccessible for screen reader users, also impacts Thunderbird users
[Describe test coverage new/current, TreeHerder]:mochitest, manual testing, lives on Central and Aurora
[Risks and why]: low, a small change
[String/UUID change made/needed]:no
Attachment #8794876 - Flags: approval-mozilla-beta?
Version: Trunk → 48 Branch
Comment on attachment 8794876 [details] [diff] [review]
beta patch

Fixes an issue in Reader Mode, stabilized on pre-beta channels, Beta50+
Attachment #8794876 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.