Closed Bug 446469 Opened 14 years ago Closed 9 years ago

Missing and/or incorrect object:state-changed:busy event when downloading files

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jdiggs, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(1 file, 4 obsolete files)

Steps to reproduce:

1. Navigate to an ftp site (I used ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/)

2. Download a file.

When the link associated with the file is activated, the document frame issues an object:state-changed:busy event with detail1 == 1 (i.e. it has STATE_BUSY). At some point, STATE_BUSY is removed from the document frame's stateset, but we are not given another object:state-changed:busy event (detail1 == 0) to notify us that this has taken place.

I'm not sure we should be getting object:state-changed:busy events in this case. (Should we?) I don't mind having them though, as long as we're told when the state is no longer BUSY. :-)

Thanks!
We shouldn't fire the initial state change event at all. This seems like it could really mess up screen readers on Windows, like Window-Eyes.
Attached patch patch (obsolete) — Splinter Review
When the link is to download a file, the doc accessible keeps the same one. We didn't set mIsLoadCompleteFired to false when firing state_busy(1) event. That causes state_busy(0) be ignored.
Assignee: nobody → Evan.Yan
Status: NEW → ASSIGNED
Attachment #336835 - Flags: review?(surkov.alexander)
Attachment #336835 - Flags: review?(aaronleventhal)
Comment on attachment 336835 [details] [diff] [review]
patch

r=me but please put additional comments why we set it to true or false.

I would like to have additional review from aaron.
We set mIsLoadCompleteFired to true when get DOCUMENT_LOAD_COMPLETE/STOPPED, meaning the doc has been loaded, so that we won't fire duplicated state_busy(detail1==0) event.

We should set mIsLoadCompleteFired to false when fire state_busy(detail1==1), meaning the doc has started loading, so that we won't omit the next DOCUMENT_LOAD_COMPLETE/STOPPED.

This bug doesn't exist for normal links (i.e. link to some web page), because a brand new doc accessible is created for the case and mIsLoadCompleteFired is initialized to false. For a link to download a file, the doc accessible keeps the same one.
Attachment #336835 - Flags: review?(surkov.alexander) → review+
Comment on attachment 336835 [details] [diff] [review]
patch

eventually I fortgot to set r+
Evan, I meant to put the comment into patch :)
Comment on attachment 336835 [details] [diff] [review]
patch

