[Webkit-unassigned] [Bug 20013] Windows AX huerusitics are poor

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20013





------- Comment #12 from jhoneycutt at apple.com  2008-07-23 21:15 PDT -------
Hi, Sankar. Looking good, a few comments:

+2008-07-21  U-stanguturi\Sankar Tanguturi  sankaraditya at 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!


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.