Last Comment Bug 727722 - Create an accessible for HTML table row by frame
: Create an accessible for HTML table row by frame
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Michał Frontczak [:fxa90id] ♥
:
Mentors:
Depends on:
Blocks: treea11y
  Show dependency treegraph
 
Reported: 2012-02-15 22:57 PST by alexander :surkov
Modified: 2012-04-25 07:41 PDT (History)
5 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
some changes (4.61 KB, patch)
2012-03-15 09:27 PDT, Michał Frontczak [:fxa90id] ♥
tbsaunde+mozbugs: review-
Details | Diff | Splinter Review
patch_727722 (6.33 KB, patch)
2012-03-16 12:46 PDT, Michał Frontczak [:fxa90id] ♥
no flags Details | Diff | Splinter Review
patch 727722 (6.06 KB, patch)
2012-03-17 19:19 PDT, Michał Frontczak [:fxa90id] ♥
tbsaunde+mozbugs: review+
roc: review+
Details | Diff | Splinter Review
fix (6.40 KB, patch)
2012-04-17 06:30 PDT, Michał Frontczak [:fxa90id] ♥
no flags Details | Diff | Splinter Review
727722 v2 (7.23 KB, patch)
2012-04-17 06:45 PDT, Michał Frontczak [:fxa90id] ♥
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-02-15 22:57:24 PST
We create an accessible for HTML table row by tag name http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#1695 so we don't create an accessible when CSS display: table-row is used. For that you should override nsIFrame::CreateAccessible for nsTableRowFrame http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableRowFrame.cpp

please add a test under tree/test_table.html (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/tree/test_table.html?force=1)
Comment 1 Michał Frontczak [:fxa90id] ♥ 2012-03-15 09:27:11 PDT
Created attachment 606249 [details] [diff] [review]
some changes
Comment 2 alexander :surkov 2012-03-15 10:59:23 PDT
Comment on attachment 606249 [details] [diff] [review]
some changes

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

::: accessible/src/base/nsAccessibilityService.cpp
@@ +456,5 @@
> +                                                     nsIPresShell* aPresShell)
> +{
> +  nsAccessible* accessible = new nsEnumRoleAccessible(aContent,
> +                                      nsAccUtils::GetDocAccessibleFor(aPresShell),
> +                                                                             roles::ROW);

wrong indentation

@@ +1692,5 @@
>      return accessible;
>    }
>  
>    if (tag == nsGkAtoms::tr) {
> +    nsAccessible* accessible = new CreateHTMLTableRowAccessible(aContent, aDoc);

you don't need it at least for consistency

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

put it into alphabetical order

::: accessible/tests/mochitest/tree/test_table.html
@@ +141,5 @@
> +              }
> +            ]
> +          }
> +        ]
> +      };

I prefer short style
accTree =
  { TABLE: [
    { ROW: [
       { CELL: [ ] }
    ] }
  }

@@ +204,5 @@
>        <th>rowheader</th>
>        <td>cell</td>
>      </tr>
>    </table>
> +  

please add <a> referring to a bug number

::: layout/tables/nsTableRowFrame.cpp
@@ +1353,5 @@
>    }
>  }
> +#ifdef ACCESSIBILITY
> +/*virtual*/
> +already_AddRefed<nsAccessible> nsTableRowFrame::CreateAccessible()

already_AddRefed<nsAccessible> on new line
you don't need /*virtual*/ comment

@@ +1357,5 @@
> +already_AddRefed<nsAccessible> nsTableRowFrame::CreateAccessible()
> +{
> +  nsAccessibilityService* accService = nsIPresShell::AccService();
> +  return accService ? accService->CreateHTMLTableRowAccessible(mContent,PresContext()->PresShell()) : nsnull;
> +}

