Bug 40132 - AX: presentational role needs to be inherited by required elements
Summary: AX: presentational role needs to be inherited by required elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on: 40133
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-03 11:24 PDT by chris fleizach
Modified: 2010-06-18 13:18 PDT (History)
5 users (show)

See Also:


Attachments
patch (10.36 KB, patch)
2010-06-14 15:16 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (10.32 KB, patch)
2010-06-14 15:22 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (10.80 KB, patch)
2010-06-16 10:33 PDT, chris fleizach
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2010-06-03 11:24:02 PDT
The current implementation of 'presentation' is correct (no work lost) but incomplete in that it did not support presentation inheritance. The previous draft was somewhat vague in that area, but the description of the presentation role (mainly the part about presentation inheritance) has been completely rewritten as of today. 

http://www.w3.org/WAI/PF/aria/complete#presentation

Changes to WebKit will include inheritance of the presentation role to structural descendants and labeling element unless those elements have an explicit role defined. Also, presentation role is to be ignored if the element is focusable. See referenced portion of spec for details.
Comment 1 chris fleizach 2010-06-14 15:16:32 PDT
Created attachment 58709 [details]
patch
Comment 2 chris fleizach 2010-06-14 15:17:28 PDT
this makes sure that WebKit behaves correctly by applying the presentation role status to required elements of an element that is presentational

translation: if <ul> is presentation, <li> will be too
Comment 3 WebKit Review Bot 2010-06-14 15:18:45 PDT
Attachment 58709 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/accessibility/AccessibilityRenderObject.cpp:3071:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 chris fleizach 2010-06-14 15:22:51 PDT
Created attachment 58710 [details]
patch
Comment 5 David Kilzer (:ddkilzer) 2010-06-15 13:00:44 PDT
Comment on attachment 58710 [details]
patch

> 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.
Comment 6 David Kilzer (:ddkilzer) 2010-06-15 13:08:14 PDT
(In reply to comment #5)
> (From update of attachment 58710 [details])
> > +    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;

Ignore my above comments.  The code at the bottom of QualifiedName.h handles the hashing for you.  It's interesting that it doesn't appear to be used elsewhere in WebCore, though.
Comment 7 chris fleizach 2010-06-16 10:33:01 PDT
Created attachment 58905 [details]
Patch
Comment 8 chris fleizach 2010-06-16 10:33:31 PDT
addressed all of david's comments in this latest patch
Comment 9 David Kilzer (:ddkilzer) 2010-06-17 19:24:47 PDT
Comment on attachment 58905 [details]
Patch

WebCore/accessibility/AccessibilityRenderObject.cpp:3059
 +      DEFINE_STATIC_LOCAL(HashSet<QualifiedName>, listItemParents, ());
This static local should be defined within the scope of the switch statement below so it's only created when it's needed.

WebCore/accessibility/AccessibilityRenderObject.cpp:3065
 +      case ListMarkerRole:
The static local variable should be defined here.

r=me
Comment 10 chris fleizach 2010-06-18 10:59:42 PDT
http://trac.webkit.org/changeset/61421
Comment 11 chris fleizach 2010-06-18 11:14:32 PDT
doesn't work on windows

c:\cygwin\home\buildbot\slave\win-release\build\WebCore\accessibility\AccessibilityRenderObject.cpp(3072) : error C2361: initialization of 'listItemParents' is skipped by 'default' label
        c:\cygwin\home\buildbot\slave\win-release\build\WebCore\accessibility\AccessibilityRenderObject.cpp(3064) : see declaration of 'listItemParents'
Comment 12 WebKit Review Bot 2010-06-18 11:15:29 PDT
http://trac.webkit.org/changeset/61421 might have broken Chromium Win Release
Comment 13 chris fleizach 2010-06-18 12:38:07 PDT
tiger and leopard are equipped to deal with lists. will ignore those tests
Comment 14 chris fleizach 2010-06-18 12:46:10 PDT
http://trac.webkit.org/changeset/61427
to skip the tests on tiger and leopard since lists are not supported there
Comment 15 WebKit Review Bot 2010-06-18 13:10:12 PDT
http://trac.webkit.org/changeset/61427 might have broken SnowLeopard Intel Release (Tests)
Comment 16 chris fleizach 2010-06-18 13:18:06 PDT
(In reply to comment #15)
> http://trac.webkit.org/changeset/61427 might have broken SnowLeopard Intel Release (Tests)

it did not break that bot