Bug 20013

Summary: Windows AX heuristics are poor
Product: WebKit Reporter: Sankar Aditya Tanguturi <sankaraditya+bugzilla>
Component: AccessibilityAssignee: Jon Honeycutt <jhoneycutt>
Status: RESOLVED LATER    
Severity: Normal CC: alice.barraclough, bdakin, eric, jhoneycutt, klinktech, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
Patch for enhancing WebKit Accessibility heuristics
eric: review-
This is the latest patch after I have done all the changes as per code review comments.
eric: review-
Latest patch after Jon and Eric reviewed second time
jhoneycutt: review-
First chunk of patch
jhoneycutt: review-
Comments for all the changes that I have done
none
This is patch contains only the changes for adding a new role i.e. DocumentRole.
none
Patch for changes to populate new role i.e separatorRole
jhoneycutt: review+
Patch for adding and poupluating a new role i.e CellRole
none
Patch for changes to populate new role i.e ListItemRole
none
Updated patch for DocumentRole.
jhoneycutt: review+
This patch contains the changes to populate "Name" attribute properly.
none
Patch for populating Role Attribute properly
none
Updated patch for Document Role
jhoneycutt: review+
Update patch for Separator Role
jhoneycutt: review+
Updated patch for ListItemRole
jhoneycutt: review+
Updated patch for CellRole
jhoneycutt: review+
Updated patch to populate Name attribute properly.
jhoneycutt: review-
Updated patch to populate Role attribute properly
jhoneycutt: review-
Patch to expose only Relavant Accessible Objects
jhoneycutt: review-
Patch to navigate between accessibility parent, children and siblings.
jhoneycutt: review-
Patch to populate the role attribute for list elements.
jhoneycutt: review-
Patch to populate the name attribute for Image Elements
jhoneycutt: review+
Patch to populate name attribute for links and headings.
jhoneycutt: review-
Patch to populate Name attribue for Text Elements
none
Patch to expose list elements and populate role attribute for them correctly.
jhoneycutt: review+
Patch to populate name attribute for text nodes, links and headings.
jhoneycutt: review-
Patch to ignore anonymous block level elements jhoneycutt: review-

Description Sankar Aditya Tanguturi 2008-07-11 16:26:41 PDT
The webkit MSAA support is less than perfect for Windows WebKit.  I've made a patch which improves the support somewhat.  I'll upload it soon.
Comment 1 Sankar Aditya Tanguturi 2008-07-21 18:30:49 PDT
Created attachment 22417 [details]
Patch for enhancing WebKit Accessibility heuristics

Webkit accessibility heuristics are not great. When I load a page in safari and invoke Inspect32 tool, proper accessible tree is not exposed. I have enhanced the webkit heuristics to expose relavant accessibile objects equal or better to that of Firefox 2.0.0.14. I have tested this path for nearly 20 web pages. Output is equal to that of Firefox 2.0.0.14. Also, navigating between parent and siblings is not proper in Inspect32. I have fixed those bugs also.
Comment 2 Eric Seidel (no email) 2008-07-22 10:04:17 PDT
Comment on attachment 22417 [details]
Patch for enhancing WebKit Accessibility heuristics

This looks pretty good.  A few comments:

1.  We use 4 spaces, not tabs in webkit.

+	virtual AccessibilityObject* accFirstChild() const ;
+    virtual AccessibilityObject* accLastChild() const ;
+
+    virtual AccessibilityObject* previousAccessibilitySibling() const ;
+    virtual AccessibilityObject* nextAccessibilitySibling() const ;

These aren't consistent.

Probably accFirstChild should change to:

firstAccessibilityChild() instead.

There seem to be some strange uses of "const" in this class... 

+WTF::HashSet<String> createTextFormattingElementNamesHash() 

No need to use WTF:: there, I believe using namespace WTF should already have been done at some point in a header for you.

Probably better to have createTextFormattingElementNamesHash return a HashSet<String>*, since HashSet does a full deep copy in its copy constructor, so you'd end up with a lot of copying going on during return.

+    HashSet<String> elementHashSet ;

extra space.

Extra space:
+    return elementHashSet;
+
+}

It seems m_accessibilityID is misnamed (not your fault).  It should probably be "m_indexInParent".

No need for an "else" after return in:
+        return textFormattingElementNames.contains(m_renderer->element()->nodeName().lower());
+    else
+        return false;

The lower() here is not necessary:
+        return textFormattingElementNames.contains(m_renderer->element()->nodeName().lower());


if you use a case-insensitive hash:

HashSet<String, CaseFoldingHash>

It also appears that the more common paradigm is not to use a HashSet* as I just said above, but rather to just init the hash in the function.  See:
http://trac.webkit.org/browser/trunk/WebCore/xml/XMLHttpRequest.cpp#L90

+bool AccessibilityRenderObject::isLinkOrListItemElement() const
should probably use a Hash in the same pattern as shown in the link above.  Darin Adler once figured out what the break-even point was... I think in terms of code complexity, anything past about 4 items looks cleaner in a Hash.

Again, extra space, and no need for "else" after return:
+                 m_renderer->element()->hasTagName(dtTag))
+        return true;
+    else
+        return false;
+
+}

Extra spaces at end of line:
     if (!m_renderer)
-        return String();
+        return String();    

We have an append() function which takes a Vector and a String, so this:
+            String elementTitle = childrenVector[i]->title();
+            elementText.append(elementTitle.characters(),elementTitle.length());

should just be:
append(elementText, elementTitle);

Again, no else needed after return:
+            if (!alt.isNull())
+                return alt.string();
+            else
+                return String();
+        }

No extra space needed before ;
+        return ariaText.isNull() || ariaText.isEmpty() || !ariaText.stripWhiteSpace().length() ;

I believe the WebKit style guides say the outer if here should have { }:
+    if (isLinkOrListItemElement())         
+        if (children().size() == 0)
+            return true;
+        else
+            return false;
+

No spaces needed before ):
+    if (m_renderer->element() && m_renderer->element()->hasTagName(htmlTag) )
+        return true;
+
+    if (m_renderer->element() && m_renderer->element()->hasTagName(bodyTag) )
+        return true;

Add  { } remove use of else after return:
+    if (m_renderer->isBlockFlow() )  
+      if (m_renderer->isAnonymousBlock())   
+          return true;
+      else  
+          return false;

+    if (node && (node->hasTagName(tdTag) ||
+                 node->hasTagName(thTag)
+                                 ))                                 
+        return CellRole;

)) should be on the same line as the rest of it.  It can all be on one line I bet.  We don't have a strong 80-column limit in WebKit, although most folks try to wrap their text to make it easy to read.

Space after if before (, but not before ):
+    if(node && node->hasTagName(liTag) )
+        return ListItemRole;
+
+    if(node && node->hasTagName(hrTag) )
+        return SeparatorRole;

Besides the strange formatting, this looks fine to me.  Jon Honeycutt should really review it for correctness.  I'll ask him to do so.
Comment 3 Eric Seidel (no email) 2008-07-22 10:05:52 PDT
Comment on attachment 22417 [details]
Patch for enhancing WebKit Accessibility heuristics

Marking r- because of the issues mentioned above.  Jon should still look at this for correctness.

