Bug 98358 - [GTK] accessibility/aria-scrollbar-role.html is failing
Summary: [GTK] accessibility/aria-scrollbar-role.html is failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, LayoutTestFailure
Depends on:
Blocks: 98347 111989
  Show dependency treegraph
 
Reported: 2012-10-04 00:35 PDT by Zan Dobersek
Modified: 2013-03-20 01:14 PDT (History)
10 users (show)

See Also:


Attachments
Patch (8.00 KB, patch)
2013-03-18 08:03 PDT, Krzysztof Czech
mrobinson: review-
Details | Formatted Diff | Diff
Patch (7.91 KB, patch)
2013-03-19 03:56 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2012-10-04 00:35:15 PDT
accessibility/aria-scrollbar-role.html is failing on all GTK platforms.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=accessibility%2Faria-scrollbar-role.html
Comment 1 Krzysztof Czech 2013-03-18 08:03:04 PDT
Created attachment 193565 [details]
Patch
Comment 2 Martin Robinson 2013-03-18 08:08:02 PDT
Comment on attachment 193565 [details]
Patch

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

Some unnecessary string duplication going on, but otherwise looks good to me. :)

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:514
> +    GOwnPtr<gchar> axOrientation;
> +    if (checkElementState(m_element, ATK_STATE_HORIZONTAL))
> +        axOrientation.set(g_strdup_printf("AXOrientation: AXHorizontalOrientation"));
> +    else if (checkElementState(m_element, ATK_STATE_VERTICAL))
> +        axOrientation.set(g_strdup_printf("AXOrientation: AXVerticalOrientation"));
> +
> +    if (!axOrientation)
> +        return JSStringCreateWithCharacters(0, 0);
> +
> +    return JSStringCreateWithUTF8CString(axOrientation.get());

See below.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:552
> +    GOwnPtr<gchar> axOrientation;
> +    if (checkElementState(m_element, ATK_STATE_HORIZONTAL))
> +        axOrientation.set(g_strdup_printf("AXOrientation: AXHorizontalOrientation"));
> +    else if (checkElementState(m_element, ATK_STATE_VERTICAL))
> +        axOrientation.set(g_strdup_printf("AXOrientation: AXVerticalOrientation"));
> +

g_strdump_printf can just be g_strdump and even more simply can just be like this:

const char* axOrientation = 0;
if (checkElementState(m_element, ATK_STATE_HORIZONTAL))
     axOrientation = "AXOrientation: AXHorizontalOrientation";
...
Comment 3 Martin Robinson 2013-03-18 08:08:40 PDT
Would love to have Joanie or Mario double-check this one.
Comment 4 Mario Sanchez Prada 2013-03-19 03:18:32 PDT
(In reply to comment #3)
> Would love to have Joanie or Mario double-check this one.

I agree with martin comments and suggestions. Apart from that, the patch looks good to me, a11y-wise.
Comment 5 Krzysztof Czech 2013-03-19 03:56:37 PDT
Created attachment 193782 [details]
Patch
Comment 6 Krzysztof Czech 2013-03-19 04:00:34 PDT

(In reply to comment #4)
> (In reply to comment #3)
> > Would love to have Joanie or Mario double-check this one.
> 
> I agree with martin comments and suggestions. Apart from that, the patch looks good to me, a11y-wise.

Thanks.
Applied Martin's suggestions
Comment 7 WebKit Review Bot 2013-03-20 01:14:37 PDT
Comment on attachment 193782 [details]
Patch

Clearing flags on attachment: 193782

Committed r146312: <http://trac.webkit.org/changeset/146312>
Comment 8 WebKit Review Bot 2013-03-20 01:14:42 PDT
All reviewed patches have been landed.  Closing bug.