727722 - Create an accessible for HTML table row by frame

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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] ♥]

Attachment: some changes

Comment 2

[alexander :surkov (:asurkov)]

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)]

> @@ +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] ♥]

Attachment: 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)]

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] ♥]

Attachment: patch 727722

Comment 7

[alexander :surkov (:asurkov)]

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)]

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)]

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 (:asurkov)]

(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)]

(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 (:asurkov)]

(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 (:asurkov)]

Michał, could you update the patch please to address comment #12?

Comment 14

[Michał Frontczak [:fxa90id] ♥]

Attachment: fix

Comment 15

[alexander :surkov (:asurkov)]

(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] ♥]

Attachment: 727722 v2

Comment 17

[alexander :surkov (:asurkov)]

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

Comment 18

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9751f50e98d4

Michał, thank you for the fix! Please feel free to take another one. Here's a link https://bugzilla.mozilla.org/buglist.cgi?emailtype1=exact;resolution=---;email1=nobody%40mozilla.org;emailassigned_to1=1;component=Disability%20Access%20APIs;product=Core;status_whiteboard=mentor;status_whiteboard_type=allwordssubstr;query_format=advanced;list_id=2957250

Comment 19

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/9751f50e98d4