285167 - Mozilla doesn't support Scrollbar accessible

Status: REOPENED

(Core :: Disability Access APIs, defect)

Description

[Louie Zhao]

AT-Tools, e.g. GOK, need scrollbar accessible to scroll the webpage in case that
the page is too long. Mozilla needs to export accessible for scrollbar elements.

Comment 1

[Kyle Yuan]

*** Bug 285168 has been marked as a duplicate of this bug. ***

Comment 2

[Louie Zhao]

Attachment: patch v1

Comment 3

[timeless]

Comment on attachment 176723 [details] [diff] [review]
patch v1

>Index: public/nsIAccessibilityService.idl
>+  nsIAccessible createXULScrollbarAccessible(in nsIDOMNode aNode, in boolean aVertical);

>Index: src/atk/nsXULFormControlAccessibleWrap.cpp

this is not the traditional style for initializers in mozilla code, it also
almost certainly doesn't fit in 80 cols:
>+nsXULScrollbarAccessibleWrap::nsXULScrollbarAccessibleWrap(nsIDOMNode* aNode, nsIWeakReference* aShell, PRBool aVertical):
>+nsXULScrollbarAccessible(aNode, aShell, aVertical) 
>+{
>+}

>Index: src/base/nsAccessibilityService.cpp
>+NS_IMETHODIMP nsAccessibilityService::CreateXULScrollbarAccessible(nsIDOMNode *aNode, PRBool aVertical, nsIAccessible **_retval)
>+{
>+#ifdef MOZ_XUL
>+  nsCOMPtr<nsIDocument> document(do_QueryInterface(aNode));
>+  if (!document)
>+    return NS_ERROR_FAILURE;
>+
>+  nsIPresShell *presShell = document->GetShellAt(0);
>+  nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(presShell));
>+
>+  *_retval = new nsXULScrollbarAccessibleWrap(aNode, weakShell, aVertical);
>+  if (! *_retval) 
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  NS_ADDREF(*_retval);
>+#else
>+  *_retval = nsnull;
why don't you return NS_ERROR_NOT_AVAILABLE or NS_ERROR_NOT_IMPLEMENTED?
>+#endif // MOZ_XUL
>+  return NS_OK;
>+}

>Index: src/base/nsDocAccessible.cpp

>+/**
>+  * Our next sibling is an vertical nsScrollbarAccessible object
>+  */
is _a vertical_ (because 'ver' is not a vowel sound)
normal mozilla style for /* is:
/**
 * ...
 */
(although it seems accessible doesn't follow that style, =()

>+NS_IMETHODIMP nsDocAccessible::GetNextSibling(nsIAccessible **aNextSibling)
>+{ 
>+  *aNextSibling = nsnull;
>+
>+  if (mNextSibling) {
>+    if (mNextSibling != DEAD_END_ACCESSIBLE)
>+      *aNextSibling = mNextSibling;
i'd suggest an early return here - the assignment line would be
NS_ADDREF(*aNextSibling = mNextSibling);
>+  }

so that you don't have to indent this code block:
>+  else {
>+    nsCOMPtr<nsIAccessibilityService> accService = do_GetService("@mozilla.org/accessibilityService;1");
>+    if (!accService)
>+      return NS_ERROR_FAILURE;
>+

>+    nsresult rv = accService->CreateXULScrollbarAccessible(mDOMNode, PR_TRUE, aNextSibling);
why bother checking NS_FAILED(rv) ?
>+    if (NS_FAILED(rv) || !*aNextSibling)
isn't it sufficient to null check *aNextSibling?
>+      return rv;
...
>+  }
>+
which would mean this code section could use NS_ADDREF:
>+  NS_IF_ADDREF(*aNextSibling);
>+  return NS_OK;
>+}

>Index: src/xul/nsXULFormControlAccessible.cpp
>@@ -661,6 +665,202 @@ NS_IMETHODIMP nsXULTextFieldAccessible::
>     *aState |= STATE_FOCUSED;
>   }
>   return rv;
> }
> 
>+/**
>+  * XUL Scrollbar
>+  *   nsXULScrollbarAccessible has no its own corresponding DOM node.
... does not have its own ...
this isn't accurate anyway, and you should have a bug reference

>+  *   Creating nsXULScrollbarAccessible with the document and use
>+  *   |this| pointer as the unique Id
are unique id's 64bits large?
>+  */
>+nsXULScrollbarAccessible::nsXULScrollbarAccessible(nsIDOMNode* aNode, nsIWeakReference* aShell, PRBool aVertical):
>+nsLeafAccessible(aNode, aShell)
>+{ 
>+  // Indicate the orientation
>+  mVertical = aVertical;
>+}
>+
>+NS_IMETHODIMP nsXULScrollbarAccessible::GetName(nsAString& aName)
>+{
>+  // Don't need name for nsXULScrollbarAccessible
don't need? it would be 'a name', but i don't like the comment.
>+  aName.Truncate();
>+  return NS_OK;
>+}

