146065 - Accessibility module assumes HTML, does not work with XHTML

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

For example, in nsHTMLGroupboxAccessible::GetAccName() there is a call to
GetElementsByTagName() which does not work as you'd expect in XHTML documents.
Please see bug 133654 on how to detect when you are in HTML and when you are in
XHTML, and what methods to use etc.

Comment 1

[Aaron Leventhal]

-> Jgaunt

Comment 2

[John Gaunt (redfive)]

heikki, are you saying all calls to GetElementsByTagName need to be split into
cases where we test for XHTML? The bug linked doesn't go into much discussion
about the issue, and only deals with the PARAM tags.

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Yes, unless you know for a fact it won't be used for XHTML or arbitrary XML.
There are probably other functions that are equally bad (basically all
methods/code that deal with element names), but I have not looked at those yet. 

Regarding bug 133654, I should have mentioned to look at the patch...

Comment 4

[John Gaunt (redfive)]

-> aaron

Comment 5

[Aaron Leventhal]

Heikki, is there a way to simplify this to use GetElementsByTagNameNS for both
cases?


   nsCOMPtr<nsIDOMNodeList> allParams; 
-  mydomElement->GetElementsByTagName(NS_LITERAL_STRING("PARAM"),
getter_AddRefs(allParams));
+
+  nsCOMPtr<nsINodeInfo> ni;
+  content->GetNodeInfo(*getter_AddRefs(ni));
+
+  if (ni->NamespaceEquals(kNameSpaceID_XHTML)) {
+    // For XHTML elements we need to take the namespace URI into
+    // account when looking for param elements.
+
+    NS_NAMED_LITERAL_STRING(xhtml_ns, "http://www.w3.org/1999/xhtml");
+
+    mydomElement->GetElementsByTagNameNS(xhtml_ns, NS_LITERAL_STRING("param"),
+                                         getter_AddRefs(allParams));
+  } else {
+    // If content is not XHTML, it must be HTML, no need to worry
+    // about namespaces then...
+
+    mydomElement->GetElementsByTagName(NS_LITERAL_STRING("param"),
+                                       getter_AddRefs(allParams));
+  }    

Comment 6

[Aaron Leventhal]

Attachment: Use GetElementsByTagNameNS

I saved some code by just asking that the namespace be the same as the parent
element. This should be okay, I think. Right?

Comment 7

[Aaron Leventhal]

I should mention that I tested the patch and it works.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Well, it's slightly different from the original but I think it might still be
wrong. Think about arbitrary XML. If you had an element whose name matched an
XHTML element name but the namespace was something completely different,
accessibility code specific to XHTML elements should not be executed. This patch
does not seem to protect against that, but is there some code earlier in the
flow of execution that is taking care of that?

Comment 9

[Aaron Leventhal]

Heikki, I thought about that too. Hmm... okay, I'll come up with a new patch.
But aren't the name spaces available in a .h file somewhere?

Comment 10

[Aaron Leventhal]

Heikki, I think I want to stand by the original patch right now.

There are 4 cases:
GetXULName -- only gets called on XUL namespace elements
XUL groupbox - only for XUL groupboxes
nsHTMLGroupboxAccessible - no way to create this except for HTML and XHTML
nsHTMLTableAccessible - this could be created in XML via CSS display: table. In
that case I'm not actually sure it would be bad to treat a <caption> tag of the
same namespace as the caption for the table.

All of our HTML accessibility objects are not designed for arbitrary styled XML
at all. It assumes certain tag and attribute names. In the future we'll
hopefully do a better job, but right now there's no way in markup to specify
some things without [x]html.

So, really the idea of making styled generic XML accessible is a way off, but
it's something we're not ignoring. A proposal is underway for that.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 147412 [details] [diff] [review]
Use GetElementsByTagNameNS

sr=jst

Comment 12

[Philip K. Warren]

Comment on attachment 147412 [details] [diff] [review]
Use GetElementsByTagNameNS

Looks ok to me.

Comment 13

[Aaron Leventhal]

Heikki, Jst -- 

Technically shouldn't I also be checking the namespace for all the
GetAttribute() and HasAttribute() calls in mozilla/accessibility?

For example, I check the readonly attribute for html form controls, but someone
could do:
<input type="text" xpq:readonly="true" />

Or is it going overboard for mozilla/accessible to check the namespace in places
like that. After all, a readonly attribute of any kind probably indicates that
the thing is readonly, and should be exposed as such to screen readers etc.