We have the ability to make various AX test cases in DumpRenderTree now, although Sam Weinig (weinig in #webkit) would have to explain how that would work here.
Comment 4 Jon Honeycutt 2008-07-22 17:47:39 PDT
Thanks for working on this, Sankar! I am really happy to see you make progress on this.

I ran into an issue when testing your patch with a change you made to the roleValue() method:
-    if (node && node->isLink()) {
-        if (m_renderer->isImage())
+	if (node && node->isLink()) {
+        if (isImage())

isImage() calls roleValue(), so this leads to infinite recursion. I think you meant to use isNativeImage().

Another issue with a change to addChildren():
         } else
+			obj->setAccessibilityID(childCounter++);
+            obj->setAccessibleParent(this);
             m_children.append(obj);

You are missing braces around those statements, so only the call to setAccessibilityID() is within the else condition.

With these changes, the accessibility tree on Windows looks much better! However, I wonder if your work here leads to changes in the accessibility tree on, for example, the Mac platform. If so, it may be necessary to make some changes to avoid breaking accessibility on the Mac.

Have you tried running the accessibility layout tests on a Mac after this patch? None of the accessibility tests will run on Windows currently, but I believe Sam Weinig is working to add support for this.

As a last note, your ChangeLog entries should explain any non-obvious changes that have been made to the code. You can look through the other ChangeLog entries to gain an idea of how verbose they should be.

Again, thanks! I will CC: our cross-platform accessibility engineers and let them weigh in.
Comment 5 Sankar Aditya Tanguturi 2008-07-22 21:23:57 PDT
Created attachment 22444 [details]
This is the latest patch after I have done all the changes as per code review comments.

I have made all the changes as per the code review comments. I have updated the patch. I am uploading the new patch.
Comment 6 Sankar Aditya Tanguturi 2008-07-22 21:25:45 PDT
(In reply to comment #5)
> Created an attachment (id=22444) [edit]
> This is the latest patch after I have done all the changes as per code review
> comments.
> I have made all the changes as per the code review comments. I have updated the
> patch. I am uploading the new patch.

Thanks Eric and Jon for your review comments. I have made all the required changes and I have uploaded the newly created patch. Please check it out.
Comment 7 Eric Seidel (no email) 2008-07-22 22:05:06 PDT
Comment on attachment 22444 [details]
This is the latest patch after I have done all the changes as per code review comments.

In general this patch looks fine.  There are still a few style issues, and Jon should look again for correctness.  I don't know if you have access to a Mac, but I can run the layout tests on mine tomorrow if you can't.

Tab:
+	virtual bool isTextFormattingElement() const;

What does
+    const AccessibilityObject* accessibleParent() {  return m_accessibleParent ;}
+    void setAccessibleParent(const AccessibilityObject* );

accessibleParent mean? I assume you mean accessibilityParent?  "accessible" reads "one that I can reach" instead of what I think you mean as "parent of this dom/render object from the perspective of the accessibility interfaces"

Tab:
+		elementHashSet.add(pTag.toString());

No spaces before ;
+    virtual AccessibilityObject* firstAccessibilityChild() const ;
+    virtual AccessibilityObject* lastAccessibilityChild() const ;
+
+    virtual AccessibilityObject* previousAccessibilitySibling() const ;
+    virtual AccessibilityObject* nextAccessibilitySibling() const ;
+

Wrong (by webkit style) spacing:
+    const AccessibilityObject* accessibleParent() {  return m_accessibleParent ;}
+    void setAccessibleParent(const AccessibilityObject* );



This looks like it won't work dynamically.  m_indexInParent is never updated when the parent's children are changed.  Maybe the point of this patch was only to handle static content?

I could see this leading to a crash however, if m_indexInParent is set, and then siblings are later removed, then next time nextAccessibilitySibling is called on this object, m_indexInParent + 1 might be much larger than siblingsVector.size(), and thus fail to trigger the:
+    if (m_indexInParent + 1 == siblingsVector.size())
+        return 0;

causing a crash in 
+    return siblingsVector[m_indexInParent + 1].get();

Am I right?


This line:
return ariaText.isNull() || ariaText.isEmpty() || !ariaText.stripWhiteSpace().length()
Should just be:
return ariaText.isNull() || ariaText.impl()->containsOnlyWhiteSpace();

I'm surprised that containsOnlyWhiteSpace() is only available on StringImpl, it really should be on String as well.  You could add:
bool containsOnlyWhiteSpace() { return isNull() || m_impl-> containsOnlyWhiteSpace(); } to PlatformString.h if you'd like and then just call:
return text().isOnlyWhiteSpace();

Yeah, again, don't use "stripWhiteSpace()" as that's unecessary mallocs:
if (textUnderElement().stripWhiteSpace().length() != 0)  

Looks like you need an addition to String there as well.  Or you can manually call the StringImpl method, but it would be better to just add containsOnlyWhiteSpace() to String.

No need for the else after the return here:
+            if (parentObjectUnignored()->ariaRoleAttribute() == MenuItemRole
+                || parentObjectUnignored()->ariaRoleAttribute() == MenuButtonRole)
+                return true;
+            else
+                return false;

This:
+        if (children().size() == 0)
+            return true;
+        else
+            return false;

should be:
return children().isEmpty();

Space between for and (, and no space between ) and ;
+    for(parentObj = m_object->parentObject() ; parentObj && parentObj->accessibilityIsIgnored(); parentObj = parentObj->parentObject()) { }


I wonder if this whole clause:
+    AccessibilityObject* parentObj = m_object->parentObject();
+    
+    for(parentObj = m_object->parentObject() ; parentObj && parentObj->accessibilityIsIgnored(); parentObj = parentObj->parentObject()) { }

shouldn't be turned into a function.  Like:

AccessibilityObject* parentObj = m_object->nonIgnoredParent();
if (parentObj)...

or similar.

Again, the change looks good (to me, not knowing much about AX), however you're tripping up on WebKit style, and a few nits here and there.  This is kinda expected... sadly it take a while to get used to hacking on WebKit.  But we're here to help.  Happy to answer questions for you in #webkit.


Oh, and:
+2008-07-21  U-stanguturi\Sankar Tanguturi  <set EMAIL_ADDRESS environment variable>

the prepare-changeLog script looks at your $USER and $EMAIL_ADDRESS environment variables.  looks like yours aren't set to useful things.  You should just edit the ChangeLogs manually to fix the name and email line.


Jon should look at this patch as well, but this will need one more round of style-cleanup before we can land.
Comment 8 Sam Weinig 2008-07-22 23:55:52 PDT
We would have a much easier time evaluating and reviewing this if you were to break this patch up into small testable chunks.  For instance, I would start with just adding support for the DocumentRole and adding a test case for it.  Making this large a change, and with many unrelated pieces is something we try to avoid.
Comment 9 Sankar Aditya Tanguturi 2008-07-23 16:59:57 PDT
Created attachment 22459 [details]
Latest patch after Jon and Eric reviewed second time

Eric,
Thanks for the code review comments. I have made all the required changes.
Please check it out.
Comment 10 Sankar Aditya Tanguturi 2008-07-23 17:01:49 PDT
Eric,
This patch works well for static pages. At no other location, we are modifying (adding/removing) children from m_children variable. So, I don't think, my code will crash at some point.

As you specified, I have made all the required changes and uploaded the new patch.

Thanks.
Comment 11 Sankar Aditya Tanguturi 2008-07-23 17:04:49 PDT
Sam,
Sorry for dumping all the changes in one shot. I have uploaded the modified patch. From next time, I will upload the patches in small chunks as you specified. All the features that are posted in this patch are inter-related. So, I thought I can post it in one shot.

~ Thanks.

(In reply to comment #8)
> We would have a much easier time evaluating and reviewing this if you were to
> break this patch up into small testable chunks.  For instance, I would start
> with just adding support for the DocumentRole and adding a test case for it. 
> Making this large a change, and with many unrelated pieces is something we try
> to avoid.

Comment 12 Jon Honeycutt 2008-07-23 21:15:20 PDT
Hi, Sankar. Looking good, a few comments:

+2008-07-21  U-stanguturi\Sankar Tanguturi  sankaraditya@gmail.com

I think U-stanguturi is the name of the machine you're working on. You should remove that part of the name field in your ChangeLogs. Also, your ChangeLogs still need to explain the changes you are making, and you should put the URL of this bug at the top of the entry. Check out the other ChangeLog entries to see how this should look.

+    unsigned indexInParent() { return m_indexInParent; }
+    const AccessibilityObject* accessibilityParent() { return m_accessibilityParent; }
These methods can be const.

+        elementHashSet.add(bTag.toString());
+        elementHashSet.add(bTag.toString());
You added the same string to this HashSet twice.

+    ||  m_renderer->element()->hasTagName(tableTag)
+    ||  m_renderer->element()->hasTagName(tdTag)
+    ||  m_renderer->element()->hasTagName(thTag)
I think you could use RenderObject::isTable(), isTableCell(), and isTableRow() instead of hasTagName(). These lines should also be indented 1 more level.

+bool AccessibilityRenderObject::isTextFormattingElement() const
+{
+    HashSet<String, CaseFoldingHash>* textFormattingElementNames = createTextFormattingElementNamesHash();
+
+    if (m_renderer && m_renderer->element())
+        return textFormattingElementNames->contains(m_renderer->element()->nodeName());
+
+    return false;
+
+}
Does Firefox ignore these items based on their tag name, or whether they are inline? For example, is <i style="display: block">test</i> ignored?

>+    if (m_renderer && m_renderer->element())
>+        return m_renderer->element()->isLink() 
>+            || elementHashSet->contains(m_renderer->element()->nodeName());
Can you use m_renderer->isListItem(), or is the HashSet of list item tag names necessary? What if an element with a different tag name sets its display type to list-item? What does Firefox do in that case?

Regarding the use of tag names in general, I wonder how is this affected by ARIA. I think Beth or Alice need to comment here.

>+    if (m_renderer->element()->isLink() || isHeading()) {
>+        const Vector< RefPtr<AccessibilityObject> >& childrenVector = children();
>+        Vector<UChar> elementText;
>+        for (unsigned i = 0; i < childrenVector.size(); ++i) {
>+            elementText.append(' ');
>+            String elementTitle = childrenVector[i]->title();
>+            append(elementText, elementTitle);
>+        }
>+
>+        return String::adopt(elementText).stripWhiteSpace();
>+        
>+    }
You can use the AccessibilityChildrenVector typedef here.

Is it possible to use textUnderElement()? If not, textUnderElement() should probably be modified, rather than having this code in title().

>+            Element* elt = static_cast<Element*>(node);
>+            const AtomicString& alt = elt->getAttribute(altAttr);
>+            if (!alt.isNull())
>+                return alt.string();
>+            return String();
>+        }
>+    }
Can probably write this as:
return static_cast<Element*>(node)->getAttribute(altAttr);

>         String ariaText = text();
>-        return ariaText.isNull() || ariaText.isEmpty();
>+        return ariaText.isNull() || ariaText.containsOnlyWhitespace();
Can probably now write this as:
return text().containsOnlyWhitespace().

>     if (m_renderer->isText()) {
>         // static text beneath MenuItems and MenuButtons are just reported along with the menu item, so it's ignored on an individual level
>-        if (parentObjectUnignored()->ariaRoleAttribute() == MenuItemRole ||
>-            parentObjectUnignored()->ariaRoleAttribute() == MenuButtonRole)
>+        if (!textUnderElement().containsOnlyWhitespace())  {
>+            if (parentObjectUnignored()->ariaRoleAttribute() == MenuItemRole
>+                || parentObjectUnignored()->ariaRoleAttribute() == MenuButtonRole)
>+                return true;
>+            return false;
>+        } else {
>             return true;
>-         return m_renderer->isBR() || !static_cast<RenderText*>(m_renderer)->firstTextBox();
>+        }
>     }
Our style is to prefer early return, so this should be written as:
if (m_renderer->isText()) {
    if (textUnderElement().containsOnlyWhitespace())
        return true;
    AccessibilityRole role = parentObjectUnignored()->ariaRoleAttribute();
    return role == MenuItemRole || role == MenuButtonRole;
}

>+    if (isLinkOrListItemElement()) {
>+        return children().isEmpty();
>+    }
No need for braces around a single-line if.

>+    if (m_renderer->isBlockFlow()) {
>+        if (m_renderer->isAnonymousBlock())
>+            return true;
>+        return false;
>+    }
Can be written as:
if (m_renderer->isBlockFlow())
    return m_renderer->isAnonymousBlock();

>+    if (node && node->hasTagName(tableTag))
>+        return TableRole;
>+
>+    if (node && (node->hasTagName(tdTag) || node->hasTagName(thTag)))
>+        return CellRole;
Same as earlier; can use isTable(), isTableCell().

>+    if(node && node->hasTagName(liTag))
>+        return ListItemRole;
>+
>+    if(node && node->hasTagName(hrTag))
>+        return SeparatorRole;
RenderObject has isListItem(), isHR().

+    bool containsOnlyWhitespace() { return isNull() || m_impl->containsOnlyWhitespace(); }
Looks like this can be const.

You still need to explain the changes in the ChangeLog before this can be r+ed. I don't understand some of the changes or their implications, so I'd like to have Beth or Alice's opinion, also. However, overall, this looks good!
Comment 13 Jon Honeycutt 2008-07-23 21:16:37 PDT
Comment on attachment 22459 [details]
Latest patch after Jon and Eric reviewed second time

r- for need of proper ChangeLog and for the above comments.
Comment 14 Jon Honeycutt 2008-07-23 21:38:37 PDT
I think Eric is right about the children changing after the tree has been created. When the children of an AccessibilityObject change, childrenChanged() is called. You can use this to update their m_indexInParent and m_accessibilityParent.

As Sam said, you should probably break this patch up into smaller patches. Fixing the navigation between elements is definitely distinct from the other changes in this patch.
Comment 15 Eric Seidel (no email) 2008-07-24 01:58:02 PDT
Comment on attachment 22459 [details]
Latest patch after Jon and Eric reviewed second time

Jon gave you plenty of comments already, but here is another:

+        elementHashSet.add(bTag.toString());

Those shouldn't need an explicit .toString() call:

 operator const String&() const { return m_string; }

Should make AtomicStrings transparently convert.  You should just be able to do:

+        elementHashSet.add(subTag);

Of course this is all moot if you find out that you should be testing:

element->style()->display() == INLINE

instead.

And unfortunately this is not an unusual number of reviews for someone's first patch to WebKit to go through.  Hopefully you don't find this process too frustrating.  Large patches like this are difficult to do as one's "first patch" :)  Especially since WebKit has a different style from so much other C++ code.
Comment 16 Eric Seidel (no email) 2008-07-24 02:00:12 PDT
(In reply to comment #14)
> As Sam said, you should probably break this patch up into smaller patches.
> Fixing the navigation between elements is definitely distinct from the other
> changes in this patch.

If you choose to break this up into smaller patches (which is always encouraged, as reviews for smaller patches go much quicker and are easier to land in parallel while other patches are still under review), you can either attach multiple patches to this bug, or you can file additional bugs and mark this bug as dependent on those bugs.
Comment 17 Sankar Aditya Tanguturi 2008-07-24 19:32:53 PDT
Created attachment 22470 [details]
First chunk of patch

AS Jon,Eric and Sam suggested, I have broken down the large patch into small chunks. I am uploading the first chunk. This patch contains the changes for addition of roles, filtering of MSAA tree, pouplation of various attributes.
Comment 18 Sankar Aditya Tanguturi 2008-07-24 19:34:26 PDT
Created attachment 22471 [details]
Comments for all the changes that I have done

This file contains the comments for all the changes that I have done.
I couldn't find out how to embed the comments in the changelog. So, I have
created a separate file explaining each and every change that I have done.
Comment 19 Eric Seidel (no email) 2008-07-29 13:41:58 PDT
Comment on attachment 22470 [details]
First chunk of patch

I assume Sankar meant to mark this for review.
Comment 20 Eric Seidel (no email) 2008-07-29 13:43:10 PDT
(In reply to comment #18)
> Created an attachment (id=22471) [edit]
> Comments for all the changes that I have done
> 
> This file contains the comments for all the changes that I have done.
> I couldn't find out how to embed the comments in the changelog. So, I have
> created a separate file explaining each and every change that I have done.

The ChangeLog is just a text file.  You can just add the comments inline.  prepare-ChangeLog is log is just a script which does all the boiler-plate editing to the ChangeLog after you've prepared a patch.  You should feel free to edit a ChangeLog by hand. :)
Comment 21 Eric Seidel (no email) 2008-07-29 13:47:23 PDT
Comment on attachment 22470 [details]
First chunk of patch

I still think that:
+        elementHashSet.add(bTag.toString());

probably don't need the explicit .toString() calls.

+        elementHashSet.add(bTag);
Should "just work".  BUt maybe I'm wrong.

This:
+            String elementTitle = childrenVector[i]->title();
+            append(elementText, elementTitle);
Can be written:
+            append(elementText, childrenVector[i]->title());
Either way is fine.  :)

Sad that we have to copy a string here:
+        return String::adopt(elementText).stripWhiteSpace();
But it should be fine.

No { } around single line if clauses:
+    if (m_renderer->isBlockFlow()) {
+        return m_renderer->isAnonymousBlock();
+    }

This change looks great to me!  I'll ask Jon to look over it one more time.
Comment 22 Mark Rowe (bdash) 2008-07-29 13:49:19 PDT
Comment on attachment 22470 [details]
First chunk of patch

The ChangeLog entry here contains only a link to an unrelated changeset in Trac.  Please provide information about the purpose of the change, and annotate the changelog with details about specific portions of the change where appropriate.
Comment 23 Eric Seidel (no email) 2008-07-29 13:50:04 PDT
Comment on attachment 22470 [details]
First chunk of patch

I r+'d this with requested changes, thus I'll volunteer to make those edits before I land it.
Comment 24 Mark Rowe (bdash) 2008-07-29 13:53:24 PDT
Comment on attachment 22470 [details]
First chunk of patch

This patch also still looks to make several unrelated changes.  I would think it could be split further into patches that contain only related changes.
Comment 25 Sam Weinig 2008-07-29 14:17:18 PDT
This still needs tests.
Comment 26 Jon Honeycutt 2008-07-29 14:23:15 PDT
Comment on attachment 22470 [details]
First chunk of patch

r-ing until I have a chance to test this more.
Comment 27 Jon Honeycutt 2008-07-29 15:28:01 PDT
Testing whether an object can be ignored by checking the tag name does not seem to work well.

This should be present in the tree as a button:
<i role="button">Test.</i>

This should be present as a container:
<i style="display: block;">Test.</i>

This should be present as a list item:
<i style="display: list-item;">Test.</i>

This can be ignored:
<i>Test.</i>

This is how these objects are presented by Firefox 3. However, with this patch, the accessibility of the <i> tag is ignored in all of these cases.

Smaller issues:
+    if(m_renderer->isListItem())
+        return ListItemRole;
+
+    if(m_renderer->isHR())
+        return SeparatorRole;
+
+    if(isListElement(node))
+        return ListRole;
+
Need spaces between if and (.

+        case WebCore::DocumentRole:
+          return ROLE_SYSTEM_DOCUMENT;
+        case WebCore::CellRole:
+          return ROLE_SYSTEM_CELL;
+        case WebCore::ListItemRole:
+          return ROLE_SYSTEM_LISTITEM;
+        case WebCore::SeparatorRole:
+          return ROLE_SYSTEM_SEPARATOR;
Needs two more spaces of indentation before each return.


As Sam and Mark said, this still needs to be broken down; the patch tries to fix issues with list items, tables, links, images, inline elements, text elements, separators, and documents, when each of these is a separate issue.

It would be best if you could submit a patch for each of these small items, along with a test.

I hope this doesn't put you off of working on this! :)
Comment 28 Sankar Aditya Tanguturi 2008-07-30 11:49:10 PDT
Created attachment 22558 [details]
This is patch contains only the changes for adding a new role i.e. DocumentRole.

As Eric,Jon,Sam and Mark suggested, I have broken down the entire patch into small patches. This is the first smallest patch. This contains only the changes that have been done to add a new role i.e DocumentRole.
Comment 29 Jon Honeycutt 2008-07-30 15:00:31 PDT
Comment on attachment 22558 [details]
This is patch contains only the changes for adding a new role i.e. DocumentRole.

Great! Couple of comments:

+        case DocumentRole:
             return false;
On Firefox, when I view the tree with AccExplorer, the document object's state includes "focusable." Can WebKit document nodes not take focus?

+        case WebCore::DocumentRole:
+            return ROLE_SYSTEM_DOCUMENT;
I think it would be better to override AccessibleBase::role() in AccessibleDocument to return ROLE_SYSTEM_DOCUMENT.
Comment 30 Sankar Aditya Tanguturi 2008-07-30 16:01:19 PDT
Created attachment 22566 [details]
Patch for changes to populate new role i.e separatorRole

This patch contains only the changes that are needed to add and populate a new role i.e separatorRole.
Comment 31 Sankar Aditya Tanguturi 2008-07-30 16:27:17 PDT
Created attachment 22568 [details]
Patch for adding and poupluating a new role i.e CellRole

This patch contains only the changes that are required to add and populate a new role i.e CellRole.

~ Thanks.
Comment 32 Sankar Aditya Tanguturi 2008-07-30 16:56:20 PDT
Created attachment 22569 [details]
Patch for changes to populate new role i.e ListItemRole

This patch contains only the changes that are required to add an populate a new role i.e ListItemRole in webkit.

~ thanks.
Comment 33 Sankar Aditya Tanguturi 2008-07-30 21:09:42 PDT
Created attachment 22573 [details]
Updated patch for DocumentRole.

As Jon suggested, Modified the code for populating the document role correctly.
Changed code to make document object focusable. Also, made changes in AccessibleDocument to override role() function of AccessibleBase.

Obsoleting the previous patch for DocumentRole.

~ Thanks.
Comment 34 Sankar Aditya Tanguturi 2008-07-31 01:17:13 PDT
Created attachment 22574 [details]
This patch contains the changes to populate "Name" attribute properly.

Enhanced the code to populate "Name" attribute properly in Webkit.
This patch contains only those changes. My comments inline in the patch.

~ Thanks.
Comment 35 Sankar Aditya Tanguturi 2008-07-31 01:57:32 PDT
Created attachment 22575 [details]
Patch for populating Role Attribute properly

WebKit doesn't properly populate the role attributes for the nodes.
Made changes to sync up WebKit behaviour with Firefox 2.0.0.14.
This patch contains only the changes required for populating role attribute.

~ Thanks.
Comment 36 Sankar Aditya Tanguturi 2008-07-31 11:07:04 PDT
I have checked in Firefox. 
<i style="display: block;">Test.</i> 

Though <i> tag is displayed in block fashion, Firefox ignores it. Firefox just
considers the text content of <i> tag. I think, webkit should follow that 
behavior as displayed in Firefox.

~ Thanks.

(In reply to comment #27)
> Testing whether an object can be ignored by checking the tag name does not seem
> to work well.
> 
> This should be present in the tree as a button:
> <i role="button">Test.</i>
> 
> This should be present as a container:
> <i style="display: block;">Test.</i>
> 
> This should be present as a list item:
> <i style="display: list-item;">Test.</i>
> 
> This can be ignored:
> <i>Test.</i>
> 
> This is how these objects are presented by Firefox 3. However, with this patch,
> the accessibility of the <i> tag is ignored in all of these cases.
> 
> Smaller issues:
> +    if(m_renderer->isListItem())
> +        return ListItemRole;
> +
> +    if(m_renderer->isHR())
> +        return SeparatorRole;
> +
> +    if(isListElement(node))
> +        return ListRole;
> +
> Need spaces between if and (.
> 
> +        case WebCore::DocumentRole:
> +          return ROLE_SYSTEM_DOCUMENT;
> +        case WebCore::CellRole:
> +          return ROLE_SYSTEM_CELL;
> +        case WebCore::ListItemRole:
> +          return ROLE_SYSTEM_LISTITEM;
> +        case WebCore::SeparatorRole:
> +          return ROLE_SYSTEM_SEPARATOR;
> Needs two more spaces of indentation before each return.
> 
> 
> As Sam and Mark said, this still needs to be broken down; the patch tries to
> fix issues with list items, tables, links, images, inline elements, text
> elements, separators, and documents, when each of these is a separate issue.
> 
> It would be best if you could submit a patch for each of these small items,
> along with a test.
> 
> I hope this doesn't put you off of working on this! :)
> 

Comment 37 Jon Honeycutt 2008-07-31 11:41:33 PDT
Comment on attachment 22573 [details]
Updated patch for DocumentRole.

This is how we usually format our ChangeLog entries:

2008-07-30  Sankar Tanguturi  <sankaraditya+bugzilla@gmail.com>

        Add an accessibility role for document objects. Part of
        https://bugs.webkit.org/show_bug.cgi?id=20013

        Reviewed by NOBODY (OOPS!).

        * page/AccessibilityObject.h:
        (WebCore::): Add DocumentRole to the AccessibilityRole enum.
        * page/AccessibilityRenderObject.cpp:
        (WebCore::AccessibilityRenderObject::canSetFocusAttribute): Return true for documents,
        matching Firefox.

r=me!
Comment 38 Jon Honeycutt 2008-07-31 11:44:40 PDT
Comment on attachment 22566 [details]
Patch for changes to populate new role i.e separatorRole

Please put the URL for the bug in the ChangeLog, https://bugs.webkit.org/show_bug.cgi?id=20013.

r=me, thanks for the patch!
Comment 39 Sankar Aditya Tanguturi 2008-07-31 13:15:44 PDT
Created attachment 22581 [details]
Updated patch for Document Role

As Jon suggested, I have updated the patch for adding DocumentRole.

~ Thanks.
Comment 40 Sankar Aditya Tanguturi 2008-07-31 13:29:04 PDT
Created attachment 22582 [details]
Update patch for Separator Role

As Jon has suggested, I have updated the patch for Separator Role with appropriate comments in the changelog.

~ Thanks.
Comment 41 Sankar Aditya Tanguturi 2008-07-31 13:32:04 PDT
Created attachment 22583 [details]
Updated patch for ListItemRole

As Jon Suggested, I have updated the patch for ListItem role. I have
added all relevant comments in the changelog.

~ Thanks.
Comment 42 Sankar Aditya Tanguturi 2008-07-31 13:37:43 PDT
Created attachment 22584 [details]
Updated patch for CellRole

As Jon Suggested, I have upadate the patch for CellRole with all my comments.

~ Thanks.
Comment 43 Sankar Aditya Tanguturi 2008-07-31 13:58:09 PDT
Created attachment 22585 [details]
Updated patch to populate Name attribute properly.

As Jon Suggested, I have update the patch for populating Name attribute, with my comments. Obsoleting the old one.

~ Thanks.
Comment 44 Sankar Aditya Tanguturi 2008-07-31 14:12:12 PDT
Created attachment 22586 [details]
Updated patch to populate Role attribute properly

As Jon suggested, I have update the patch for populating the role attribute with my comments. So, obseleting the old patch.

~ Thanks.
Comment 45 Eric Seidel (no email) 2008-07-31 15:09:28 PDT
Comment on attachment 22470 [details]
First chunk of patch

I just realized there is another problem with this patch:

The "create*Hash" functions are "wrong" because they end up causing the lookup to be against the "prefix:localName" pair.  Two elements can have matching qualified names, even if their prefixes are different.  All you care about is that their namespaceURIs and localNames match.

+static HashSet<String, CaseFoldingHash>* createTextFormattingElementNamesHash() 
+{
+    static HashSet<String, CaseFoldingHash> elementHashSet;
+    if (elementHashSet.isEmpty()) {

For example, this snippet of xhtml:

<foobar:p xmlns:foobar="http://www.w3.org/1999/xhtml" />

That is a "p" tag, and should be treated just like <p> in an HTML document or just like <xhtml:p> (if in some other document "xhtml" is bound to the xhtml namespace).

The current code would do a lookup comparing "p" against "foobar:p" and fail.  That's not what you want. :)

A better way to do this lookup would probably be to just inline the hashtable back into the isTextFormattingElement function and do the namespaceURI comparison right before the hash-lookup. Like this:

bool AccessibilityRenderObject::isTextFormattingElement() const
{
    if (!m_renderer || !m_renderer->element())
           return false;

    Element* element = m_renderer->element();
    if (element->namespaceURI() != HTMLNames::htmlNamespaceURI)
        return false;

    static HashSet<String, CaseFoldingHash> formattingElements;
    if (formattingElements.isEmpty()) {
        formattingElements.add(bTag.localName());
        formattingElements.add(bdoTag.localName());
        ...
    }

    return formattingElements.contains(element->localName());
}

I suggest inlining the hash setup back into the isTextFormattingElement function, because the hash can't really stand alone w/o the namespace lookup.
Comment 46 Sankar Aditya Tanguturi 2008-08-01 00:29:48 PDT
Created attachment 22593 [details]
Patch to expose only Relavant Accessible Objects 

This patch contains only the changes to expose only the relevant accessible
objects. Implemented few heuristics to clean MSAA tree. These heuristics have
been implemented to sync up the behaviour with Firefox 2.0.0.14.

As Jon suggested, updated the patch with all comments inline.

~ Thanks.
Comment 47 Sankar Aditya Tanguturi 2008-08-01 00:41:55 PDT
(In reply to comment #46)
Jon,
I have created this patch to expose relavant objects. I have created a new function textformattinglements. I have changed the order in heuristics are
implemented.
According to the new heuristics,
This will be present in the tree as a button:
<i role="button">Test.</i>

This will be present as a list item:
<i style="display: list-item;">Test.</i>

This will be ignored:
<i>Test.</i>

This will also be ignored. This is done to match the behavior in firefox 2.0.0.14.
<i style="display: block;">Test.</i>

~ Thanks.
Comment 48 Sankar Aditya Tanguturi 2008-08-01 08:39:50 PDT
Created attachment 22599 [details]
Patch to navigate between accessibility parent, children and siblings.

This patch contains only the changes to navigate properly between accessibility parent, accessibility children and accessibility siblings.

As Jon suggested, I have updated the patch with all my comments.

~ Thanks.
Comment 49 Eric Seidel (no email) 2008-08-02 01:26:43 PDT
Comment on attachment 22581 [details]
Updated patch for Document Role

These are all really directed @ Jon Honeycutt for review.  Any reviewer should of course feel free to review them, but I'm adding Jon as the Requestee to remove them from the main list.
Comment 50 Jon Honeycutt 2008-08-03 13:44:21 PDT
Comment on attachment 22581 [details]
Updated patch for Document Role

r=me

Thanks, Sankar!
Comment 51 Jon Honeycutt 2008-08-03 14:04:16 PDT
Comment on attachment 22582 [details]
Update patch for Separator Role

r=me! Some very minor typos:

>+        * page/AccessibilityObject.h:
>+        (WebCore::):Add Separator Role to AccessibilityRole enum.

Missing a space before Add, extra space in SeparatorRole.

>+        (WebCore::AccessibilityRenderObject::canSetFocusAttribute): return
>+        false for <hr> tags, matching Firefox.

Lower case 'r' in return.

>+        * win/AccessibleBase.cpp:
>+        (MSAARole):return ROLE_SYSTEM_SEPARATOR for Separator roles.

Missing space and lower case 'r' in return, "Separator roles" -> SeparatorRole.

Thanks for the patch!
Comment 52 Jon Honeycutt 2008-08-03 14:07:01 PDT
Comment on attachment 22583 [details]
Updated patch for ListItemRole

r=me! Some typos:

>+        (WebCore::AccessibilityRenderObject::roleValue): Return
>+        listItemRole when rendered object is a list item.

listItemRole -> ListItemRole

>+        * win/AccessibleBase.cpp:
>+        (MSAARole): Return ROLE_SYSTEM_LISTITEM for lisitem role.

lisitem role -> ListItemRole

Thanks, Sankar!
Comment 53 Jon Honeycutt 2008-08-14 00:07:51 PDT
Comment on attachment 22584 [details]
Updated patch for CellRole

+    if (m_renderer->isTableCell() || (node && node->hasTagName(thTag)))

I think TH tags become RenderTableCells, and so the check for the TH tag name isn't necessary.

r=me with that condition removed. Thanks for the patch!
Comment 54 Jon Honeycutt 2008-08-14 00:19:36 PDT
Comment on attachment 22585 [details]
Updated patch to populate Name attribute properly.

+    virtual const AccessibilityChildrenVector& children() const{ return m_children; }
Missing space between const and {.

These changes look good, but as they fix three different, unrelated object types, I think they should be split up: a patch to fix links and headings, a patch for images, and a patch for text objects.

r- for this reason, but otherwise looks good, thanks!
Comment 55 Jon Honeycutt 2008-08-14 01:08:34 PDT
Comment on attachment 22586 [details]
Updated patch to populate Role attribute properly

Looks like the changes related to children made it into your Name and Roles patches; they should be removed from one of them.

         return WebAreaRole;
+    if (m_renderer->isTable())
Needs a new line before the if.

+    if(isListElement(node))
Missing a space between if and (.

When I use AccExplorer to look at Firefox 3's tree, label, abbr, acronym, and q tags do not return ROLE_SYSTEM_GROUPING: label returns ROLE_SYSTEM_STATICTEXT, acronym returns the string "acronym," abbr returns the string "abbr," and q returns the string "q." I think you said you were testing Firefox 2, is that right? Maybe this has changed in the newer version. I think we should try to match Firefox 3.

Also, like the names patch, this patch changes the behavior of several unrelated objects. I think you should split this into separate patches for each object type: tables, lists, labels, etc.

r- for these reasons, but thanks for the patch! :)
Comment 56 Sankar Aditya Tanguturi 2008-08-15 10:41:03 PDT
Created attachment 22820 [details]
Patch to populate the role attribute for list elements.

As Jon suggested, I have obseleted the patch for populating role attribute. 
I have broken down the patch into small patches. I am uploading the patch that contains only the changes to populate ListRole for List elements.

~ Thanks.
Comment 57 Sankar Aditya Tanguturi 2008-08-15 12:08:51 PDT
Created attachment 22822 [details]
Patch to populate the name attribute for Image Elements

As Jon Suggested, I have broken down the patch to populate Name attribute. This patch contains only the changes required to populate the Name attribute for Image elements.

~ Thanks.
Comment 58 Sankar Aditya Tanguturi 2008-08-15 13:06:10 PDT
Created attachment 22825 [details]
Patch to populate name attribute for links and headings.

As jon suggested, I have broken down the patch to populate the Name attribute of the rendered elements. This patch contains only the changes to populate the Name attribute (In Inspect32) for links and headings.

~ Thanks.
Comment 59 Sankar Aditya Tanguturi 2008-08-15 14:06:45 PDT
Created attachment 22826 [details]
Patch to populate Name attribue for Text Elements

As Jon suggested, I have broken down the patch to populate the Name attribute into small chunks. As part of that, I am uploading this patch. This patch contains only the changes required to populate the Name attribute of Text elements. (In Inspect32, name attribute for text elements is not populated
properly. This patch is done to fix that issue.)
Comment 60 Eric Seidel (no email) 2008-08-21 20:18:10 PDT
Comment on attachment 22820 [details]
Patch to populate the role attribute for list elements.

Looks fine to me (except for not having a layout test), but Jon is our man here.
Comment 61 Darin Adler 2008-08-21 20:23:26 PDT
We've set things up so you should be able to make layout tests for these kinds of AX tests. The existing tests are in LayoutTests/accessibility. Our usual policy is that we require regression tests for each fix, so you should learn how to add tests!
Comment 62 Eric Seidel (no email) 2008-08-21 20:24:22 PDT
Comment on attachment 22822 [details]
Patch to populate the name attribute for Image Elements

Do we already support the title attribute?  Should we add support for such as well?

title="foo" is a valid attribute on any HTML tag, I'm not sure if it should be represented via AX or not...

In general this looks fine to me.   Passing to Jon for final approval.
Comment 63 Jon Honeycutt 2008-08-21 21:52:49 PDT
Comment on attachment 22593 [details]
Patch to expose only Relavant Accessible Objects 

Sankar, thanks for the patch!

This patch will be much easier to review if it is split up into smaller patches, and these changes will require layout tests to ensure that the behavior is correct now and will not regress in the future.

The layout tests should test, for example, that:

<i style="display: block">Test</i> is not ignored and is represented in the tree as a container element, and

<i role="button">Test</i> is not ignored and is represented in the tree as a push button.

I am not sure that testing objects by tag name is the right method for this, because I think that Firefox 3 only ignores elements based on whether they are inline.

This patch also contains your previous patch's changes to the children/addChildren methods. Those should be removed.

+    virtual bool isTextFormattingElement() const;
+    virtual bool isLinkOrListItemElement() const;
+    virtual bool isFormOrTableElement() const;
I don't think any of these methods need to be virtual methods on AccessibilityObject.

r-
Comment 64 Jon Honeycutt 2008-08-21 22:13:34 PDT
Comment on attachment 22599 [details]
Patch to navigate between accessibility parent, children and siblings.

+        Add functions to navigate properly between the accessibility parent
+        and accessibility children. 

I'm not sure that this description gets the point across. The bug is that, currently, calling previousSibling, nextSibling, etc on an accessibility object could return items that shouldn't be exposed because their accessibility is ignored, correct? This should be more explicit in the ChangeLog.

This patch contains the previous patch's changes to children/addChildren. These should be removed from this patch.

I'm not sure that this patch will work properly if objects in the DOM are moved between parents, or if objects are added or removed. This will need a layout tests that confirm that, after these sorts of DOM operations, navigating between elements gives the expected result. If you find that the behavior is not correct, you might look at how parentObjectUnignored works and follow the same method.

I think the name of the new methods should be consistent with the name of the parentObjectUnignored method, so the methods should be renamed nextSiblingUnignored, previousSiblingUnignored, and so on.

+    AccessibilityObject* parentObj = m_object->parentObjectUnignored();
+
+    if (parentObj)  {
+        *parent = static_cast<IDispatch*>(wrapper(parentObj));
+        (*parent)->AddRef();
+        return S_OK;
+    }
+
This is a separate bug that could have its own patch.

Thanks, Sankar! r- for these issues.
Comment 65 Jon Honeycutt 2008-08-21 22:19:12 PDT
Comment on attachment 22820 [details]
Patch to populate the role attribute for list elements.

The change looks right, but without the change to expose list elements from your "Patch to expose only Relavant Accessible Objects ", it can't be verified in an accessibility tool that this patch works as expected.

I think you should create a "patch to properly expose list elements" patch that exposes list elements, returns the correct role, and includes a short layout test.

Thanks! r-, but the behavior looks correct.
Comment 66 Jon Honeycutt 2008-08-21 22:32:16 PDT
Comment on attachment 22822 [details]
Patch to populate the name attribute for Image Elements

Eric is correct that we should respect the title attribute for image elements. Firefox 3 falls back to the title attribute if it doesn't find an alt attribute.

Firefox respects the title attribute for many other elements, though, so I think that should be considered a separate bug.

Firefox also ignores the alt attribute on objects that aren't native images, but I believe that's a bug.

This change seems good to me, but I think a layout test that tests that both native images and objects with role="img" have their alternative text exposed.

r=me, but please add a test.
Comment 67 Jon Honeycutt 2008-08-21 22:38:07 PDT
Comment on attachment 22825 [details]
Patch to populate name attribute for links and headings.

+        Populate Name attribute for links and Headins appropriately.
Typos: Name, Headings shouldn't be capitalized, and headings is missing a g.

This change looks fine, but it requires that your "Patch to populate Name attribue for Text Elements" be applied to test, so I think you should combine the two patches.

r- for that, and please add layout tests.

Thanks!
Comment 68 Jon Honeycutt 2008-08-21 22:38:54 PDT
Comment on attachment 22826 [details]
Patch to populate Name attribue for Text Elements

The change looks good, but it should be combined with the previous patch -- clearing review.
Comment 69 Eric Seidel (no email) 2008-08-22 00:05:24 PDT
Comment on attachment 22822 [details]
Patch to populate the name attribute for Image Elements

Is it even possible to write AX layout tests for webkit on windows yet?  My understanding is that Sankar has been doing his development mostly from windows.
Comment 70 Sankar Aditya Tanguturi 2008-08-22 00:14:19 PDT
Jon/Eric,
I have been doing the development on Windows Machine only. I have not done any
type of development on MAC. It would be great if you can provide some pointers
to any documents that specify the steps to create the Accessibility layout
tests on Windows.

~ Thanks.

(In reply to comment #69)
> (From update of attachment 22822 [details] [edit])
> Is it even possible to write AX layout tests for webkit on windows yet?  My
> understanding is that Sankar has been doing his development mostly from
> windows.
> 

Comment 71 Eric Seidel (no email) 2008-08-22 00:25:32 PDT
(In reply to comment #70)
> Jon/Eric,
> I have been doing the development on Windows Machine only. I have not done any
> type of development on MAC. It would be great if you can provide some pointers
> to any documents that specify the steps to create the Accessibility layout
> tests on Windows.
> 
> ~ Thanks.

http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/AccessibilityController.h
http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/AccessibilityController.cpp
http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/AccessibilityUIElement.h
http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp
and
http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/mac/AccessibilityControllerMac.mm
http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/mac/AccessibilityUIElementMac.mm

Show how we support AX layout tests on mac using AccessibilityController an AccessibilityUIElement.

The problem is the windows-specific bits (for tying into Windows AX apis) have not been written yet in:
http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/win
(AccessibilityControllerWin.cpp and AccessibilityUIElementWin.cpp don't exist yet)

Adding support for such is outside the scope of this bug, but until it's done, it is (as darin and Jon have pointed out), very difficult to assure the correctness of any changes to windows-specific AX support.

If you're looking for examples of Accessibility layout tests, they can be found here:
http://trac.webkit.org/browser/trunk/LayoutTests/accessibility
you can look at the .html files and the -expected.txt files to see how these AccessibilityController and AccessibilityUIElement objects are accessed via these javascript-based tests.

But again, those tests are disabled when running "run-webkit-tests" on Windows:
http://trac.webkit.org/browser/trunk/LayoutTests/platform/win/Skipped#L409

If you were to use "run-webkit-tests" on a Mac it would correctly run the accessibility tests.  Unfortunately that doesn't help you much when you're trying to make windows-specific changes to our AX support. :)  When you change common/cross-platform files, however, it is helpful to be able to run-webkit-tests on a mac to see what results have changed for the accessibility tests.
Comment 72 Jon Honeycutt 2008-08-22 14:02:30 PDT
Sankar, I believe most of your changes are cross-platform, and so you should be able to write tests that will run on the Mac (and eventually Windows).
Comment 73 Jon Honeycutt 2008-08-24 06:05:50 PDT
Sankar, I've landed some initial support for accessibility layout tests on Windows; see bug 20497. This should help with the layout tests for some of your patches.
Comment 74 Sankar Aditya Tanguturi 2008-08-26 11:07:38 PDT
Created attachment 23005 [details]
Patch to expose list elements and populate role attribute for them correctly.

As Jon suggested, I have merged the patches "populate role attribute for list elements" with "patch to expose list elements". Also, I have added as simple
atomic layout test for this patch.

In short, this patch contains only the changes that expose the list elements and
set the role attribute of list elements to "list".

~ Thanks.
Comment 75 Sankar Aditya Tanguturi 2008-08-26 14:34:59 PDT
Created attachment 23006 [details]
Patch to populate name attribute for text nodes, links and headings.

As jon has suggested, I have merged the two patches i.e "patch to populate name attribute for text elements" and "patch to populate name attribute for links and headings".

This patch contains only the required changes to populate the name attribute for text elements, links and headings. Also, this patch contains the atomic test cases for the fix.

~ Thanks.
Comment 76 Sankar Aditya Tanguturi 2008-08-26 15:27:27 PDT
Created attachment 23007 [details]
Patch to ignore anonymous block level elements

As Jon suggested, I have broken down the patch to expose relavant accessible objects. This is the first patch as part of that process.

This patch contains only the atomic changes needed to ignore anonymous block level elements.

~ Thanks.
Comment 77 Sankar Aditya Tanguturi 2008-08-26 15:33:41 PDT
Jon,
As per http://webkit.org/blog/115/webcore-rendering-ii-blocks-and-inlines/, webkit creates many intermediate anonymous blocks to satisfy few variants. As specified in that webpage, consider the following block:

<i>Italic only <b>italic and bold
<div>
Wow, a block!
</div>
<div>
Wow, another block!
</div>
More italic and bold text</b> More italic text</i>

This block will be modified into 

<anonymous pre block>
<i>Italic only <b>italic and bold</b></i>
</anonymous pre block>
<anonymous middle block>
<div>
Wow, a block!
</div>
<div>
Wow, another block!
</div>
</anonymous middle block>
<anonymous post block>
<i><b>More italic and bold text</b> More italic text</i>
</anonymous post block>

Anonymous blocks create a lot number of intermediate elements of type grouping. This doesn't look nice and also makes the tree a heavy object. I have checked the behavior for the same HTML snippet in Firefox 3. Tree looks clean and simple. To match the behaviour of Firefox3, we shouldn't expose anonymous blocks. I have made the changes for this. Can you please check this.

~ Thanks
Comment 78 Sankar Aditya Tanguturi 2008-08-26 15:47:18 PDT
Jon,
I have checked this code for dynamic webpages also. I have written a simple webpage that adds and removes a <div> element when a button is pressed. My changes are working perfectly fine. I will create the layout tests ASAP and 
upload them. 

I am referencing m_children variable in the new functions that I have created. So, I need to change the functions i.e addChildren() and children() into const functions. As a part of this, I need to change the m_children variable into a mutable object. Otherwise we will face compilation errors.

~ Thanks.

(In reply to comment #64)
> (From update of attachment 22599 [details] [edit])
> +        Add functions to navigate properly between the accessibility parent
> +        and accessibility children. 
> 
> I'm not sure that this description gets the point across. The bug is that,
> currently, calling previousSibling, nextSibling, etc on an accessibility object
> could return items that shouldn't be exposed because their accessibility is
> ignored, correct? This should be more explicit in the ChangeLog.
> 
> This patch contains the previous patch's changes to children/addChildren. These
> should be removed from this patch.
> 
> I'm not sure that this patch will work properly if objects in the DOM are moved
> between parents, or if objects are added or removed. This will need a layout
> tests that confirm that, after these sorts of DOM operations, navigating
> between elements gives the expected result. If you find that the behavior is
> not correct, you might look at how parentObjectUnignored works and follow the
> same method.
> 
> I think the name of the new methods should be consistent with the name of the
> parentObjectUnignored method, so the methods should be renamed
> nextSiblingUnignored, previousSiblingUnignored, and so on.
> 
> +    AccessibilityObject* parentObj = m_object->parentObjectUnignored();
> +
> +    if (parentObj)  {
> +        *parent = static_cast<IDispatch*>(wrapper(parentObj));
> +        (*parent)->AddRef();
> +        return S_OK;
> +    }
> +
> This is a separate bug that could have its own patch.
> 
> Thanks, Sankar! r- for these issues.
> 

Comment 79 Jon Honeycutt 2008-08-26 16:28:28 PDT
Comment on attachment 23005 [details]
Patch to expose list elements and populate role attribute for them correctly.

Sankar, great! Really happy to see this.

+++ LayoutTests/accessibility/listelements-detection-expected.txt

Because the role text is going to be different between platforms, I suggest you move the listelements-detection-expected.txt result to LayoutTests/platform/win/accessibility/. We can then put the mac-specific results in platform/mac/accessibility.

Also, accessibility tests are currently disabled on Windows. You should edit the LayoutTests/platform/win/Skipped file, remove the lines:

# Still need to implement AccessibilityController for DumpRenderTree on Windows
accessibility

and replace them with, for example:

# These tests don't run under Windows or have mac-specific results. https://bugs.webkit.org/show_bug.cgi?id=20497
accessibility/aria-describedby-on-input.html
accessibility/aria-labelledby-on-input.html
accessibility/aria-range-value.html
accessibility/aria-range.html
accessibility/aria-roles.html
accessibility/aria-slider.html
accessibility/aria-spinbutton.html
accessibility/bounds-for-range.html
accessibility/document-attributes.html
accessibility/image-map1.html
accessibility/image-map2.html
accessibility/internal-link-anchors.html
accessibility/internal-link-anchors2.html
accessibility/radio-button-checkbox-size.html
accessibility/radio-button-group-members.html
accessibility/table-attributes.html
accessibility/table-cell-spans.html
accessibility/table-cells.html
accessibility/table-detection.html
accessibility/table-sections.html
accessibility/table-with-rules.html
accessibility/textarea-insertion-point-line-number.html
accessibility/textarea-line-for-index.html

r=me with this change, and thanks!
Comment 80 Jon Honeycutt 2008-08-26 20:08:06 PDT
Comment on attachment 23006 [details]
Patch to populate name attribute for text nodes, links and headings.

+    if (isLink() || isHeading()) {

I tested this in Firefox 3:
<h1>heading <div>div</div></h1>
<a>anchor <div>div</div></a>

The anchor's title was "anchor div" which matches the new results from your patch, but the heading was nameless, so it seems that Firefox doesn't use this same method with a heading. I think the isHeading() check should be moved back where it was before this patch, and the layout tests for headings should be removed.

+            append(elementText, childrenVector[i]->title());

This doesn't match what Firefox 3 does with:

<a title="title">text</a>

In WebKit, the name would appear as "title", but in Firefox the name would be "text". I think you should use textUnderElement() instead of title().

Although this helps us to match Firefox, this patch breaks some of the current layout tests on the Mac. I'm going to ask one of our Mac engineers to look at this. It may be necessary to have different behavior for Mac and Windows.

r- for the above issues. Thanks for the patch!
Comment 81 Jon Honeycutt 2008-08-26 21:11:42 PDT
Comment on attachment 23007 [details]
Patch to ignore anonymous block level elements

+        Ignore Anonymous blocks. Don't expose them in MSAA tree. Part of

This should say, "don't expose them in the accessibility tree," since Mac uses the same tree.

This change looks good, but it needs a test. For example, you could test:

<div id="outer">A<div>B</div></div>

The "outer" element's first child should be a text node.

Thanks! r- for missing test.
Comment 82 Jon Honeycutt 2008-08-26 21:12:40 PDT
Comment on attachment 23006 [details]
Patch to populate name attribute for text nodes, links and headings.

These test results should also be in platform/win/accessibility.
Comment 83 Jon Honeycutt 2008-08-28 15:45:21 PDT
Comment on attachment 23006 [details]
Patch to populate name attribute for text nodes, links and headings.

Sankar, I verified with an engineer for accessibility on the Mac that RenderText objects should not return textUnderElement() for their name attribute on the Mac, so it would be necessary to have different behavior between the platforms.
Comment 84 Sankar Aditya Tanguturi 2008-09-12 14:41:10 PDT
Jon,
I have created few tests for this bug. When I was testing the run-webkit-tests, i have got the following erro:

<error>
$ WebKit/WebKitTools/Scripts/run-webkit-tests accessibility
  10752 [main] perl 1360 C:\cygwin\bin\perl.exe: *** fatal error - unable to rem
ap C:\cygwin\lib\perl5\5.10\i686-cygwin\auto\File\Glob\Glob.dll to same address
as parent(0x1260000) != 0x1CD0000
   5373 [main] perl 9136 fork: child 1360 - died waiting for dll loading, errno
11
5226630 [main] perl 9204 C:\cygwin\bin\perl.exe: *** fatal error - unable to rem
ap C:\cygwin\lib\perl5\5.10\i686-cygwin\auto\File\Glob\Glob.dll to same address
as parent(0x1260000) != 0x1CD0000
5246988 [main] perl 9136 fork: child 9204 - died waiting for dll loading, errno
11

</error>

I browser the bugzilla for the solution. But, I couldn't find one. Can you throw some light on resolving this issue.

~ Thanks.
(In reply to comment #83)
> (From update of attachment 23006 [details] [edit])
> Sankar, I verified with an engineer for accessibility on the Mac that
> RenderText objects should not return textUnderElement() for their name
> attribute on the Mac, so it would be necessary to have different behavior
> between the platforms.
> 
Comment 85 Jon Honeycutt 2008-09-15 19:38:26 PDT
(In reply to comment #84)

Hi, Sankar! Are you running Windows Vista? If so, look at the last part of step 3 on http://webkit.org/building/tools.html . If you aren't, I'm not sure what would cause that error.
Comment 86 Darin Adler 2008-10-12 16:54:34 PDT
I tried to land one of these patches and svn-apply couldn't apply it successfully. We need to do something about to get the many review+ patches attached to this bug landed. And also, why are we planning to land all these with no regression tests?
Comment 87 Mark Rowe (bdash) 2008-12-02 16:53:56 PST
Jon, what's the plan for these?  We can't leave them sitting in the commit queue indefinitely.  If they can't be landed as-is, they shouldn't be marked as r+.
Comment 88 Mark Rowe (bdash) 2009-01-12 23:38:39 PST
Jon, please do something to get these out of the commit queue.  If they're not ready to be committed they should have the review flag cleared.
Comment 89 Sankar Aditya Tanguturi 2009-01-27 19:40:52 PST
Discussed with Jon. Closing this bug. Decided to log separate bugs to
track each issue.

Thanks,
Sankar.
(In reply to comment #88)
> Jon, please do something to get these out of the commit queue.  If they're not
> ready to be committed they should have the review flag cleared.
>