Closed
Bug 783072
Opened 12 years ago
Closed 12 years ago
[AccessFu] Support gesture scrolling and pages
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: eeejay, Assigned: eeejay)
Details
Attachments
(1 file)
6.32 KB,
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #652201 -
Flags: review?(dbolter)
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
(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•12 years ago
|
||
>
> > @@ +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•12 years ago
|
||
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.
Attachment #652201 -
Flags: review?(dbolter) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Sry! https://hg.mozilla.org/mozilla-central/rev/0d9fbfd32c10
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Updated•12 years ago
|
Assignee: nobody → eitan
You need to log in
before you can comment on or make changes to this bug.
Description
•