275010 - HTML table doesn't support "AtkTable" interface

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Louie Zhao]

Currently, XUL table has "AtkTable" interface. HTML table include 2 kinds: data
table and layout table. data table should also support such interface while
layout table shouldn't export such interface. In order to reach this goal, both
mozilla and AT-Tools need some fix. Please refer to
https://bugzilla.mozilla.org/show_bug.cgi?id=246770#c15.

Comment 1

[Aaron Leventhal]

Sorry, I was wrong.

The code currently purposely turns of the table interface for HTML tables, which is just plain wrong.

See http://lxr.mozilla.org/seamonkey/source/accessible/src/atk/nsAccessibleWrap.cpp#362

If the AT's don't want to implement a special heuristic for HTML content, then we may internally implement heuristic to determine which tables are layout tables . This could be done reasonably accurately, but the AT's would have less ability to tweak it. Perhaps what we need to do is provide an object attribute that provides a hint. However, not providing the table interface for any HTML tables is absolutely wrong.

Comment 2

[Aaron Leventhal]

Attachment: Support AtkTable for html:table and implement heuristic on nsIAccessibleTable. Will expose heuristic as object attribute when they become available.

Ginn, I put a lot of debugging into the heuristic because I expect to make changes after we get it in. We can have someone look at its output from the accessible descriptions exposed on tables in the daily trunk build. Once we have it working I'll make it so that the debug descriptions are only exposed on debug builds.

Comment 3

[Aaron Leventhal]

Attachment: Fix bugs in heuristic, make better looking code, expose reason for decision in description as debugging tool for now

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 229920 [details] [diff] [review]
Fix bugs in heuristic, make better looking code, expose reason for decision in description as debugging tool for now

I'm not big accessible guy and therefore I have more questions than remarks :)

>Index: accessible/public/nsIAccessibleTable.idl
>===================================================================

>+  /**
>+    * Use heuristics to determine if table is most likely used for layout
>+    */
>+  boolean isProbablyForLayout();
> };

Probably is it better something like isPresetntationTable() or?

>+#include "nsPIDOMWindow.h"

Why is it needed?

> NS_IMETHODIMP
> nsAccessNode::GetComputedStyleValue(const nsAString& aPseudoElt, const nsAString& aPropertyName, nsAString& aValue)
> {

>-  viewCSS->GetComputedStyle(domElement, aPseudoElt, getter_AddRefs(styleDecl));
>+  GetComputedStyleDeclaration(domElement, getter_AddRefs(styleDecl));

It looks like aPseudoElt argument of GetComputedStyleValue() isn't used.


>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v
>retrieving revision 1.182

>@@ -1867,10 +1867,15 @@ NS_IMETHODIMP nsAccessibilityService::Ge

>             if (nsAccessNode::HasRoleAttribute(tableContent)) {
>               // Table that we're a descendant of is presentational
>-              return NS_ERROR_FAILURE;
>+              tryFrame = PR_FALSE;
>+            }

>+            nsIFrame *tableFrame = aPresShell->GetPrimaryFrameFor(tableContent);
>+            if (tableFrame->GetType() != nsAccessibilityAtoms::tableOuterFrame) {
>+              // Table that we're a descendant of is not styled as a table,
>+              // and has no table accessible for an ancestor
>+              tryFrame = PR_FALSE;
>             }

probably that should code be in 'else if' block?

> 
>   // If no accessible, see if we need to create a generic accessible because
>Index: accessible/src/html/nsHTMLTableAccessible.cpp
>===================================================================

>+PRBool nsHTMLTableAccessible::HasDescendant(char *aTagName)
>+{
>+  // XXX expose this info via object attributes to AT-SPI
>+  nsCOMPtr<nsIDOMElement> tableElt(do_QueryInterface(mDOMNode));
>+  NS_ENSURE_TRUE(tableElt, PR_FALSE);
>+
>+  nsCOMPtr<nsIDOMNodeList> nodeList;
>+  nsAutoString tagName;
>+  tagName.AssignWithConversion(aTagName);
>+  tableElt->GetElementsByTagName(tagName, getter_AddRefs(nodeList));
>+  NS_ENSURE_TRUE(nodeList, NS_ERROR_FAILURE);
>+  PRUint32 length;
>+  nodeList->GetLength(&length);
>+  
>+  if (length == 1) {
>+    // Make sure it's not the table itself
>+    nsCOMPtr<nsIDOMNode> firstItem;
>+    nodeList->Item(0, getter_AddRefs(firstItem));
>+    return firstItem != mDOMNode;
>+  }
>+

I was impressed :), GetElementsByTagName() begins searching from node for that method was called, right?

>+  return length > 0;
>+}
>+
>+NS_IMETHODIMP nsHTMLTableAccessible::IsProbablyForLayout(PRBool *aIsProbablyForLayout)

