Bug 40133

Summary: AX: need ListItemRole and PresentationalRole
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, dglazkov, eric, mrobinson, webkit.review.bot, wsiegrist, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 40132    
Attachments:
Description Flags
Patch
none
patch
none
patch
none
patch
darin: review-
patch darin: review+

Description chris fleizach 2010-06-03 11:24:55 PDT
We need both these roles to be exposed directly to help solve https://bugs.webkit.org/show_bug.cgi?id=40132
Comment 1 chris fleizach 2010-06-03 11:30:01 PDT
no functional changes. just prep work
Comment 2 chris fleizach 2010-06-03 11:46:03 PDT
Created attachment 57796 [details]
Patch
Comment 3 WebKit Review Bot 2010-06-03 20:31:24 PDT
Attachment 57796 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3102006
Comment 4 chris fleizach 2010-06-07 14:22:57 PDT
Created attachment 58083 [details]
patch

this should hopefully work for chomium too
Comment 5 chris fleizach 2010-06-07 14:26:20 PDT
Created attachment 58084 [details]
patch

now with correct changelogs
Comment 6 chris fleizach 2010-06-07 14:30:17 PDT
Created attachment 58086 [details]
patch

for some reason the last patch didn't take
Comment 7 chris fleizach 2010-06-07 14:38:09 PDT
weird only details is showing up
Comment 8 Darin Adler 2010-06-12 19:12:28 PDT
Comment on attachment 58086 [details]
patch

> +Node* AccessibilityRenderObject::node() const
> +{
> +    if (!m_renderer) 
> +        return 0;
> +    return m_renderer->node();
> +}

What is this function for? It's added in the patch and not used.

The patch otherwise seems great. review- because of the seemingly-unrelated change.
Comment 9 chris fleizach 2010-06-14 09:29:01 PDT
(In reply to comment #8)
> (From update of attachment 58086 [details])
> > +Node* AccessibilityRenderObject::node() const
> > +{
> > +    if (!m_renderer) 
> > +        return 0;
> > +    return m_renderer->node();
> > +}
> 
> What is this function for? It's added in the patch and not used.
> 
> The patch otherwise seems great. review- because of the seemingly-unrelated change.

That was part of the follow up patch. Forgot to take it out of this one
Comment 10 chris fleizach 2010-06-14 09:35:16 PDT
Created attachment 58664 [details]
patch

Patch is same as before, but removes the not necessary node() function from this patch
Comment 11 Darin Adler 2010-06-14 13:54:11 PDT
Comment on attachment 58664 [details]
patch

> +        (WebCore::):

Lines like this should be deleted from the ChangeLog. And we should fix prepare-ChangeLog to not generate them.

> +        (RoleEntry::):

Ditto.
Comment 12 chris fleizach 2010-06-14 14:48:51 PDT
http://trac.webkit.org/changeset/61151
Comment 13 Eric Seidel (no email) 2010-06-14 20:15:41 PDT
Seems to have caused a Gtk API test regression:

ERROR:../../WebKit/gtk/tests/testatkroles.c:98:get_child_and_test_role: assertion failed: (child_role == role)
Comment 14 Eric Seidel (no email) 2010-06-14 20:16:14 PDT
Sheriffbot didn't notice because the buildbot seems to have not been notified of this revision.  Not sure why or how.
Comment 15 chris fleizach 2010-06-14 22:53:36 PDT
(In reply to comment #14)
> Sheriffbot didn't notice because the buildbot seems to have not been notified of this revision.  Not sure why or how.

hopefully i can fix this tonite
Comment 16 chris fleizach 2010-06-14 23:51:06 PDT
looks like
http://trac.webkit.org/changeset/61176 fixed it