space after mContent and before PresContext
make sure you don't break 80 char line restriction
Comment 3 Trevor Saunders (:tbsaunde) 2012-03-15 11:39:27 PDT
> @@ +1692,5 @@
> >      return accessible;
> >    }
> >  
> >    if (tag == nsGkAtoms::tr) {
> > +    nsAccessible* accessible = new CreateHTMLTableRowAccessible(aContent, aDoc);
> 
> you don't need it at least for consistency

actually, that's really broken too, I'd like to believe converting an alreadyAddRefed to a raw pointer like that doesn't compile.  It also means we AddRef() an extra time which is a leak and really bad.
Comment 4 Michał Frontczak [:fxa90id] ♥ 2012-03-16 12:46:21 PDT
Created attachment 606686 [details] [diff] [review]
patch_727722

compiles fine, 
alphabetic order, 
correct indent, 
short style, removed comment, 
tried to not break 80 chars
add <a> tag with bug url
Comment 5 Trevor Saunders (:tbsaunde) 2012-03-17 09:48:02 PDT
Comment on attachment 606686 [details] [diff] [review]
patch_727722

>diff --git a/accessible/src/base/nsAccessibilityService.cpp b/accessible/src/base/nsAccessibilityService.cpp
>--- a/accessible/src/base/nsAccessibilityService.cpp
>+++ b/accessible/src/base/nsAccessibilityService.cpp
>@@ -443,13 +443,24 @@ nsAccessibilityService::CreateHTMLTableA
> already_AddRefed<nsAccessible>
> nsAccessibilityService::CreateHTMLTableCellAccessible(nsIContent* aContent,
>                                                       nsIPresShell* aPresShell)
> {
>   nsAccessible* accessible = 
>     new nsHTMLTableCellAccessibleWrap(aContent,
>-                                      nsAccUtils::GetDocAccessibleFor(aPresShell));
>+                                   nsAccUtils::GetDocAccessibleFor(aPresShell));

wrong indentation

>+nsAccessibilityService::CreateHTMLTableRowAccessible(nsIContent* aContent,
>+                                                     nsIPresShell* aPresShell)
>+{
>+  nsAccessible* accessible = 
>+    new nsEnumRoleAccessible(aContent,
>+      nsAccUtils::GetDocAccessibleFor(aPresShell), roles::ROW);

wrong indentation

>   if (tag == nsGkAtoms::tr) {
>-    nsAccessible* accessible = new nsEnumRoleAccessible(aContent, aDoc,
>-                                                        roles::ROW);
>+    nsAccessible* accessible = 
>+            new nsEnumRoleAccessible(aContent,aDoc,roles::ROW);

leave it as is.

>   virtual already_AddRefed<nsAccessible>
>     CreateOuterDocAccessible(nsIContent* aContent, nsIPresShell* aPresShell);
>-

no need to change.

>+  <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=727722">
>+  Mozilla Bug 727722
>+  </a>

keep it with the other links to bugs

otherwise  seems good.
Comment 6 Michał Frontczak [:fxa90id] ♥ 2012-03-17 19:19:50 PDT
Created attachment 606926 [details] [diff] [review]
patch 727722
Comment 7 alexander :surkov 2012-03-20 00:31:51 PDT
Comment on attachment 606926 [details] [diff] [review]
patch 727722

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

Michał, you should ask for review when you file new patch
Comment 8 Trevor Saunders (:tbsaunde) 2012-03-20 10:53:07 PDT
Comment on attachment 606926 [details] [diff] [review]
patch 727722

>   virtual already_AddRefed<nsAccessible>
>     CreateOuterDocAccessible(nsIContent* aContent, nsIPresShell* aPresShell);
>-
>   virtual nsAccessible* AddNativeRootAccessible(void* aAtkAccessible);

leave the blank line

>+  <a target="_blank"
>+     title="when div has display style table-row"

"create a table row by frame for display:table-row"

r=me with that
Comment 9 Trevor Saunders (:tbsaunde) 2012-03-20 10:55:10 PDT
Alex I'm guesing you'll ask a layout person to take a look? I took a quick look and it seems fine, but I don't really know they're style rules.
Comment 10 alexander :surkov 2012-03-20 16:14:18 PDT
(In reply to alexander :surkov from comment #2)

> @@ +1692,5 @@
> >      return accessible;
> >    }
> >  
> >    if (tag == nsGkAtoms::tr) {
> > +    nsAccessible* accessible = new CreateHTMLTableRowAccessible(aContent, aDoc);
> 
> you don't need it at least for consistency

what about this comment?
Comment 11 Trevor Saunders (:tbsaunde) 2012-03-20 19:19:11 PDT
(In reply to alexander :surkov from comment #10)
> (In reply to alexander :surkov from comment #2)
> 
> > @@ +1692,5 @@
> > >      return accessible;
> > >    }
> > >  
> > >    if (tag == nsGkAtoms::tr) {
> > > +    nsAccessible* accessible = new CreateHTMLTableRowAccessible(aContent, aDoc);
> > 
> > you don't need it at least for consistency
> 
> what about this comment?

uh, the way I read that comment is you should use plain new nsEnumRoleAccessible() here, which is what the current ptch does no?  afaik that is consist with what we usually do, but I don't exactly remember.
Comment 12 alexander :surkov 2012-03-20 19:23:17 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> (In reply to alexander :surkov from comment #10)
> > (In reply to alexander :surkov from comment #2)
> > 
> > > @@ +1692,5 @@
> > > >      return accessible;
> > > >    }
> > > >  
> > > >    if (tag == nsGkAtoms::tr) {
> > > > +    nsAccessible* accessible = new CreateHTMLTableRowAccessible(aContent, aDoc);
> > > 
> > > you don't need it at least for consistency
> > 
> > what about this comment?
> 
> uh, the way I read that comment is you should use plain new
> nsEnumRoleAccessible() here, which is what the current ptch does no?  afaik
> that is consist with what we usually do, but I don't exactly remember.

we don't create an accessible by tagname if we create an accessible by frame type (we have an open bug for this issue though). Anyway I think it makes just to be consistent here so if we don't do that for td element then we shouldn't to that for tr element.
Comment 13 alexander :surkov 2012-04-05 04:29:55 PDT
Michał, could you update the patch please to address comment #12?
Comment 14 Michał Frontczak [:fxa90id] ♥ 2012-04-17 06:30:19 PDT
Created attachment 615689 [details] [diff] [review]
fix
Comment 15 alexander :surkov 2012-04-17 06:37:44 PDT
(In reply to Michał Frontczak :fxa90id from comment #14)
> Created attachment 615689 [details] [diff] [review]
> fix

comment #12 is not still addressed?
Comment 16 Michał Frontczak [:fxa90id] ♥ 2012-04-17 06:45:55 PDT
Created attachment 615694 [details] [diff] [review]
727722 v2
Comment 17 alexander :surkov 2012-04-17 06:50:08 PDT
Comment on attachment 615694 [details] [diff] [review]
727722 v2

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

r=me, I'll fix nits before landing. Thank you! Please feel free to take another bug :)

::: accessible/src/base/nsAccessibilityService.cpp
@@ +457,5 @@
> +                                                     nsIPresShell* aPresShell)
> +{
> +  nsAccessible* accessible =
> +    new nsEnumRoleAccessible(aContent,
> +                             nsAccUtils::GetDocAccessibleFor(aPresShell),

no this method on trunk (today landing), I'll fix it locally

::: layout/tables/nsTableRowFrame.cpp
@@ +1352,5 @@
>        NS_ERROR("invalid NS_SIDE arg");
>    }
>  }
> +#ifdef ACCESSIBILITY
> +already_AddRefed<nsAccessible> nsTableRowFrame::CreateAccessible()

you should keep return type on separate line

@@ +1356,5 @@
> +already_AddRefed<nsAccessible> nsTableRowFrame::CreateAccessible()
> +{
> +  nsAccessibilityService* accService = nsIPresShell::AccService();
> +  return accService ? accService->CreateHTMLTableRowAccessible(mContent,
> +    PresContext()->PresShell()): nsnull;

wrong indentation

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