[webkit-reviews] review denied: [Bug 40132] AX: presentational role needs to be inherited by required elements : [Attachment 58710] patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 40132: AX: presentational role needs to be inherited by required elements
https://bugs.webkit.org/show_bug.cgi?id=40132

Attachment 58710: patch
https://bugs.webkit.org/attachment.cgi?id=58710&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> Index: WebCore/accessibility/AccessibilityRenderObject.cpp
> +Node* AccessibilityRenderObject::node() const
> +{
> +    if (!m_renderer) 
> +	   return 0;
> +    return m_renderer->node();
> +}

You may want to consider a follow-up patch for AccessibilityRenderObject.cpp to
switch to using this new node() method, and moving the method to
AccessibilityRenderObject.h to make it inline.

>  static inline bool isInlineWithContinuation(RenderObject* renderer)
>  {
> @@ -1738,6 +1745,9 @@
>      if (roleValue() == PresentationalRole)
>	   return true;
>      
> +    if (roleValue() == PresentationalRole || inheritsPresentationalRole())
> +	   return true;
> +    

You probably want to remove the previous if statement here.

> @@ -3051,6 +3061,45 @@
>      return AccessibilityObject::orientation();
>  }
>      
> +bool AccessibilityRenderObject::inheritsPresentationalRole() const
> +{
> +    // ARIA spec says that when a parent object is presentational, and it
has required child elements,
> +    // those child elements are also presentational. For example, <li>
becomes presentational from <ul>.

Might be nice to provide a link to the spec here if possible.

> +    bool checkForPresentationalParent = false;
> +    HashSet<QualifiedName> possibleParentTagNames;

Are you sure this HashSet works as intended?  Looking at QualifiedName.cpp, I
see it defines:

typedef HashSet<QualifiedName::QualifiedNameImpl*, QualifiedNameHash> QNameSet;


And NodeRareData.h defines:

typedef HashMap<RefPtr<QualifiedName::QualifiedNameImpl>, TagNodeList*>
TagNodeListCache;

Looking at const QualifiedName& operator=(const QualifiedName&) in
QualifiedName.h leads me to believe that two different QualifiedName objects
can point to the same QualiifedNameImpl, in which case you're not really
getting a set.	I suspect you want either the QNameSet from QualifiedName.cpp
(to avoid the RefPtr<> overhead) or something like this:

HashSet<RefPtr<QualifiedName::QualifiedNameImpl> > possibleParentTagNames;

> +    switch (roleValue()) {
> +    case ListItemRole:
> +    case ListMarkerRole:
> +	   checkForPresentationalParent = true;
> +	   possibleParentTagNames.add(ulTag);
> +	   possibleParentTagNames.add(olTag);
> +	   possibleParentTagNames.add(dlTag);
> +	   break;
> +    default:
> +	   break;
> +    }

Instead of dynamically creating possibleParentTagNames each time this method is
called, it might be nice to define possibleParentTagNames as a pointer
(HashSet<RefPtr<QualifiedName::QualifiedNameImpl> > *possibleParentTagNames),
then use DEFINE_STATIC_LOCAL() in the ListItemRole/ListMarkerRole case
statement to create it and assign it to possibleParentTagNames when needed. 
You would need some additional NULL checks of possibleParentTagNames below if
you did this.

This would also shortcut some hash lookups in the for loop below if the hash is
empty.

> +    // Not all elements need to check for this.
> +    if (!checkForPresentationalParent)
> +	   return false;
> +    
> +    for (AccessibilityObject* parent = parentObject(); parent; parent =
parent->parentObject()) { 
> +	   if (!parent->isAccessibilityRenderObject())
> +	       continue;
> +	   
> +	   Node* elementNode =
static_cast<AccessibilityRenderObject*>(parent)->node();
> +	   if (!elementNode || !elementNode->isElementNode())
> +	       continue;
> +	   
> +	   // If native tag of the parent element matches an acceptable name,
then return
> +	   // based on its presentational status.
> +	   if
(possibleParentTagNames.contains(static_cast<Element*>(elementNode)->tagQName()
))
> +	       return parent->roleValue() == PresentationalRole;
> +    }
> +    
> +    return false;
> +}

r- to remove the redundant if statement and to fix the HashSet<QualifiedName>
issue.	The rest is optional.