783072 - [AccessFu] Support gesture scrolling and pages

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

Three finger swipes should scroll one screen in the opposite direction of the swipe (so it physically like a drag).

If there is no scrolling to be done, we should check if the content is paginated, if it is we should advance or go back a page, depending on direction.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Support accessible gesture scrolling and paging.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 652201 [details] [diff] [review]
Support accessible gesture scrolling and paging.

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

::: accessible/src/jsat/Utils.jsm
@@ +70,5 @@
>        return this.getBrowserApp(aWindow).contentBrowser.contentDocument;
>      return this.getBrowserApp(aWindow).selectedBrowser.contentDocument;
>    },
>  
> +  getAllContentDocuments: function getAllContentDocuments(aWindow) {

Please call this getAllDocuments or getAllDocumentsForWindow or maybe best: getAllAccessibleDOMDocuments. 'ContentDocument' is sort of a loaded semantic for gecko a11y (used for the document representing the main document for a current browser 'tab').

@@ +75,5 @@
> +    let doc = gAccRetrieval.
> +      getAccessibleFor(this.getCurrentContentDoc(aWindow)).
> +      QueryInterface(Ci.nsIAccessibleDocument);
> +    let docs = [];
> +    function getVisibleContentDocumentsInternal(aDocument) {

Please call this getAllChildDocumentsInternal

@@ +137,5 @@
> +
> +      // Second, try to scroll main section or current target if there is no
> +      // main section.
> +      let main = doc.querySelector('[role=main]') ||
> +        doc.querySelector(':target');

Should we be concerned that main could be assigned something unreasonable? (I know role=main can be unreasonable due to author error but that's a risk probably worth taking, however I'm not really sure about :target... ?)

@@ +149,5 @@
> +              main.scrollTop += aPage * main.clientHeight;
> +              return true;
> +            }
> +          } else {
> +            if (s.overflowX == 'scroll' || s.overflowX == 'auto') {

ok

@@ +163,5 @@
> +  },
> +
> +  changePage: function changePage(aWindow, aPage) {
> +    if (aPage == 0)
> +      return true;

I don't see any callers hitting this case.

@@ +168,5 @@
> +
> +    for each (let doc in this.getAllContentDocuments(aWindow)) {
> +      // Get current main section or active target.
> +      let main = doc.querySelector('[role=main]') ||
> +        doc.querySelector(':target');

(Please see earlier concern)

@@ +182,5 @@
> +
> +      for (var i=0; controllers.targetsCount > i; i++) {
> +        let controller = controllers.getTarget(i);
> +        // If the section has a controlling slider, it should be considered
> +        // the page-turner.

Really?

@@ +184,5 @@
> +        let controller = controllers.getTarget(i);
> +        // If the section has a controlling slider, it should be considered
> +        // the page-turner.
> +        if (controller.role == Ci.nsIAccessibleRole.ROLE_SLIDER) {
> +          // Sliders are controlled with ctrl+right/left. I just decided :)

Page up/down is probably better. See: http://dev.aol.com/dhtml_style_guide/#slider

Rest of patch looks fine.

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #2)
> Comment on attachment 652201 [details] [diff] [review]
> Support accessible gesture scrolling and paging.
> 
> Review of attachment 652201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/Utils.jsm
> @@ +70,5 @@
> >        return this.getBrowserApp(aWindow).contentBrowser.contentDocument;
> >      return this.getBrowserApp(aWindow).selectedBrowser.contentDocument;
> >    },
> >  
> > +  getAllContentDocuments: function getAllContentDocuments(aWindow) {
> 
> Please call this getAllDocuments or getAllDocumentsForWindow or maybe best:
> getAllAccessibleDOMDocuments. 'ContentDocument' is sort of a loaded semantic
> for gecko a11y (used for the document representing the main document for a
> current browser 'tab').
> 

Done.

> @@ +75,5 @@
> > +    let doc = gAccRetrieval.
> > +      getAccessibleFor(this.getCurrentContentDoc(aWindow)).
> > +      QueryInterface(Ci.nsIAccessibleDocument);
> > +    let docs = [];
> > +    function getVisibleContentDocumentsInternal(aDocument) {
> 
> Please call this getAllChildDocumentsInternal

Done.

> 
> @@ +137,5 @@
> > +
> > +      // Second, try to scroll main section or current target if there is no
> > +      // main section.
> > +      let main = doc.querySelector('[role=main]') ||
> > +        doc.querySelector(':target');
> 
> Should we be concerned that main could be assigned something unreasonable?
> (I know role=main can be unreasonable due to author error but that's a risk
> probably worth taking, however I'm not really sure about :target... ?)
> 

If :target or [role=main] are scrollable then scroll, if not don't bother.

> @@ +149,5 @@
> > +              main.scrollTop += aPage * main.clientHeight;
> > +              return true;
> > +            }
> > +          } else {
> > +            if (s.overflowX == 'scroll' || s.overflowX == 'auto') {
> 
> ok
> 
> @@ +163,5 @@
> > +  },
> > +
> > +  changePage: function changePage(aWindow, aPage) {
> > +    if (aPage == 0)
> > +      return true;
> 
> I don't see any callers hitting this case.
> 

I'll erase that.

> @@ +168,5 @@
> > +
> > +    for each (let doc in this.getAllContentDocuments(aWindow)) {
> > +      // Get current main section or active target.
> > +      let main = doc.querySelector('[role=main]') ||
> > +        doc.querySelector(':target');
> 
> (Please see earlier concern)
> 

We figured this out on IRC.

> @@ +182,5 @@
> > +
> > +      for (var i=0; controllers.targetsCount > i; i++) {
> > +        let controller = controllers.getTarget(i);
> > +        // If the section has a controlling slider, it should be considered
> > +        // the page-turner.
> 
> Really?
> 

I made that up. You're welcome. We could discuss this more next week.

> @@ +184,5 @@
> > +        let controller = controllers.getTarget(i);
> > +        // If the section has a controlling slider, it should be considered
> > +        // the page-turner.
> > +        if (controller.role == Ci.nsIAccessibleRole.ROLE_SLIDER) {
> > +          // Sliders are controlled with ctrl+right/left. I just decided :)
> 
> Page up/down is probably better. See:
> http://dev.aol.com/dhtml_style_guide/#slider
> 

It is taken by the volume rocker in B2G.

> Rest of patch looks fine.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

> 
> > @@ +182,5 @@
> > > +
> > > +      for (var i=0; controllers.targetsCount > i; i++) {
> > > +        let controller = controllers.getTarget(i);
> > > +        // If the section has a controlling slider, it should be considered
> > > +        // the page-turner.
> > 
> > Really?
> > 
> 
> I made that up. You're welcome. We could discuss this more next week.

Actually I get it now.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 652201 [details] [diff] [review]
Support accessible gesture scrolling and paging.

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

r=me with your local fixes.

Comment 6

[Eitan Isaacson [:eeejay]]

Sry!
https://hg.mozilla.org/mozilla-central/rev/0d9fbfd32c10