745986 - [AccessFu] Display page loading states

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

When a page is loading, and when it is done loading, the user needs to get notified.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Report new tab, page loading and page done loading

I am going to sleep on this solution before putting it up for review...
I find the mix of events and notifications a bit strange for such a simple thing.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Bug 745986 - Report page loading, loaded and new tabs.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 616828 [details] [diff] [review]
Bug 745986 - Report page loading, loaded and new tabs.

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

::: accessible/src/jsat/AccessFu.jsm
@@ +105,5 @@
> +        this.presenters.forEach(function(p) {p.tabSelected(docAcc);});
> +
> +        // Keep an eye out for new child doc of new browser.
> +        if (!docAcc)
> +          this._pendingDocuments[browserApp.selectedBrowser] = true;

I wonder why do you need this pendingDocuments at all, you could just listen for busy state change event

@@ +178,5 @@
>                                  event.isEnabled() ? 'check' : 'uncheck');
>                }
>              );
>            }
> +          if (event.state == Ci.nsIAccessibleStates.STATE_BUSY &&

you should return if you hit the 'if' above

@@ +181,5 @@
>            }
> +          if (event.state == Ci.nsIAccessibleStates.STATE_BUSY &&
> +              !(event.isExtraState()) && event.isEnabled() &&
> +              (event.accessible.role == Ci.nsIAccessibleRole.ROLE_DOCUMENT ||
> +               event.accessible.role == Ci.nsIAccessibleRole.ROLE_APPLICATION)) {

I wouldn't prefer to have a local variable for accessible.role

@@ +184,5 @@
> +              (event.accessible.role == Ci.nsIAccessibleRole.ROLE_DOCUMENT ||
> +               event.accessible.role == Ci.nsIAccessibleRole.ROLE_APPLICATION)) {
> +            this.presenters.forEach(
> +              function(p) {
> +                p.pageStateChanged(event.accessible, 'loading');

note: state busy can be fired when something gets loaded, for example, when you click a link to a file to download it

::: accessible/src/jsat/Presenters.jsm
@@ +66,5 @@
>    selectionChanged: function selectionChanged(aObject) {},
>  
>    /**
> +   * The page state has changed.
> +   * @param {nsIAccessible} aObject the page that has had it's state changed.

'the page' as description of aObject is not descriptive, perhaps, tab document accessible or what you mean here

it's -> its?

@@ +67,5 @@
>  
>    /**
> +   * The page state has changed.
> +   * @param {nsIAccessible} aObject the page that has had it's state changed.
> +   * @param {string} aPageState the state name for the page, either 'loading' or 'loaded'

what about 'reload'? it'd be nice to have a complete list somewhere

@@ +74,3 @@
>  
>    /**
>     * The tab has changed.

btw, maybe: the current tab has changed, otherwise it's ambigious

@@ +74,5 @@
>  
>    /**
>     * The tab has changed.
> +   * @param {nsIAccessible} aObject the document contained in the tab, or null
> +   *    if it is a new tab with no attached document yet.

"in the tab" -> "by the tab accessible", otherwise ambiguous because the tab can contain a lot of nested documents.

btw, about the comment style, where it comes from? we use different style in c++ part

@@ +154,5 @@
> +  if (aObject) {
> +    let vcDoc = aObject.QueryInterface(Ci.nsIAccessibleCursorable);
> +    this.pivotChanged(vcDoc.virtualCursor.position);
> +  } else {
> +    this.hide();

VisualPresenter.prototype.pivotChanged makes this.hide() for null aObject. So what is this change for?

@@ +249,5 @@
>    });
>  };
>  
>  AndroidPresenter.prototype.tabSelected = function(aObject) {
> +  if (!aObject) {

I really prefer to have
if (aObject) {
  // bala
  return;
}

// bla bla

@@ +261,5 @@
> +        text: newTabUtterance,
> +        addedCount: newTabUtterance.join('').length,
> +        removedCount: 0,
> +        fromIndex: 0
> +      }

perhaps you could have a nice helper method wrapping sendMessageToJava?

@@ +263,5 @@
> +        removedCount: 0,
> +        fromIndex: 0
> +      }
> +    });
> +    return;

btw, why you don't reuse pageStateChanged method?

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +56,5 @@
>    genForAction: function(aObject, aActionName) {
>      return [gStringBundle.GetStringFromName(this.gActionMap[aActionName])];
>    },
>  
> +  genForAppStateChange: function (aObject, aAppState) {

I don't know why you don't like to comment methods, it really keeps code more readable

@@ +70,5 @@
> +        return [gStringBundle.GetStringFromName('stoppedState')];
> +      case 'reload':
> +        return [gStringBundle.GetStringFromName('reloadState')];
> +      default:
> +        return [];

using an array where you don't need an array looks weird

isn't it nicer to prefix them by 'doc' like 'docReloading' or 'docLoadStopped'?

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #3)
> Comment on attachment 616828 [details] [diff] [review]
> Bug 745986 - Report page loading, loaded and new tabs.
> 
> Review of attachment 616828 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +105,5 @@
> > +        this.presenters.forEach(function(p) {p.tabSelected(docAcc);});
> > +
> > +        // Keep an eye out for new child doc of new browser.
> > +        if (!docAcc)
> > +          this._pendingDocuments[browserApp.selectedBrowser] = true;
> 
> I wonder why do you need this pendingDocuments at all, you could just listen
> for busy state change event
> 

The document accessible object is created with the busy state, so there is no state change.


> @@ +178,5 @@
> >                                  event.isEnabled() ? 'check' : 'uncheck');
> >                }
> >              );
> >            }
> > +          if (event.state == Ci.nsIAccessibleStates.STATE_BUSY &&
> 
> you should return if you hit the 'if' above
> 

It would be weird if that were the only return in the entire switch block. I changed the second if to "else if".

> @@ +181,5 @@
> >            }
> > +          if (event.state == Ci.nsIAccessibleStates.STATE_BUSY &&
> > +              !(event.isExtraState()) && event.isEnabled() &&
> > +              (event.accessible.role == Ci.nsIAccessibleRole.ROLE_DOCUMENT ||
> > +               event.accessible.role == Ci.nsIAccessibleRole.ROLE_APPLICATION)) {
> 
> I wouldn't prefer to have a local variable for accessible.role
> 

Done.

> @@ +184,5 @@
> > +              (event.accessible.role == Ci.nsIAccessibleRole.ROLE_DOCUMENT ||
> > +               event.accessible.role == Ci.nsIAccessibleRole.ROLE_APPLICATION)) {
> > +            this.presenters.forEach(
> > +              function(p) {
> > +                p.pageStateChanged(event.accessible, 'loading');
> 
> note: state busy can be fired when something gets loaded, for example, when
> you click a link to a file to download it
> 

This is unfortunate! How does one know that a document is being loaded? It seems that there should be an equivalent event for EVENT_DOCUMENT_LOAD_COMPLETE, of EVENT_DOCUMENT_LOAD. I am tempted to scratch this whole thing and just go with nsIWebProgressListener.

> ::: accessible/src/jsat/Presenters.jsm
> @@ +66,5 @@
> >    selectionChanged: function selectionChanged(aObject) {},
> >  
> >    /**
> > +   * The page state has changed.
> > +   * @param {nsIAccessible} aObject the page that has had it's state changed.
> 
> 'the page' as description of aObject is not descriptive, perhaps, tab
> document accessible or what you mean here
> 

changed it to tabStateChanged, and updated docs.

> @@ +67,5 @@
> >  
> >    /**
> > +   * The page state has changed.
> > +   * @param {nsIAccessible} aObject the page that has had it's state changed.
> > +   * @param {string} aPageState the state name for the page, either 'loading' or 'loaded'
> 
> what about 'reload'? it'd be nice to have a complete list somewhere
> 

Done.

> @@ +74,3 @@
> >  
> >    /**
> >     * The tab has changed.
> 
> btw, maybe: the current tab has changed, otherwise it's ambigious
> 

Done.

> @@ +74,5 @@
> >  
> >    /**
> >     * The tab has changed.
> > +   * @param {nsIAccessible} aObject the document contained in the tab, or null
> > +   *    if it is a new tab with no attached document yet.
> 
> "in the tab" -> "by the tab accessible", otherwise ambiguous because the tab
> can contain a lot of nested documents.

Done.

> 
> btw, about the comment style, where it comes from? we use different style in
> c++ part
> 

It is jsdoc: http://code.google.com/p/jsdoc-toolkit/wiki/InlineDocs
It is what gjslint requires.

> @@ +154,5 @@
> > +  if (aObject) {
> > +    let vcDoc = aObject.QueryInterface(Ci.nsIAccessibleCursorable);
> > +    this.pivotChanged(vcDoc.virtualCursor.position);
> > +  } else {
> > +    this.hide();
> 
> VisualPresenter.prototype.pivotChanged makes this.hide() for null aObject.
> So what is this change for?
> 

For the fact that tabSelected could receive a null aObject. I changed this to something else, but it really doesn't matter!

> @@ +249,5 @@
> >    });
> >  };
> >  
> >  AndroidPresenter.prototype.tabSelected = function(aObject) {
> > +  if (!aObject) {
> 
> I really prefer to have
> if (aObject) {
>   // bala
>   return;
> }
> 
> // bla bla
> 

bla bla. I don't understand. Have a comment after the if block to explain the "else"?

> @@ +261,5 @@
> > +        text: newTabUtterance,
> > +        addedCount: newTabUtterance.join('').length,
> > +        removedCount: 0,
> > +        fromIndex: 0
> > +      }
> 
> perhaps you could have a nice helper method wrapping sendMessageToJava?

sendMessageToJava is a wrapper already. How do you mean? For calculating addedCount? I changed it under your suggestion to appear in only one place, so it is not needed any more.

> 
> @@ +263,5 @@
> > +        removedCount: 0,
> > +        fromIndex: 0
> > +      }
> > +    });
> > +    return;
> 
> btw, why you don't reuse pageStateChanged method?
> 

changed it to tabStateChanged, and did just that.

> ::: accessible/src/jsat/UtteranceGenerator.jsm
> @@ +56,5 @@
> >    genForAction: function(aObject, aActionName) {
> >      return [gStringBundle.GetStringFromName(this.gActionMap[aActionName])];
> >    },
> >  
> > +  genForAppStateChange: function (aObject, aAppState) {
> 
> I don't know why you don't like to comment methods, it really keeps code
> more readable
> 

I *do* like commenting methods :) I'll document the UtteranceGenerator.

> @@ +70,5 @@
> > +        return [gStringBundle.GetStringFromName('stoppedState')];
> > +      case 'reload':
> > +        return [gStringBundle.GetStringFromName('reloadState')];
> > +      default:
> > +        return [];
> 
> using an array where you don't need an array looks weird

I think an "utterance" should be consistent throughout the code. So if one method generates an utterance as an array and another as a string, it looks broken. That is why I want to have all utterances generate arrays, and have the presenters choose how to flatten them. Also, in the future an action utterance or state change utterance could have more than one item.

> 
> isn't it nicer to prefix them by 'doc' like 'docReloading' or
> 'docLoadStopped'?

OK. Maybe tabReloading?

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Bug 745986 - Report page loading, loaded and new tabs.

I'll leave documentation of UtteranceGenerator to another patch in bug 748069.

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Bug 745986 - Report page loading, loaded and new tabs.

Oops, a syntax error in the previous patch.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #4)
> > I wonder why do you need this pendingDocuments at all, you could just listen
> > for busy state change event
> > 
> 
> The document accessible object is created with the busy state, so there is
> no state change.

ok, then reorder event

> > you should return if you hit the 'if' above
> > 
> 
> It would be weird if that were the only return in the entire switch block. I
> changed the second if to "else if".

ok, up to you, but you could replace those break on return statements.

> > > +              (event.accessible.role == Ci.nsIAccessibleRole.ROLE_DOCUMENT ||
> > > +               event.accessible.role == Ci.nsIAccessibleRole.ROLE_APPLICATION)) {
> > 
> > I wouldn't prefer to have a local variable for accessible.role
> Done.

thank you for reading me correct :)

> > note: state busy can be fired when something gets loaded, for example, when
> > you click a link to a file to download it
> > 
> 
> This is unfortunate! How does one know that a document is being loaded? It
> seems that there should be an equivalent event for
> EVENT_DOCUMENT_LOAD_COMPLETE, of EVENT_DOCUMENT_LOAD. I am tempted to
> scratch this whole thing and just go with nsIWebProgressListener.

when new document appears then
1) in case of document reload, fire reload event on old document
2) fire reorder on document container when new document is attached
3.1) fire document loaded if document was loaded
3.2) fire document stopped if document loading was stopped

state busy change event is really about busy state.

> > btw, about the comment style, where it comes from? we use different style in
> > c++ part
> > 
> 
> It is jsdoc: http://code.google.com/p/jsdoc-toolkit/wiki/InlineDocs
> It is what gjslint requires.

ok

> > VisualPresenter.prototype.pivotChanged makes this.hide() for null aObject.
> > So what is this change for?
> > 
> 
> For the fact that tabSelected could receive a null aObject. I changed this
> to something else, but it really doesn't matter!

well, this is a little bit weird and perhaps that matters :) but really why do you need null check a Object if pivotChanged does the same.

> bla bla. I don't understand. Have a comment after the if block to explain
> the "else"?

I meant you do:
if (!aObject) {
  // 1
} else {
  // 2
}
but I'd prefer
if (aObject) {
  // 2
} else {
  // 1
}

> 
> > @@ +261,5 @@
> > > +        text: newTabUtterance,
> > > +        addedCount: newTabUtterance.join('').length,
> > > +        removedCount: 0,
> > > +        fromIndex: 0
> > > +      }
> > 
> > perhaps you could have a nice helper method wrapping sendMessageToJava?
> 
> sendMessageToJava is a wrapper already. How do you mean? For calculating
> addedCount? I changed it under your suggestion to appear in only one place,
> so it is not needed any more.

mmm, a helper function like sendAndroidEvent(aEventType, aText);
perhaps with extra argument for addedCount because sometimes you use it but sometimes you don't.

> I *do* like commenting methods :) I'll document the UtteranceGenerator.

cool, just bunch of methods got not documented nowdays :)

> > using an array where you don't need an array looks weird
> 
> I think an "utterance" should be consistent throughout the code. So if one
> method generates an utterance as an array and another as a string, it looks
> broken. That is why I want to have all utterances generate arrays, and have
> the presenters choose how to flatten them. Also, in the future an action
> utterance or state change utterance could have more than one item.

if you're going to that in the future then it's good argument, otherwise I'm not sure.

> > isn't it nicer to prefix them by 'doc' like 'docReloading' or
> > 'docLoadStopped'?
> 
> OK. Maybe tabReloading?

yep, sounds good, that should be friendly, until we want to do similar stuff for ARIA applications

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Bug 745986 - Report page loading, loaded and new tabs.

Reworked this to make it a bit more focus dependant. Also added a new state 'newdoc' for when a document is first attached to a tab.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 618864 [details] [diff] [review]
Bug 745986 - Report page loading, loaded and new tabs.

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

::: accessible/src/jsat/AccessFu.jsm
@@ +100,5 @@
> +      case 'TabOpen':
> +      {
> +        let browser = aEvent.target.linkedBrowser || aEvent.target;
> +        this._pendingDocuments[browser] = true;
> +        this.presenters.forEach(function(p) {p.tabStateChanged(null, 'newtab');});

nit: space after { and before }

@@ +177,5 @@
>              );
>            }
> +          else if (event.state == Ci.nsIAccessibleStates.STATE_BUSY &&
> +                   !(event.isExtraState()) && event.isEnabled()) {
> +            let accRole = event.accessible.role;

it makes sense to name it 'role' (since event and not accevent)

@@ +203,5 @@
> +              if (state.value & Ci.nsIAccessibleStates.STATE_BUSY &&
> +                  this.isNotChromeDoc(docAcc))
> +                this.presenters.forEach(
> +                  function(p) { p.tabStateChanged(docAcc, 'loading'); }
> +                );

loading again? why busy state is not enough?

@@ +208,5 @@
> +              delete this._pendingDocuments[aEvent.DOMNode];
> +            }
> +            if (this.isBrowserDoc(docAcc))
> +              this.presenters.forEach(
> +                function(p) { p.tabStateChanged(docAcc, 'newdoc'); }

when you type an URL into address bar then no 'newdoc' notification, why?

I still don't understand the idea of pendingDocuments I think

@@ +228,5 @@
> +      case Ci.nsIAccessibleEvent.EVENT_DOCUMENT_LOAD_STOPPED:
> +        {
> +          this.presenters.forEach(
> +            function(p) {
> +              p.tabStateChanged(aEvent.accessible, 'stopped');

loadstopped?

@@ +247,5 @@
> +      {
> +        if (this.isBrowserDoc(aEvent.DOMNode)) {
> +          // The document recieved focus, call tabSelected to present current tab.
> +          this.presenters.forEach(
> +            function(p) {p.tabSelected(aEvent.accessible);});

nit: spaces after { and before }

what if I switched tab but control within document is focused? It's not tab select?

@@ +255,5 @@
>          break;
>      }
>    },
>  
> +  isBrowserDoc: function (aObject) {

aObject is not descriptive, please document the method, give a name to function and find better name for argument

@@ +257,5 @@
>    },
>  
> +  isBrowserDoc: function (aObject) {
> +    let parent = aObject.parent;
> +    if (!parent) return false;

wrong style

@@ +261,5 @@
> +    if (!parent) return false;
> +    let domNode = parent.DOMNode;
> +    if (!domNode) return false;
> +    let nodeName = domNode.nodeName;
> +    return (nodeName == 'xul:browser' || nodeName == 'browser');

check local name and namespace instead

@@ +268,5 @@
> +  isNotChromeDoc: function (aObject) {
> +    let location = aObject.DOMNode.location;
> +    if (!location) return false;
> +    return location.protocol != "about:";
> +  },

same for this method

::: accessible/src/jsat/Presenters.jsm
@@ +67,5 @@
>  
>    /**
> +   * The tab, or the tab's document state has changed.
> +   * @param {nsIAccessible} aObject the tab document accessible that has had its
> +   *    state changed, or null if the tab has no document child yet.

I prefer less generic name than aObject

@@ +153,5 @@
>    }
>  };
>  
>  VisualPresenter.prototype.tabSelected = function(aObject) {
> +  let vcPos = (aObject) ?

you don't need to wrap it by ()

@@ +162,5 @@
> +};
> +
> +VisualPresenter.prototype.tabStateChanged = function(aObject, aPageState) {
> +  if (aPageState == "newdoc")
> +    this.hide();

perhaps pivotChanged(null)?

@@ +255,5 @@
>      }
>    });
>  };
>  
>  AndroidPresenter.prototype.tabSelected = function(aObject) {

generic argument names makes code hard readable

@@ +256,5 @@
>    });
>  };
>  
>  AndroidPresenter.prototype.tabSelected = function(aObject) {
> +  // Send a focus event with the full context utterance for this document.

do you mean that pivotChanged send focus event?

::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
@@ +90,5 @@
>  clickAction    =      clicked
>  collapseAction =      collapsed
>  expandAction   =      expanded
>  activateAction =      activated
> +cycleAction    =      cycled

just curious what was a change?

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #9)
> Comment on attachment 618864 [details] [diff] [review]
> Bug 745986 - Report page loading, loaded and new tabs.
> 
> Review of attachment 618864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +203,5 @@
> > +              if (state.value & Ci.nsIAccessibleStates.STATE_BUSY &&
> > +                  this.isNotChromeDoc(docAcc))
> > +                this.presenters.forEach(
> > +                  function(p) { p.tabStateChanged(docAcc, 'loading'); }
> > +                );
> 
> loading again? why busy state is not enough?
> 

I don't understand the question.

> @@ +208,5 @@
> > +              delete this._pendingDocuments[aEvent.DOMNode];
> > +            }
> > +            if (this.isBrowserDoc(docAcc))
> > +              this.presenters.forEach(
> > +                function(p) { p.tabStateChanged(docAcc, 'newdoc'); }
> 
> when you type an URL into address bar then no 'newdoc' notification, why?
> 
> I still don't understand the idea of pendingDocuments I think
> 

This might answer the question above. The desired effect is to have AccessFu speak/display "loading" whenever the browser is busy loading a page.

This could happen in a number of circumstances, whether this is via link, awesomebar, external app, or context menu doesn't matter:
1. User loads a URL in the current page.
2. User loads a URL in a new tab.

If the user does this in an existing tab, we roughly get this sequence:
- state change to busy (old doc)
- reorder (old and new doc parent, the new doc is now here with a BUSY state from creation)
- document load complete (new doc)

if we do this in a new tab we get:
- reorder (new doc parent, the new doc is now here with a BUSY state from creation)
- document load complete (new doc)

So, in both cases we want to send a "loading" message as early as possible. With an existing tab that is when there is a BUSY state change, and for a new tab, it is when the child document is first inserted (and its state is BUSY).

Now, when we get a reorder event, we need to know if we should send a "loading" message or not. We know this by checking the _pendingDocuments hash, if the container is there, it is a new tab, and we should send "loading"! In either case we send a "newdoc" message.

How is the _pendingDocuments populated? By listening to TabOpen DOM events...

Complicated, I know. That is why I filed bug 748082.

> @@ +228,5 @@
> > +      case Ci.nsIAccessibleEvent.EVENT_DOCUMENT_LOAD_STOPPED:
> > +        {
> > +          this.presenters.forEach(
> > +            function(p) {
> > +              p.tabStateChanged(aEvent.accessible, 'stopped');
> 
> loadstopped?
> 

Done. This is the third version of the patch with this line, why didn't you mention this earlier? I feel like this: http://i63.photobucket.com/albums/h145/bennie8016/wackamole.gif

> @@ +247,5 @@
> > +      {
> > +        if (this.isBrowserDoc(aEvent.DOMNode)) {
> > +          // The document recieved focus, call tabSelected to present current tab.
> > +          this.presenters.forEach(
> > +            function(p) {p.tabSelected(aEvent.accessible);});
> 
> nit: spaces after { and before }
> 
> what if I switched tab but control within document is focused? It's not tab
> select?
> 

You will have the entire new context spoken up to the focused item. That is desired.

> @@ +257,5 @@
> >    },
> >  
> > +  isBrowserDoc: function (aObject) {
> > +    let parent = aObject.parent;
> > +    if (!parent) return false;
> 
> wrong style
> 

New line? please say so. Done.

> ::: accessible/src/jsat/Presenters.jsm
> @@ +67,5 @@
> >  
> >    /**
> > +   * The tab, or the tab's document state has changed.
> > +   * @param {nsIAccessible} aObject the tab document accessible that has had its
> > +   *    state changed, or null if the tab has no document child yet.
> 
> I prefer less generic name than aObject
> 

aDocObj

> 
> @@ +256,5 @@
> >    });
> >  };
> >  
> >  AndroidPresenter.prototype.tabSelected = function(aObject) {
> > +  // Send a focus event with the full context utterance for this document.
> 
> do you mean that pivotChanged send focus event?
> 

Send a pivot change message with the full context utterance for this doc.

> ::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
> @@ +90,5 @@
> >  clickAction    =      clicked
> >  collapseAction =      collapsed
> >  expandAction   =      expanded
> >  activateAction =      activated
> > +cycleAction    =      cycled
> 
> just curious what was a change?

A new line added.

Comment 11

[Eitan Isaacson [:eeejay]]

Attachment: Report page loading, loaded and new tabs.

Comment 12

[Eitan Isaacson [:eeejay]]

(In reply to Eitan Isaacson [:eeejay] from comment #10)
> Done. This is the third version of the patch with this line, why didn't you
> mention this earlier? I feel like this:
> http://i63.photobucket.com/albums/h145/bennie8016/wackamole.gif
> 

As the filename suggests, this is a frustrated game of wackamole.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #10)
> > loading again? why busy state is not enough?
> > 
> 
> I don't understand the question.

I meant you send 'loading' on state busy and on reorder event but yes your answer below clarifies this, you send 'loading' on busy for existing documents and 'loading' new document on reorder.

> Now, when we get a reorder event, we need to know if we should send a
> "loading" message or not. We know this by checking the _pendingDocuments
> hash, if the container is there, it is a new tab, and we should send
> "loading"! In either case we send a "newdoc" message.
> 
> How is the _pendingDocuments populated? By listening to TabOpen DOM events...
> 
> Complicated, I know. That is why I filed bug 748082.

yep, complicated. so messages you fire are:
1) open new tab (if empty): new tab
2) open a link in new tab: new tab, newdoc, loadcomplete
3) open a link in existing tab: loading, newdoc, loadcomplete
4) clicking a file link (no document load): loading

> Done. This is the third version of the patch with this line, why didn't you
> mention this earlier? I feel like this:
> http://i63.photobucket.com/albums/h145/bennie8016/wackamole.gif

:))

this patch like a forest for me, slowly digging through it. I have many questions/comments, don't like those pendingDocuments but I don't have anything better, etc. some things are escaping from my eyes :)

> > what if I switched tab but control within document is focused? It's not tab
> > select?
> > 
> 
> You will have the entire new context spoken up to the focused item. That is
> desired.

where is it handled because this.isBrowserDoc(aEvent.DOMNode) under focus event handler works for document accessible only, right?

> > > +    if (!parent) return false;
> > 
> > wrong style
> > 
> 
> New line? please say so. Done.

yes

> > > +   * @param {nsIAccessible} aObject the tab document accessible that has had its
> > > +   *    state changed, or null if the tab has no document child yet.
> > 
> > I prefer less generic name than aObject
> > 
> 
> aDocObj

do you really need that Obj? why not aDocAcc? because document object can be treated as document accessible or document node or both if you like

> > >  AndroidPresenter.prototype.tabSelected = function(aObject) {
> > > +  // Send a focus event with the full context utterance for this document.
> > 
> > do you mean that pivotChanged send focus event?
> > 
> 
> Send a pivot change message with the full context utterance for this doc.

so wrong comment I guess

> > > +cycleAction    =      cycled
> > 
> > just curious what was a change?
> 
> A new line added.

ah, you should keep empty line in the end of each file, please make sure to do that

Comment 14

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #13)
> yep, complicated. so messages you fire are:
> 1) open new tab (if empty): new tab
> 2) open a link in new tab: new tab, newdoc, loadcomplete

new tab, loading, newdoc, loadcomplete

> 3) open a link in existing tab: loading, newdoc, loadcomplete
> 4) clicking a file link (no document load): loading


> > > > +   * @param {nsIAccessible} aObject the tab document accessible that has had its
> > > > +   *    state changed, or null if the tab has no document child yet.
> > > 
> > > I prefer less generic name than aObject
> > > 
> > 
> > aDocObj
> 
> do you really need that Obj? why not aDocAcc? because document object can be
> treated as document accessible or document node or both if you like
> 

Because every other method in has an argument aObject. In the presenter an object is always an accessible object.

> > > >  AndroidPresenter.prototype.tabSelected = function(aObject) {
> > > > +  // Send a focus event with the full context utterance for this document.
> > > 
> > > do you mean that pivotChanged send focus event?
> > > 
> > 
> > Send a pivot change message with the full context utterance for this doc.
> 
> so wrong comment I guess
> 

I fixed that in the new patch from yesterday.

> > > > +cycleAction    =      cycled
> > > 
> > > just curious what was a change?
> > 
> > A new line added.
> 
> ah, you should keep empty line in the end of each file, please make sure to
> do that

Right, sorry.

Comment 15

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #13)
> (In reply to Eitan Isaacson [:eeejay] from comment #10)
> this patch like a forest for me, slowly digging through it. I have many
> questions/comments, don't like those pendingDocuments but I don't have
> anything better, etc. some things are escaping from my eyes :)
> 

I don't either. That is why I opened bug 748082.

Comment 16

[Eitan Isaacson [:eeejay]]

Attachment: Report page loading, loaded and new tabs.

Added newline at end of AccessFu.properties

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #14)
> (In reply to alexander :surkov from comment #13)
> > yep, complicated. so messages you fire are:
> > 1) open new tab (if empty): new tab
> > 2) open a link in new tab: new tab, newdoc, loadcomplete
> 
> new tab, loading, newdoc, loadcomplete

when you click a link in one tab that opens it in new tab then don't you get two 'loading' events: one for existing tab (from busy state change event), another one is for document in new tab (from reorder event)?

> > > aDocObj
> > 
> > do you really need that Obj? why not aDocAcc? because document object can be
> > treated as document accessible or document node or both if you like
> > 
> 
> Because every other method in has an argument aObject. In the presenter an
> object is always an accessible object.

my point was that aObject is perfect name for argument and you shouldn't use it. Sure I don't ask you to change every method but please change those you touch. In this case I'd like to have aDocAcc if the method takes any document accessible object. But for example if takes tab document accessible object then I'd name it as aTabDocAcc. Sometimes I miss 'acc' postfix in names if I deal with accessible object primarily but then I make sure to add 'node' prefix to DOM node objects. For example, aTabDocNode vs aTabDoc or aTabDoc vs aTabDocAcc.

Comment 18

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #17)
> (In reply to Eitan Isaacson [:eeejay] from comment #14)
> > (In reply to alexander :surkov from comment #13)
> > > yep, complicated. so messages you fire are:
> > > 1) open new tab (if empty): new tab
> > > 2) open a link in new tab: new tab, newdoc, loadcomplete
> > 
> > new tab, loading, newdoc, loadcomplete
> 
> when you click a link in one tab that opens it in new tab then don't you get
> two 'loading' events: one for existing tab (from busy state change event),
> another one is for document in new tab (from reorder event)?
> 

as far as I can tell no. the document with the link on it does not change to busy.

Comment 19

[alexander :surkov (:asurkov)]

then why do you need to fire 'loading' on state busy change event?

Comment 20

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #19)
> then why do you need to fire 'loading' on state busy change event?

for links that open in the same tab. the reorder event with the new busy doc only happens after the http request was successful and a document started loading. and we want to send 'loading' as soon as possible.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #20)
> (In reply to alexander :surkov from comment #19)
> > then why do you need to fire 'loading' on state busy change event?
> 
> for links that open in the same tab. the reorder event with the new busy doc
> only happens after the http request was successful and a document started
> loading. and we want to send 'loading' as soon as possible.

so why do you want to say 'loading' asap when a link is open in the same tab and you are ok to say 'loading' somewhere later when link is open in new tab?

Comment 22

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #21)
> (In reply to Eitan Isaacson [:eeejay] from comment #20)
> > (In reply to alexander :surkov from comment #19)
> > > then why do you need to fire 'loading' on state busy change event?
> > 
> > for links that open in the same tab. the reorder event with the new busy doc
> > only happens after the http request was successful and a document started
> > loading. and we want to send 'loading' as soon as possible.
> 
> so why do you want to say 'loading' asap when a link is open in the same tab
> and you are ok to say 'loading' somewhere later when link is open in new tab?

because it is the best we could do under the current circumstances :) that is why i opened bug 748082.

practically, it is not that terrible. if a user opens a link in a new tab, they get immediately "new tab" and after a second or two "loading". the "new tab" message at least gives some feel of responsiveness. if the user was left with nothing, and only after a few seconds heard "loading", than it would not be good.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #22)

> > so why do you want to say 'loading' asap when a link is open in the same tab
> > and you are ok to say 'loading' somewhere later when link is open in new tab?
> 
> because it is the best we could do under the current circumstances :) that
> is why i opened bug 748082.

well, bug 748082 is still not clear

> practically, it is not that terrible. if a user opens a link in a new tab,
> they get immediately "new tab" and after a second or two "loading". the "new
> tab" message at least gives some feel of responsiveness. if the user was
> left with nothing, and only after a few seconds heard "loading", than it
> would not be good.

but in this case the alternative of 'new tab' is 'link clicked', no?

Comment 24

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #23)
> (In reply to Eitan Isaacson [:eeejay] from comment #22
> but in this case the alternative of 'new tab' is 'link clicked', no?

yeah, 'clicked' feedback will always happen. so it is not complete silence.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #24)
> (In reply to alexander :surkov from comment #23)
> > (In reply to Eitan Isaacson [:eeejay] from comment #22
> > but in this case the alternative of 'new tab' is 'link clicked', no?
> 
> yeah, 'clicked' feedback will always happen. so it is not complete silence.

so would it be reasonable alternative to 'newdoc' case? You get consistent behavior and simpler code logic.

Comment 26

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #25)
> (In reply to Eitan Isaacson [:eeejay] from comment #24)
> > (In reply to alexander :surkov from comment #23)
> > > (In reply to Eitan Isaacson [:eeejay] from comment #22
> > > but in this case the alternative of 'new tab' is 'link clicked', no?
> > 
> > yeah, 'clicked' feedback will always happen. so it is not complete silence.
> 
> so would it be reasonable alternative to 'newdoc' case? You get consistent
> behavior and simpler code logic.

first of all, clicking links is not the only way to load a page. a url could be typed in the awesomebar. in the android case a url could be typed in the 'new tab' dialog which is an awesomebar too. a page could be loaded from an external application. an addon could do all sorts of funky things. etc. there are many reasons why a user would be suspended for a couple of seconds of uncertainty.

even if they did click a link, it would be good if they are informed immediately of what happened because of that click. so in the same page case, they will immediately hear 'loading', and if they opened a new tab, they will immediately hear 'new tab', and 'loading' shortly after.

i am not a fan of the complexity either, but for me it is acceptable for now.

Comment 27

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #17)
> my point was that aObject is perfect name for argument and you shouldn't use
> it. Sure I don't ask you to change every method but please change those you
> touch. In this case I'd like to have aDocAcc if the method takes any
> document accessible object. But for example if takes tab document accessible
> object then I'd name it as aTabDocAcc. Sometimes I miss 'acc' postfix in
> names if I deal with accessible object primarily but then I make sure to add
> 'node' prefix to DOM node objects. For example, aTabDocNode vs aTabDoc or
> aTabDoc vs aTabDocAcc.

I opened a bug for dealing with this. "object" arguments must be abolished completely. especially in our code where there are mingling DOM nodes and accessible nodes. bug 751985.

Comment 28

[Eitan Isaacson [:eeejay]]

Attachment: Bug 745986 - Report page loading, loaded and new tabs.

Updated to include better comments about the different invokings of 'loading' messages.

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 621158 [details] [diff] [review]
Bug 745986 - Report page loading, loaded and new tabs.

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

::: accessible/src/jsat/AccessFu.jsm
@@ +79,5 @@
>     */
>    disable: function disable() {
>      dump('AccessFu disable');
>  
> +    this.presenters.forEach(function(p) { p.detach(); });

nit: usually arguments are prefixed by 'a', here and below. Up to you if it's general pattern

@@ +228,5 @@
>        case Ci.nsIAccessibleEvent.EVENT_REORDER:
>          {
> +          let acc = aEvent.accessible;
> +          if (acc.childCount) {
> +            let docAcc = acc.getChildAt(0);

I'd replaced if (acc.childCount) by if (docAcc) (it shouldn't throw iirc)

@@ +231,5 @@
> +          if (acc.childCount) {
> +            let docAcc = acc.getChildAt(0);
> +            if (this._pendingDocuments[aEvent.DOMNode]) {
> +              // This is a document in a new tab. Check if it is
> +              // in a BUSY state (ie. loading), and inform presenters.

ie -> i.e

@@ +263,5 @@
> +              }
> +            );
> +          }
> +          break;
> +        }

I hope indentation of { } goes with style

@@ +324,5 @@
>          break;
>      }
>    },
>  
> +  isBrowserDoc: function (aDocAcc) {

nit: give a name to function and below

@@ +330,5 @@
> +    if (!parent)
> +      return false;
> +    let domNode = parent.DOMNode;
> +    if (!domNode)
> +      return false;

nit: add empty lines after returns (here and below)

@@ +331,5 @@
> +      return false;
> +    let domNode = parent.DOMNode;
> +    if (!domNode)
> +      return false;
> +    let ns = 'http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul';

const instead let?

@@ +335,5 @@
> +    let ns = 'http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul';
> +    return (domNode.localName == 'browser' && domNode.namespaceURI == ns);
> +  },
> +
> +  isNotChromeDoc: function (aObject) {

it'd be nice to document them (and above)

@@ +339,5 @@
> +  isNotChromeDoc: function (aObject) {
> +    let location = aObject.DOMNode.location;
> +    if (!location)
> +      return false;
> +    return location.protocol != "about:";

might be not working for thuderderbird or for UI but I'm fine it it works for all cases you want to support currently.

::: accessible/src/jsat/Presenters.jsm
@@ +67,5 @@
>  
>    /**
> +   * The tab, or the tab's document state has changed.
> +   * @param {nsIAccessible} aDocObj the tab document accessible that has had its
> +   *    state changed, or null if the tab has no document child yet.

maybe 'has no associated document yet'

@@ +82,2 @@
>     */
> +  tabSelected: function tabSelected(aDocObj) {},

btw, usually function names are given in format 'documentName_methodName' in case when different objects have same named methods.

Comment 30

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #29)
> Comment on attachment 621158 [details] [diff] [review]
> Bug 745986 - Report page loading, loaded and new tabs.
> 
> Review of attachment 621158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +228,5 @@
> >        case Ci.nsIAccessibleEvent.EVENT_REORDER:
> >          {
> > +          let acc = aEvent.accessible;
> > +          if (acc.childCount) {
> > +            let docAcc = acc.getChildAt(0);
> 
> I'd replaced if (acc.childCount) by if (docAcc) (it shouldn't throw iirc)
> 

It does throw an exception.

> @@ +82,2 @@
> >     */
> > +  tabSelected: function tabSelected(aDocObj) {},
> 
> btw, usually function names are given in format 'documentName_methodName' in
> case when different objects have same named methods.

Yeah, I think the subclasses should have prefixes. But they are anonymous for now. So that is an item for bug 752127.

Comment 31

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #29)
> Comment on attachment 621158 [details] [diff] [review]
> Bug 745986 - Report page loading, loaded and new tabs.
> 
> Review of attachment 621158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +79,5 @@
> >     */
> >    disable: function disable() {
> >      dump('AccessFu disable');
> >  
> > +    this.presenters.forEach(function(p) { p.detach(); });
> 
> nit: usually arguments are prefixed by 'a', here and below. Up to you if
> it's general pattern
> 

I think that is a bit excessive for in-line methods. Brevity is the point here. We could open a bug for this if you feel strongly about it.

Comment 32

[Eitan Isaacson [:eeejay]]

Attachment: Report page loading, loaded and new tabs. r=surkov

Comment 33

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1bb9382bcbd1

Comment 34

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/1bb9382bcbd1

Comment 35

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #30)

> > I'd replaced if (acc.childCount) by if (docAcc) (it shouldn't throw iirc)
> > 
> 
> It does throw an exception.

then firstChild

Comment 36

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #31)

> > nit: usually arguments are prefixed by 'a', here and below. Up to you if
> > it's general pattern
> > 
> 
> I think that is a bit excessive for in-line methods. Brevity is the point
> here. We could open a bug for this if you feel strongly about it.

ok, let's keep it for now

Comment 37

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #35)
> (In reply to Eitan Isaacson [:eeejay] from comment #30)
> 
> > > I'd replaced if (acc.childCount) by if (docAcc) (it shouldn't throw iirc)
> > > 
> > 
> > It does throw an exception.
> 
> then firstChild

I thought about that too after I landed the patch :(

Comment 38

[Marco Zehe (:MarcoZ)]

Verified fixed in Fennec/15.0a1 2012-05-21