Last Comment Bug 446469 - Missing and/or incorrect object:state-changed:busy event when downloading files
: Missing and/or incorrect object:state-changed:busy event when downloading files
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla19
Assigned To: alexander :surkov
:
Mentors:
Depends on: 566103
Blocks: orca eventa11y
  Show dependency treegraph
 
Reported: 2008-07-21 13:39 PDT by Joanmarie Diggs
Modified: 2012-11-17 05:12 PST (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (802 bytes, patch)
2008-09-04 04:23 PDT, Evan Yan
surkov.alexander: review+
aaronlev: review+
Details | Diff | Review
patch v2 (5.45 KB, patch)
2008-09-12 04:29 PDT, Evan Yan
no flags Details | Diff | Review
minor change (5.48 KB, patch)
2008-09-12 04:34 PDT, Evan Yan
no flags Details | Diff | Review
patch (7.37 KB, patch)
2012-11-14 23:49 PST, alexander :surkov
no flags Details | Diff | Review
patch (9.31 KB, patch)
2012-11-16 03:21 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Review

Description Joanmarie Diggs 2008-07-21 13:39:21 PDT
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!
Comment 1 Aaron Leventhal 2008-08-25 03:11:41 PDT
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.
Comment 2 Evan Yan 2008-09-04 04:23:02 PDT
Created attachment 336835 [details] [diff] [review]
patch

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.
Comment 3 alexander :surkov 2008-09-04 20:18:04 PDT
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.
Comment 4 Evan Yan 2008-09-04 21:06:57 PDT
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.
Comment 5 alexander :surkov 2008-09-04 21:08:18 PDT
Comment on attachment 336835 [details] [diff] [review]
patch

eventually I fortgot to set r+
Comment 6 alexander :surkov 2008-09-04 21:08:45 PDT
Evan, I meant to put the comment into patch :)
Comment 7 Aaron Leventhal 2008-09-05 02:01:31 PDT
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.
Comment 8 Evan Yan 2008-09-05 02:44:10 PDT
But how can we know the link is for downloading a file or redirecting to some other web page?
Comment 9 Aaron Leventhal 2008-09-05 02:47:15 PDT
(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.
Comment 10 Evan Yan 2008-09-10 03:38:22 PDT
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.
Comment 11 Aaron Leventhal 2008-09-10 06:55:06 PDT
Evan, okay. 

FWIW using state_change predates IAccessible2 which has a better set of events for doc loading.
Comment 12 Aaron Leventhal 2008-09-10 06:55:43 PDT
Comment on attachment 336835 [details] [diff] [review]
patch

Evan, we won't fire DOC_LOAD_COMPLETE event for this case will we?
Comment 13 Evan Yan 2008-09-11 02:38:52 PDT
(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?
Comment 14 Aaron Leventhal 2008-09-11 02:56:39 PDT
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.
Comment 15 Evan Yan 2008-09-11 03:08:58 PDT
The doc, which is the downloaded file in this case, did finish loading. And EndLoadCallback() got called.
Comment 16 Aaron Leventhal 2008-09-11 03:14:00 PDT
Right, but we're firing the event on the content doc right? So that makes it look like the content doc finished loading.
Comment 17 Evan Yan 2008-09-12 04:29:12 PDT
Created attachment 338293 [details] [diff] [review]
patch v2

I moved the code of firing state_busy(0) to fix some other situation that state_busy(1) and state_busy(0) unpaired.
Comment 18 Evan Yan 2008-09-12 04:34:50 PDT
Created attachment 338294 [details] [diff] [review]
minor change
Comment 19 Aaron Leventhal 2008-09-12 05:47:19 PDT
Marco, can you test this?
Comment 20 Marco Zehe (:MarcoZ) 2008-09-12 07:22:37 PDT
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.
Comment 21 Evan Yan 2008-09-12 23:59:03 PDT
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.
Comment 22 Marco Zehe (:MarcoZ) 2008-09-13 00:23:38 PDT
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.
Comment 23 Evan Yan 2008-09-14 22:25:50 PDT
(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.
Comment 24 Marco Zehe (:MarcoZ) 2008-10-03 04:06:03 PDT
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.
Comment 25 Aaron Leventhal 2008-10-03 04:52:14 PDT
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?
Comment 26 Aaron Leventhal 2008-10-03 05:07:53 PDT
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.
Comment 27 Evan Yan 2008-10-14 05:01:13 PDT
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.
Comment 28 Aaron Leventhal 2008-10-15 01:31:39 PDT
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.
Comment 29 David Bolter [:davidb] 2009-09-30 12:17:16 PDT
Evan, what is the status of this bug? We should probably find another reviewer for the patch since Aaron has moved on.
Comment 30 Ginn Chen 2009-10-09 03:30:43 PDT
Evan has also moved on.

Per comment #20 and comment #28, the patch should be reworked.
Comment 31 alexander :surkov 2010-06-08 10:06:03 PDT
This one should be fixed by bug 566103. Keep open because we want mochitest for this.
Comment 32 alexander :surkov 2012-03-06 22:58:49 PST
Could you please check if it's still valid (bug 566103 is fixed).
Comment 33 Trevor Saunders (:tbsaunde) 2012-03-09 07:54:16 PST
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.
Comment 34 alexander :surkov 2012-03-22 05:12:52 PDT
So nsDocAccessible::NotifyOfLoading triggers but nsDocAccessible::ProcessLoad doesn't. Nothing obvious, somebody needs to debug.
Comment 35 JP Rosevear [:jpr] 2012-11-14 08:14:14 PST
Comment on attachment 338294 [details] [diff] [review]
minor change

Alex, is this patch still valid?
Comment 36 alexander :surkov 2012-11-14 22:51:47 PST
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.
Comment 37 alexander :surkov 2012-11-14 23:49:39 PST
Created attachment 681879 [details] [diff] [review]
patch

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.
Comment 38 alexander :surkov 2012-11-15 23:37:26 PST
Comment on attachment 681879 [details] [diff] [review]
patch

intermittent failures, need to fix it
Comment 39 alexander :surkov 2012-11-16 03:21:47 PST
Created attachment 682407 [details] [diff] [review]
patch
Comment 40 Trevor Saunders (:tbsaunde) 2012-11-16 08:10:26 PST
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.
Comment 41 alexander :surkov 2012-11-16 08:13:00 PST
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.
Comment 42 Trevor Saunders (:tbsaunde) 2012-11-16 11:21:33 PST
(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.
Comment 44 alexander :surkov 2012-11-16 19:58:05 PST
(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
Comment 45 Ryan VanderMeulen [:RyanVM] 2012-11-17 05:12:10 PST
https://hg.mozilla.org/mozilla-central/rev/7a37a54f83d4

Note You need to log in before you can comment on or make changes to this bug.