We shouldn't be firing this event at all in the case of just downloading a file. It's the event that screen readers with virtual buffers use to decide when to reload the the virtual buffer for that doc. However, the doc itself has not changed.
Attachment #336835 - Flags: review?(aaronleventhal) → review-
But how can we know the link is for downloading a file or redirecting to some other web page?
(In reply to comment #8)
> But how can we know the link is for downloading a file or redirecting to some
> other web page?

Biesi should know.

Otherwise, ask in the newsgroups, or #developers, or check the code or APIs available. There is probably a way.
Aaron, I think we shouldn't depend on the link type to decide whether to fire state_busy event or not.

First, when StartLoadCallback() is called, a connection is just set up. We don't know whether the link is some web page or a file to be downloaded. We just know we're BUSY on loading something.

Second, a link to download a file can also result in opening a new web page. For example, the link is not valid anymore, or the link points to some plain text.

I think it's bad assumption that screen reader reload its virtual buffer on state_busy event.
Evan, okay. 

FWIW using state_change predates IAccessible2 which has a better set of events for doc loading.
Comment on attachment 336835 [details] [diff] [review]
patch

Evan, we won't fire DOC_LOAD_COMPLETE event for this case will we?
Attachment #336835 - Flags: review- → review+
(In reply to comment #12)
> (From update of attachment 336835 [details] [diff] [review])
> Evan, we won't fire DOC_LOAD_COMPLETE event for this case will we?

Yes, we will. Is that a problem?
It doesn't make sense to me. That doc we're firing it on didn't just finish loading.

So yes, it's a problem.
The doc, which is the downloaded file in this case, did finish loading. And EndLoadCallback() got called.
Right, but we're firing the event on the content doc right? So that makes it look like the content doc finished loading.
Attached patch patch v2 (obsolete) — Splinter Review
I moved the code of firing state_busy(0) to fix some other situation that state_busy(1) and state_busy(0) unpaired.
Attachment #336835 - Attachment is obsolete: true
Attachment #338293 - Flags: review?(aaronleventhal)
Attached patch minor change (obsolete) — Splinter Review
Attachment #338293 - Attachment is obsolete: true
Attachment #338294 - Flags: review?(aaronleventhal)
Attachment #338293 - Flags: review?(aaronleventhal)
Marco, can you test this?
I'm not sure I like what this patch is doing:
1. If I download a .zip or other non-executable file on Windows from the above URL, everything is OK. The "Download" dialog opens, and I can choose what I want to do.
2. However, if I click an executable file, there are now events that trick JAWS into thinking that the document just reloaded, and it starts to read everything from the top again.
Note that due to a different bug, whose number escapes me at the moment, downloading an executable file does not generate a focus event on the active "Download" dialog control.

Without this patch, JAWS would not start reading the document over again whenc licking the executable's link.

I don't think this is good.
As Aaron suggested, it should be the new added state_busy(0) event which cause JAWS reload its buffer.

state_busy doesn't necessarily mean the document is reloaded. So I think it's not right to depend on state_busy to reload the buffer. Instead, JAWS might look at load_completed event.
Evan, I'm mostly worried about the older versions of commercial screen readers. Those that don't use the more granular IA2 states yet will be negatively affected by this change. And not all users can afford to upgrade to the latest versions even if JAWS 10 and Window-Eyes 7 may adapt to this new firing event stuff.
(In reply to comment #20)
> Note that due to a different bug, whose number escapes me at the moment,
> downloading an executable file does not generate a focus event on the active
> "Download" dialog control.
> 
Marco, did you recall that bug? If with that bug's fix, JAWS works well with downloading executable file, then I guess we got a way that makes both Orca and JAWS happy.
The number is bug 452478. Initially, I thought this was concerning all download links, but it turns out that these are primarily concerning executables on Windows. But something is going wrong with some of the others as well, so that bug definitely needs some attention.
Evan, when a load starts, is there absolutely no way for us to tell what it is? What if we wait to fire the start load event until we can find out whether it's a new document load?
Evan/Biesi, what if we use STATE_TRANSFERRING instead of STATE_START? By that time we should know if it's really a document that's loading, as opposed to a file downloading.
What if we do nothing at load start, but at load end we fire both of state_busy(1) and state_busy(0) if a new doc has been loaded ?
If no one depends on state_busy(1), I think it's doable.
It's tricky, but it makes Orca and JAWS happy.

For bug 452478, I didn't reproduce it on Linux. The fired events are the same for executable and other type download file. The bug seems Windows only. If we can get bug 452478 fixed, it might be another way to make JAWS happy.
Evan, I would prefer we use comment 26. Fire the start load event and state_busy(1) once data starts transferring. At that point we should be able to determine what we need.
Evan, what is the status of this bug? We should probably find another reviewer for the patch since Aaron has moved on.
Evan has also moved on.

Per comment #20 and comment #28, the patch should be reworked.
Depends on: 566103
This one should be fixed by bug 566103. Keep open because we want mochitest for this.
Could you please check if it's still valid (bug 566103 is fixed).
Blocks: eventa11y
seems to still be present, if I try and download a file that firefox wants to save (my test cse was the latest-aurora linux build) the only busy state change events I get are

object:state-changed:busy(1, 0, 0)
	source:  [document frame | Index of ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora/] [<enum ATSPI_STATE_BUSY of type StateType>, <enum ATSPI_STATE_ENABLED of type StateType>, <enum ATSPI_STATE_FOCUSABLE of type StateType>, <enum ATSPI_STATE_HORIZONTAL of type StateType>, <enum ATSPI_STATE_OPAQUE of type StateType>, <enum ATSPI_STATE_SENSITIVE of type StateType>, <enum ATSPI_STATE_SHOWING of type StateType>, <enum ATSPI_STATE_STALE of type StateType>, <enum ATSPI_STATE_VISIBLE of type StateType>] 
	application:  [application | Firefox] 
object:state-changed:busy(1, 0, 0)
	source:  [document frame | Index of ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora/] [<enum ATSPI_STATE_BUSY of type StateType>, <enum ATSPI_STATE_ENABLED of type StateType>, <enum ATSPI_STATE_FOCUSABLE of type StateType>, <enum ATSPI_STATE_HORIZONTAL of type StateType>, <enum ATSPI_STATE_OPAQUE of type StateType>, <enum ATSPI_STATE_SENSITIVE of type StateType>, <enum ATSPI_STATE_SHOWING of type StateType>, <enum ATSPI_STATE_STALE of type StateType>, <enum ATSPI_STATE_VISIBLE of type StateType>] 
	application:  [application | Firefox] 

 so it looks like we say the state changed from off to on and then again from off to on, are we just not negating something we should I wonder?

iirc the code mapping gecko state changes to atk ones is straight forward so I'd expect you could test this in domi.
So nsDocAccessible::NotifyOfLoading triggers but nsDocAccessible::ProcessLoad doesn't. Nothing obvious, somebody needs to debug.
Target Milestone: --- → mozilla13
Version: Trunk → Other Branch
Target Milestone: mozilla13 → ---
Version: Other Branch → Trunk
Comment on attachment 338294 [details] [diff] [review]
minor change

Alex, is this patch still valid?
Attachment #338294 - Flags: review?(aaronlev) → review?(surkov.alexander)
Comment on attachment 338294 [details] [diff] [review]
minor change

it's a little bit out of date and it's easier to write a new patch than resurrect this one.
Attachment #338294 - Attachment is obsolete: true
Attachment #338294 - Flags: review?(surkov.alexander)
Attached patch patch (obsolete) — Splinter Review
we used to fire state change busy true when file is clicked but we never filed state change busy false after that. Patches fixes missed busy false event.
Assignee: evan.yan → surkov.alexander
Attachment #681879 - Flags: review?(trev.saunders)
Comment on attachment 681879 [details] [diff] [review]
patch

intermittent failures, need to fix it
Attachment #681879 - Flags: review?(trev.saunders)
Attached patch patchSplinter Review
Attachment #681879 - Attachment is obsolete: true
Attachment #682407 - Flags: review?(trev.saunders)
Comment on attachment 682407 [details] [diff] [review]
patch

>+  // If the document is loaded completely then network activity was presumingly
>+  // cased by file loading. Fire busy state change event.

nit, caused

>+      // XXX: state change busy false event might be delievered when document
>+      // has state busy true already (these events should be coalesced actually
>+      // in this case as nothing happened).

ugh, sadly I'm not sure I have an idea how to fix though other than use a bigger file.
Attachment #682407 - Flags: review?(trev.saunders) → review+
it's intermittent so it's still ok for testing, sometimes we hit it.

but it shown us another bug we need to fix and that's good.
(In reply to alexander :surkov from comment #41)
> it's intermittent so it's still ok for testing, sometimes we hit it.
> 
> but it shown us another bug we need to fix and that's good.

when we fix that bug won't we have to fix the test again since sometimes there will be no events, and it seems sort of bad we can't test that busy state starts then goes away on download instead of never happening at all.
(In reply to Trevor Saunders (:tbsaunde) from comment #42)
> (In reply to alexander :surkov from comment #41)
> > it's intermittent so it's still ok for testing, sometimes we hit it.
> > 
> > but it shown us another bug we need to fix and that's good.
> 
> when we fix that bug won't we have to fix the test again since sometimes
> there will be no events, and it seems sort of bad we can't test that busy
> state starts then goes away on download instead of never happening at all.

of course, we'll have intermittent testing
https://hg.mozilla.org/mozilla-central/rev/7a37a54f83d4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.