Bug 129814

Summary:

Regression: AX: Don't expose role or notifications for invalid menus

Product:

WebKit

Reporter:

James Craig <jcraig>

Component:

Accessibility

Assignee:

chris fleizach <cfleizach>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, jdiggs, mario, samuel_white, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
patch	mario: review+

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