>+NS_IMETHODIMP nsXULScrollbarAccessible::GetState(PRUint32 *_retval)
>+{
>+  return nsAccessible::GetState(_retval);
>+}
>+
>+
why the extra line ^ ?
>+NS_IMETHODIMP nsXULScrollbarAccessible::GetExtState(PRUint32 *_retval)

>+NS_IMETHODIMP nsXULScrollbarAccessible::GetValue(nsAString& _retval)
>+{
>+  // TODO:
no bug number for followup work?
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}

>+NS_IMETHODIMP nsXULScrollbarAccessible::GetNextSibling(nsIAccessible **aNextSibling)
see previous comments

>+void nsXULScrollbarAccessible::GetBoundsRect(nsRect& aBounds, nsIFrame** aBoundingFrame)
>+{
>+  // TODO
bug#? note that the syntax for this todo doesn't match the previous one.
>+  return;

>+// Get the value from nsRect directly instead of converting to pixel
pixel_s_ ?
but... converting *what*?

>+  return (*aMaximum < *aCurValue) ? NS_ERROR_FAILURE:NS_OK;
spaces around : ?

>+    // XXX: better way to get scrollable frame ? 
is there a ...

>+    frame = frame->GetFirstChild(nsnull);
>+    scrollFrame = do_QueryInterface(frame);
>+  }
>+  return scrollFrame;
>+}
>+          
  ^ please don't add trailing whitespace

>+PRBool nsXULScrollbarAccessible::IsVisible()
...
>+  if (mVertical)
>+    return isVerticalVisible;
drop this else after return:
>+  else
>+    return isHorizontalVisible;
>+}
>Index: src/xul/nsXULFormControlAccessible.h

Comment 4

[Aaron Leventhal]

I wonder if we shouldn't just be creating an accessible for /toolkit/content/widgets/nativescrollbar.xml
/toolkit/content/widgets/scrollbar.xml
via nsIAccessibleProvider.

Comment 5

[Aaron Leventhal]

Also, Ginn -- do you think we should hold up the new-atk work for this bug? I'm not sure if we must really have it for Firefox 2. What's your thought?

Comment 6

[Ginn Chen]

Scrollbar code changed a lot.
I'm porting the old patch to trunk. Work in progress.
I think it doesn't have conflicts with new-atk.
Also I don't want this patch wait for another year. We may need to port it again.

Comment 7

[Ginn Chen]

Attachment: patch ported to trunk (WIP)

To do:
1) With this patch, it always creates scrollbars no matter they're visible or not. We'd better create them dynamicly.
2) Do we need GetBoundsRect?
I've no idea how to get ScrollableFrame.

Comment 8

[Aaron Leventhal]

(In reply to comment #7)
> 2) Do we need GetBoundsRect?
Can't we inherit that from nsAccessible?


> I've no idea how to get ScrollableFrame.
You should be able to get it from the doc accessible's frame:
201         nsIScrollableFrame *scrollFrame = nsnull;
202         CallQueryInterface(frame, &scrollFrame);
 

Comment 9

[Ginn Chen]

*** Bug 340482 has been marked as a duplicate of this bug. ***

Comment 10

[Aaron Leventhal]

See also bug 109215 about making slider accessible (scrollbars have sliders underneath).

Comment 11

[alexander :surkov (:asurkov)]

mac scrollbar
http://developer.apple.com/documentation/Accessibility/Conceptual/AccessibilityMacOSX/OSXAXRoleReference/chapter_7_section_30.html#//apple_ref/doc/uid/TP40001078-CH211-DontLinkElementID_34