Can you put list of instruction how is table type defined? I guess it would be good for code readers.

>+{
>+  // Implement a heuristic to determine if table is most likely used for layout
>+  // XXX do we want to look for rowspan or colspan, especialy that span all but a couple cells
>+  // at the beginning or end of a row/col, and especially when they occur at the edge of a table?

nit: I guess it's usual practice to file bug for xxx section.


>+  // Check role and role attribute
>+  PRBool hasNonTableRole = (Role(this) != ROLE_TABLE);
>+  if (hasNonTableRole) {
>+    ReturnLayoutAnswer(PR_FALSE, "Has role attribute");
>+  }
>+
>+  if (HasRoleAttribute(content)) {
>+    ReturnLayoutAnswer(PR_TRUE, "Has role attribute, and role is table");
>+  }

What's difference between Role(this) and HasRoleAttribute()?


> // --------------------------------------------------------
> // nsHTMLTableHeadAccessible Accessible
> // --------------------------------------------------------
> NS_IMPL_ISUPPORTS_INHERITED0(nsHTMLTableHeadAccessible, nsHTMLTableAccessible)
> 
>@@ -660,5 +834,6 @@ nsHTMLTableHeadAccessible::GetRows(PRInt
>   rv = head->GetRows(getter_AddRefs(rows));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return rows->GetLength((PRUint32 *)aRows);
> }
>+
>Index: accessible/src/html/nsHTMLTableAccessible.h
>===================================================================

>+// XXX For now always on, for testing purposes
>+// |#define SHOW_LAYOUT_HEURISTIC DEBUG| for final release

Is it means SHOW_LAYOUT_HEURISTIC will be defined in debug mode?

>+#define SHOW_LAYOUT_HEURISTIC
>+#ifdef SHOW_LAYOUT_HEURISTIC
>+#define ReturnLayoutAnswer(isLayout, heuristic) \
>+  { *aIsProbablyForLayout = isLayout; \
>+    mLayoutHeuristic = isLayout ? NS_LITERAL_STRING("layout table: ") : NS_LITERAL_STRING("data table: "); \
>+    mLayoutHeuristic += NS_LITERAL_STRING(heuristic); return NS_OK; }
>+#else
>+#define ReturnLayoutAnswer(isLayout, heuristic) { *aIsProbablyForLayout = isLayout; return NS_OK; }
>+#endif

If ReturnLayoutAnswer is macros then should it be RETURN_LAYOUT_ANSWER.

Probably ReturnLayoutAnswer should be defined in IsProbablyForLayout() mehtod since it used in it only?

If IsProbablyForLayout() definition is wrapped by SHOW_LAYOUT_HEURISTIC macros then why do we need the second ReturnLayoutAnswer?


>   if (!found) {
>-    NS_WARNING("Event not supported!");
>+    //NS_WARNING("Event not supported!");

Why?

Comment 5

[alexander :surkov (:asurkov)]

How can I test your patch?

Comment 6

[Aaron Leventhal]

Oops the nsRoleMap.h part won't work -- ignore that part of the patch.

Comment 7

[Aaron Leventhal]

(In reply to comment #4)
> >+  boolean isProbablyForLayout();
I want to keep the word "probably" in the name because we can't know for sure. We're going to use this to provide a hint to screen reader vendors whether to put the user in table navigation mode or not. They can override us, but we provide a good hint using information they can't get as easily or as quickly, such as the border/style info. We'll eventually provide this info via an object attributes, once object attributes get implemented.

> >+#include "nsPIDOMWindow.h"
> Why is it needed?
It's needed for this line:
  nsCOMPtr<nsIDOMViewCSS> viewCSS(do_QueryInterface(doc->GetWindow()));

> >+  GetComputedStyleDeclaration(domElement, getter_AddRefs(styleDecl));
> It looks like aPseudoElt argument of GetComputedStyleValue() isn't used.
That's a preexisting public interface that we provide for scripting authors. For example, Fire Vox could use it to find out pseudo elt style information. I don't see a need to remove it.

> probably that should code be in 'else if' block?
I cleaned it up to add a null frame check and used an || instead of 2 separate blocks.

> I was impressed :), GetElementsByTagName() begins searching from node for that
> method was called, right?
Correct.
> Can you put list of instruction how is table type defined? I guess it would be
> good for code readers.
I put the comments inline instead because I am afraid that a large block comment at the top will get out of date as people forget to update it. However, I will change it if you think it's still better to put it.

> nit: I guess it's usual practice to file bug for xxx section.
Not always. I promise to file sensible followup bugs when we're done with this first patch though.

> What's difference between Role(this) and HasRoleAttribute()?
The role attribute is the dynamic content role attribute that an author can use to override the natural role for an element. It's a string that's optionally defined in the content.
http://developer.mozilla.org/en/docs/Accessible_DHTML
However, Role(this) provides the final enumerated value, the nsIAccesible role constant for the accessible, and it takes into account the fact that the role attribute can overrid what the frame or element type thinks the role should be. That role enumeration is expose via MSAA get_accRole and ATK getRoleCB

> >+// XXX For now always on, for testing purposes
> >+// |#define SHOW_LAYOUT_HEURISTIC DEBUG| for final release
> 
> Is it means SHOW_LAYOUT_HEURISTIC will be defined in debug mode?
Later. For now it will be turned on in release mode too, but this is only for trunk. It will allow me to give a release trunk build to a tester and have them determine if there are many errors, and why, while browsing normally.

> If ReturnLayoutAnswer is macros then should it be RETURN_LAYOUT_ANSWER.
> Probably ReturnLayoutAnswer should be defined in IsProbablyForLayout() mehtod
> since it used in it only?
Both done.

> If IsProbablyForLayout() definition is wrapped by SHOW_LAYOUT_HEURISTIC macros
> then why do we need the second ReturnLayoutAnswer?
The second version doesn't set the accessible description, and we'll use that for final release. Ultimately SHOW_LAYOUT_HEURISTIC is only for debugging.

> >   if (!found) {
> >-    NS_WARNING("Event not supported!");
> >+    //NS_WARNING("Event not supported!");
> 
> Why?
That shouldn't have made it into the patch. 

As far as testing, just build with this and open up a page with accessible explorer from the MSAA SDK. After loading up a tree, type Ctrl+F to do a fine. Select the radio button to search on the role, and use search string "table". The description on the table will tell you if it's considered data or layout, and why. Each time you type F3 you will reach a new table.

Comment 8

[Aaron Leventhal]

Attachment: Updated patch

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #4)
> > >+  boolean isProbablyForLayout();
> I want to keep the word "probably" in the name because we can't know for sure.
> We're going to use this to provide a hint to screen reader vendors whether to
> put the user in table navigation mode or not. They can override us, but we
> provide a good hint using information they can't get as easily or as quickly,
> such as the border/style info. We'll eventually provide this info via an object
> attributes, once object attributes get implemented.

Probably is it better to use the word "heuristics" to be more precisely? As you wish though.

> > >+#include "nsPIDOMWindow.h"
> > Why is it needed?
> It's needed for this line:
>   nsCOMPtr<nsIDOMViewCSS> viewCSS(do_QueryInterface(doc->GetWindow()));

The file includes nsIDOMViewCSS.h already. Is it not enought?

> > >+  GetComputedStyleDeclaration(domElement, getter_AddRefs(styleDecl));
> > It looks like aPseudoElt argument of GetComputedStyleValue() isn't used.
> That's a preexisting public interface that we provide for scripting authors.
> For example, Fire Vox could use it to find out pseudo elt style information. I
> don't see a need to remove it.

I just have in view the next: nsAccessNode::GetComputedStyleValue has aPseudoElt  argument but nsAccessNode::GetComputedStyleDeclaration hans't it. But previosly aPseudoElt was used. It looks strange.

> > Can you put list of instruction how is table type defined? I guess it would be
> > good for code readers.
> I put the comments inline instead because I am afraid that a large block
> comment at the top will get out of date as people forget to update it. However,
> I will change it if you think it's still better to put it.

Correct, let it will be as you did.

> > >+// XXX For now always on, for testing purposes
> > >+// |#define SHOW_LAYOUT_HEURISTIC DEBUG| for final release
> > 
> > Is it means SHOW_LAYOUT_HEURISTIC will be defined in debug mode?
> Later. 

I can't undersntad why will the feauture will be accessible in debug mode only?

Comment 10

[Aaron Leventhal]

> The file includes nsIDOMViewCSS.h already. Is it not enought?
No, it won't compile without that include. it needs the nsPIDOMWindow type.

> > > >+  GetComputedStyleDeclaration(domElement, getter_AddRefs(styleDecl));
> > > It looks like aPseudoElt argument of GetComputedStyleValue() isn't used.
> > That's a preexisting public interface that we provide for scripting authors.
> > For example, Fire Vox could use it to find out pseudo elt style information. I
> > don't see a need to remove it.
> 
> I just have in view the next: nsAccessNode::GetComputedStyleValue has
> aPseudoElt  argument but nsAccessNode::GetComputedStyleDeclaration hans't it.
> But previosly aPseudoElt was used. It looks strange.
I don't understand -- one is a public interface usable by scripts as well, and the other is a static convenience method. Once you have the style declaration you can use pseudo elements with it if you want.

> I can't undersntad why will the feauture will be accessible in debug mode only?
Two things:
1) The feature will be available in both debug and release. However, it won't be available via MSAA or ATK until we expose it through those. That will be the next step. For ATK we will use object attributes, and that's dependant on the work Ming Gao is doing to expose those now. For MSAA we don't know how we'll expose it yet, but we'll likely expose object attributes through some COM interface.
2) The description field is not how we expose the information officially. It's only there for now to help us debug the feature, since we don't have another way to easily look at it. The descriptions of "data table: bordered cells" etc. are so we can easily look at a web page with debugging tools and see what this code is doing. For now we will put that even in the release builds since our testers will use those to debug the feature. Later we will use those debugging descriptions only in the debug builds.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 229959 [details] [diff] [review]
Updated patch

(In reply to comment #10)

> > > > >+  GetComputedStyleDeclaration(domElement, getter_AddRefs(styleDecl));
> > > > It looks like aPseudoElt argument of GetComputedStyleValue() isn't used.
> > > That's a preexisting public interface that we provide for scripting authors.
> > > For example, Fire Vox could use it to find out pseudo elt style information. I
> > > don't see a need to remove it.
> > 
> > I just have in view the next: nsAccessNode::GetComputedStyleValue has
> > aPseudoElt  argument but nsAccessNode::GetComputedStyleDeclaration hans't it.
> > But previosly aPseudoElt was used. It looks strange.
> I don't understand -- one is a public interface usable by scripts as well, and
> the other is a static convenience method. Once you have the style declaration
> you can use pseudo elements with it if you want.
> 

Previously code was:

nsAccessNode::GetComputedStyleValue(const nsAString& aPseudoElt, const nsAString& aPropertyName, nsAString& aValue)
{
  nsCOMPtr<nsIDOMElement> domElement(do_QueryInterface(mDOMNode));
  nsPresContext *presContext = GetPresContext();
  NS_ENSURE_TRUE(domElement && presContext, NS_ERROR_FAILURE);
  nsCOMPtr<nsISupports> container = presContext->GetContainer();
  nsCOMPtr<nsIDOMWindow> domWin(do_GetInterface(container));
  nsCOMPtr<nsIDOMViewCSS> viewCSS(do_QueryInterface(domWin));
  NS_ENSURE_TRUE(viewCSS, NS_ERROR_FAILURE);
  nsCOMPtr<nsIDOMCSSStyleDeclaration> styleDecl;

  // We use aPseudoElt argument for styles getting.
  viewCSS->GetComputedStyle(domElement, aPseudoElt, getter_AddRefs(styleDecl));

Now code is:

nsAccessNode::GetComputedStyleValue(const nsAString& aPseudoElt, const nsAString& aPropertyName, nsAString& aValue)
{
  nsCOMPtr<nsIDOMElement> domElement(do_QueryInterface(mDOMNode));
  if (!domElement) {
    return NS_ERROR_FAILURE;
  }
  nsCOMPtr<nsIDOMCSSStyleDeclaration> styleDecl;

  // We don't use aPseudoElt argument for style getting
  GetComputedStyleDeclaration(domElement, getter_AddRefs(styleDecl));

It shows previously aPseudoElt argument was used but it isn't. Why?

With those r+

Comment 12

[alexander :surkov (:asurkov)]

Attachment: testcase

Probably the testcase will be useful for somebody.