Bug 129814 - Regression: AX: Don't expose role or notifications for invalid menus
Summary: Regression: AX: Don't expose role or notifications for invalid menus
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-06 12:27 PST by James Craig
Modified: 2014-03-24 14:27 PDT (History)
9 users (show)

See Also:


Attachments
patch (11.60 KB, patch)
2014-03-18 20:58 PDT, chris fleizach
mario: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2014-03-06 12:27:01 PST
AX: Don't expose role or notifications for invalid menus

For example, on this page:
http://www.usmagazine.com/celebrity-news/news/andrew-garfield-refused-to-present-with-batkid-at-oscars-report-201463

Seems like this could be solved by having WebKit not fire the notification or example the menu role if the menu is invalid. It might be wise to not even expose it as an AXMenu if it's invalid. For example, this menu contains a link child (invalid) and no menu items (invalid).

<li class="" role="menu">
  <a class="dropdown-toggle" href="#" data-action="flag" data-role="flag" title="Flag as inappropriate">
    <i aria-hidden="true" class="icon icon-flag"></i>
  </a>
</li>

This appears to be fallout from bug 123133.
Comment 1 James Craig 2014-03-06 12:27:09 PST
<rdar://problem/16251311>
Comment 2 James Craig 2014-03-06 12:27:56 PST
s/or example the menu role/or expose the menu role/
Comment 3 chris fleizach 2014-03-18 20:58:38 PDT
Created attachment 227154 [details]
patch
Comment 4 Mario Sanchez Prada 2014-03-21 11:31:28 PDT
Comment on attachment 227154 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227154&action=review

Lgtm. Please consider those suggestions below before landing

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:121
> +    m_object->updateChildrenIfNecessary();

For your consideration before landing: I wonder if you could fold this into updateBackingStore(), so you don't need to call it explicitly here, and other platforms get benefit from that too

> LayoutTests/ChangeLog:13
> +        * platform/mac/accessibility/table-with-aria-role-expected.txt:

You can move this test expectation now to LayoutTests/accessibility, as there should produce the same (platform independent) output now for all the platforms
Comment 5 chris fleizach 2014-03-24 12:24:51 PDT
(In reply to comment #4)
> (From update of attachment 227154 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227154&action=review
> 
> Lgtm. Please consider those suggestions below before landing
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:121
> > +    m_object->updateChildrenIfNecessary();
> 
> For your consideration before landing: I wonder if you could fold this into updateBackingStore(), so you don't need to call it explicitly here, and other platforms get benefit from that too
> 
> > LayoutTests/ChangeLog:13
> > +        * platform/mac/accessibility/table-with-aria-role-expected.txt:
> 
> You can move this test expectation now to LayoutTests/accessibility, as there should produce the same (platform independent) output now for all the platforms

Will do. Thanks!
Comment 6 chris fleizach 2014-03-24 14:27:09 PDT
http://trac.webkit.org/changeset/166194