Joanmarie Diggs Reporter
	Description • 16 years ago

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!

	Aaron Leventhal
	Comment 1 • 16 years ago

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.

	Evan Yan
	Comment 2 • 16 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 3 • 16 years ago

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.

	Evan Yan
	Comment 4 • 16 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 5 • 16 years ago

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

eventually I fortgot to set r+

	alexander :surkov (:asurkov) Assignee
	Comment 6 • 16 years ago

Evan, I meant to put the comment into patch :)

	Aaron Leventhal
	Comment 7 • 16 years ago

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.

	Evan Yan
	Comment 8 • 16 years ago

But how can we know the link is for downloading a file or redirecting to some other web page?

	Aaron Leventhal
	Comment 9 • 16 years ago

(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.

	Evan Yan
	Comment 10 • 16 years ago

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.

	Aaron Leventhal
	Comment 11 • 16 years ago

Evan, okay. 

FWIW using state_change predates IAccessible2 which has a better set of events for doc loading.

	Aaron Leventhal
	Comment 12 • 16 years ago

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

Evan, we won't fire DOC_LOAD_COMPLETE event for this case will we?

	Evan Yan
	Comment 13 • 16 years ago

(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?

	Aaron Leventhal
	Comment 14 • 16 years ago

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.

	Evan Yan
	Comment 15 • 16 years ago

The doc, which is the downloaded file in this case, did finish loading. And EndLoadCallback() got called.

	Aaron Leventhal
	Comment 16 • 16 years ago

Right, but we're firing the event on the content doc right? So that makes it look like the content doc finished loading.

	Evan Yan
	Comment 17 • 16 years ago

I moved the code of firing state_busy(0) to fix some other situation that state_busy(1) and state_busy(0) unpaired.

	Aaron Leventhal
	Comment 19 • 16 years ago

Marco, can you test this?

	Marco Zehe (:MarcoZ)
	Comment 20 • 16 years ago

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.

	Evan Yan
	Comment 21 • 16 years ago

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.

	Marco Zehe (:MarcoZ)
	Comment 22 • 16 years ago

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.

	Evan Yan
	Comment 23 • 16 years ago

(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.

	Marco Zehe (:MarcoZ)
	Comment 24 • 16 years ago

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.

	Aaron Leventhal
	Comment 25 • 16 years ago

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?

	Aaron Leventhal
	Comment 26 • 16 years ago

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.

	Evan Yan
	Comment 27 • 16 years ago

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.

	Aaron Leventhal
	Comment 28 • 16 years ago

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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 29 • 15 years ago

Evan, what is the status of this bug? We should probably find another reviewer for the patch since Aaron has moved on.

	Ginn Chen
	Comment 30 • 15 years ago

Evan has also moved on.

Per comment #20 and comment #28, the patch should be reworked.

	alexander :surkov (:asurkov) Assignee
	Comment 31 • 14 years ago

This one should be fixed by bug 566103. Keep open because we want mochitest for this.

	alexander :surkov (:asurkov) Assignee
	Comment 32 • 13 years ago

Could you please check if it's still valid (bug 566103 is fixed).

	Trevor Saunders (:tbsaunde)
	Comment 33 • 13 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 34 • 13 years ago

So nsDocAccessible::NotifyOfLoading triggers but nsDocAccessible::ProcessLoad doesn't. Nothing obvious, somebody needs to debug.

	JP Rosevear [:jpr]
	Comment 35 • 12 years ago

Comment on attachment 338294 [details] [diff] [review]
minor change

Alex, is this patch still valid?

	alexander :surkov (:asurkov) Assignee
	Comment 36 • 12 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 37 • 12 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 38 • 12 years ago

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

intermittent failures, need to fix it

	Trevor Saunders (:tbsaunde)
	Comment 40 • 12 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 41 • 12 years ago

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.

	Trevor Saunders (:tbsaunde)
	Comment 42 • 12 years ago

(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.

	alexander :surkov (:asurkov) Assignee
	Comment 43 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/7a37a54f83d4

	alexander :surkov (:asurkov) Assignee
	Comment 44 • 12 years ago

(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

	Ryan VanderMeulen [:RyanVM]
	Comment 45 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/7a37a54f83d4