msaa scollbar
http://msdn2.microsoft.com/en-us/library/ms697649.aspx

Comment 12

[Aaron Leventhal]

Bug 109215 for sliders has been fixed. Does that make this any easier?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> Bug 109215 for sliders has been fixed. Does that make this any easier?
> 

It wasn't fixed actually because I roll backed patch due to tree has been closed. Yes it makes easier. All we need is to fix scrollbar.xml to implement nsIAccessibleProvider and to make accessible an anonymous content for scrollbars.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 15

[Marco Zehe (:MarcoZ)]

Comment on attachment 374255 [details] [diff] [review]
patch3

>+    if (prop == "role") {
>       is(roleToString(acc[prop]), roleToString(aAccTree[prop]), msg);
>-    else if (prop != "children")
>+
>+    } else if (prop == "states") {

What does that empty line and the braces around that single-line block mean? Did you intend to add something else, or is this an unfinished edit?

>+     * Return stucture describing subtree of scrollbar

Nit: "structure", a missing r.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)

> What does that empty line and the braces around that single-line block mean?
> Did you intend to add something else, or is this an unfinished edit?

I did this for readability. There "else if" block here containing multiple lines therefore I decided to wrap other blocks by braces also.

Comment 17

[Wayne Mery (:wsmwk)]

Note: this is the last bug blocking  Bug 263575 -  [meta] track porting accessibility fixes from Sun Mozilla 1.7  and bug's reporter address (same as this bug) is dead.  So please close Bug 263575 when this bug is fixed.

Comment 18

[[e-mail]]

Comment on attachment 374255 [details] [diff] [review]
patch3

Looks fine to me (despite the slightly odd output from hg diff) although I don't see the point of the uuid change.

Comment 19

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

Comment on attachment 374255 [details] [diff] [review]
patch3

https://bugzilla.mozilla.org/show_bug.cgi?id=285167>     case nsIAccessibleProvider::XULScale:
>+      *aAccessible = new nsEnumRoleXBLAccessible(aNode, weakShell,
>+                                                 nsIAccessibleRole::ROLE_GROUPING);

I still find the mapping of XULScale to ROLE_GROUPING odd, but it works.
r=me (if you can resolve Neil's uuid concern)

Alexander, I'm not sure I love the method names matching "nsEnumRole*"... but we can IRC on that :)

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> (From update of attachment 374255 [details] [diff] [review])
> Looks fine to me (despite the slightly odd output from hg diff) although I
> don't see the point of the uuid change.

Neil, I changed constants values, for example, XULListCell = 0x00001026; - > const long XULListCell         = 0x0000100C; I must change uuid in this case, right?

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> (From update of attachment 374255 [details] [diff] [review])

> I still find the mapping of XULScale to ROLE_GROUPING odd, but it works.
> r=me (if you can resolve Neil's uuid concern)

me as well. I'll file bug for this.

> Alexander, I'm not sure I love the method names matching "nsEnumRole*"... but
> we can IRC on that :)

Sure, any ideas are great.

Comment 22

[[e-mail]]

Ah, I hadn't noticed that, I thought you were just reformatting and commenting.

Comment 23

[alexander :surkov (:asurkov)]

Enn, any chance to review?

Comment 24

[alexander :surkov (:asurkov)]

checked-in, http://hg.mozilla.org/mozilla-central/rev/f640af4933fc

Comment 25

[alexander :surkov (:asurkov)]

I disabled new scrollbar tests on linux due to failure (http://hg.mozilla.org/mozilla-central/rev/ca15d173817d)

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

This should be backed-out, as it caused a TXul regression on trunk:

 http://graphs.mozilla.org/#show=794371,794398,794384&sel=1241843527,1242189127

Comment 27

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

Backed out. Let's confirm this regressed TXul.
http://hg.mozilla.org/mozilla-central/rev/9aca8f755e95

Comment 28

[alexander :surkov (:asurkov)]

How can we see the regressed test to know what's wrong in this patch?

Comment 29

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

Alex, what should we do with this bug? (Doesn't seem like a priority)

Comment 30

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #29)
> Alex, what should we do with this bug? (Doesn't seem like a priority)

it doesn't look like high priority as we don't have complains about inaccessilbe scrollbars.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.