Last Comment Bug 705179 - Implement initial canvas accessible and baseline test.
: Implement initial canvas accessible and baseline test.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: David Bolter [:davidb]
:
Mentors:
Depends on:
Blocks: 495912
  Show dependency treegraph
 
Reported: 2011-11-24 12:03 PST by David Bolter [:davidb]
Modified: 2012-02-01 13:57 PST (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (13.30 KB, patch)
2011-11-24 12:06 PST, David Bolter [:davidb]
surkov.alexander: review+
Details | Diff | Review
just the layout bit. (1.18 KB, patch)
2011-11-25 07:57 PST, David Bolter [:davidb]
ehsan: review+
Details | Diff | Review

Description David Bolter [:davidb] 2011-11-24 12:03:56 PST
Create a canvas html accessible, and a baseline test.
Comment 1 David Bolter [:davidb] 2011-11-24 12:06:06 PST
Created attachment 576815 [details] [diff] [review]
patch
Comment 2 alexander :surkov 2011-11-24 19:36:44 PST
Comment on attachment 576815 [details] [diff] [review]
patch

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

::: accessible/src/base/nsAccessibilityService.h
@@ +94,5 @@
>    virtual already_AddRefed<nsAccessible>
>      CreateHTMLHRAccessible(nsIContent* aContent, nsIPresShell* aPresShell);
>    virtual already_AddRefed<nsAccessible>
> +    CreateHTMLCanvasAccessible(nsIContent* aContent, nsIPresShell* aPresShell);
> +  virtual already_AddRefed<nsAccessible>

nit: it doesn't need to be virtual, put it in alphabetical order

::: accessible/src/html/nsHTMLCanvasAccessible.cpp
@@ +18,5 @@
> + * Mozilla Foundation
> + * Portions created by the Initial Developer are Copyright (C) 2011
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):

nit: put your name here

@@ +44,5 @@
> +  nsHyperTextAccessible(aContent, aShell)
> +{
> +}
> +
> +NS_IMPL_ISUPPORTS_INHERITED0(nsHTMLCanvasAccessible, nsHyperTextAccessible)

It doesn't sound like we really need this because nsISupports::QueryInterface takes const nsIID&, IUnknown version takes nsIID&. nsQueryInterface class deals with const nsIID& so it doesn't hit IUnknown::QueryInterface.

@@ +52,5 @@
> +{
> +  return nsIAccessibleRole::ROLE_CANVAS;
> +}
> +
> +

nit: extra empty line

::: accessible/src/html/nsHTMLCanvasAccessible.h
@@ +18,5 @@
> + * Mozilla Foundation
> + * Portions created by the Initial Developer are Copyright (C) 2011
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):

nit: your name

@@ +39,5 @@
> +
> +#ifndef _nsHTMLCanvasAccessible_H_
> +#define _nsHTMLCanvasAccessible_H_
> +
> +class nsHTMLCanvasAccessible : public nsHyperTextAccessible

nit: add a comment to describe the class

@@ +42,5 @@
> +
> +class nsHTMLCanvasAccessible : public nsHyperTextAccessible
> +{
> +public:
> +  nsHTMLCanvasAccessible(nsIContent *aContent, nsIWeakReference *aShell);

nit: do type*, not type *

@@ +45,5 @@
> +public:
> +  nsHTMLCanvasAccessible(nsIContent *aContent, nsIWeakReference *aShell);
> +  virtual ~nsHTMLCanvasAccessible() { }
> +
> +  NS_DECL_ISUPPORTS_INHERITED

you don't need this

@@ +51,5 @@
> +  // nsAccessible
> +  virtual PRUint32 NativeRole();
> +};
> +
> +

nit: extra empty line

::: layout/generic/nsHTMLCanvasFrame.cpp
@@ +366,5 @@
>  {
> +  nsAccessibilityService* accService = nsIPresShell::AccService();
> +  if (accService) {
> +    return accService->CreateHTMLCanvasAccessible(mContent,
> +                                                 PresContext()->PresShell());

nit: indent properly second line
Comment 3 David Bolter [:davidb] 2011-11-25 07:57:13 PST
Created attachment 576937 [details] [diff] [review]
just the layout bit.
Comment 4 :Ehsan Akhgari (out sick) 2011-11-25 08:01:49 PST
Comment on attachment 576937 [details] [diff] [review]
just the layout bit.

r=me
Comment 5 David Bolter [:davidb] 2011-11-25 09:07:57 PST
Thanks guys.

https://hg.mozilla.org/integration/mozilla-inbound/rev/39d6a7336070
Comment 6 Ed Morley [:emorley] 2011-11-26 00:28:52 PST
https://hg.mozilla.org/mozilla-central/rev/39d6a7336070

Note You need to log in before you can comment on or make changes to this bug.