Bug 106924 – [GTK] GTK does not expose heading level correctly. Was: accessibility/heading-level.html is failing

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

The accessibility/heading-level.html test is failing since it's been added in r139534.
http://trac.webkit.org/changeset/139534

--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/accessibility/heading-level-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/accessibility/heading-level-actual.txt
@@ -3,24 +3,24 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS: level is 1.
-PASS: level is 2.
-PASS: level is 3.
-PASS: level is 4.
-PASS: level is 5.
-PASS: level is 6.
-PASS: level is 1.
-PASS: level is 2.
-PASS: level is 3.
-PASS: level is 4.
-PASS: level is 5.
-PASS: level is 6.
-PASS: level is 1.
-PASS: level is 2.
-PASS: level is 3.
-PASS: level is 4.
-PASS: level is 5.
-PASS: level is 6.
+FAIL: level is 0 for <h1 class="ex" data-expected="1" id="ex0">X</h1>, expected 1.
+FAIL: level is 0 for <h2 class="ex" data-expected="2" id="ex1">X</h2>, expected 2.
+FAIL: level is 0 for <h3 class="ex" data-expected="3" id="ex2">X</h3>, expected 3.
+FAIL: level is 0 for <h4 class="ex" data-expected="4" id="ex3">X</h4>, expected 4.
+FAIL: level is 0 for <h5 class="ex" data-expected="5" id="ex4">X</h5>, expected 5.
+FAIL: level is 0 for <h6 class="ex" data-expected="6" id="ex5">X</h6>, expected 6.
+FAIL: level is 0 for <h6 class="ex" role="heading" data-expected="1" aria-level="1" id="ex6">X</h6>, expected 1.
+FAIL: level is 0 for <h5 class="ex" role="heading" data-expected="2" aria-level="2" id="ex7">X</h5>, expected 2.
+FAIL: level is 0 for <h4 class="ex" role="heading" data-expected="3" aria-level="3" id="ex8">X</h4>, expected 3.
+FAIL: level is 0 for <h3 class="ex" role="heading" data-expected="4" aria-level="4" id="ex9">X</h3>, expected 4.
+FAIL: level is 0 for <h2 class="ex" role="heading" data-expected="5" aria-level="5" id="ex10">X</h2>, expected 5.
+FAIL: level is 0 for <h1 class="ex" role="heading" data-expected="6" aria-level="6" id="ex11">X</h1>, expected 6.
+FAIL: level is 0 for <div class="ex" role="heading" data-expected="1" aria-level="1" id="ex12">X</div>, expected 1.
+FAIL: level is 0 for <div class="ex" role="heading" data-expected="2" aria-level="2" id="ex13">X</div>, expected 2.
+FAIL: level is 0 for <div class="ex" role="heading" data-expected="3" aria-level="3" id="ex14">X</div>, expected 3.
+FAIL: level is 0 for <div class="ex" role="heading" data-expected="4" aria-level="4" id="ex15">X</div>, expected 4.
+FAIL: level is 0 for <div class="ex" role="heading" data-expected="5" aria-level="5" id="ex16">X</div>, expected 5.
+FAIL: level is 0 for <div class="ex" role="heading" data-expected="6" aria-level="6" id="ex17">X</div>, expected 6.
 PASS successfullyParsed is true
 
 TEST COMPLETE

[Lukasz Gajowy]

Generally proposed solution in atk/AccessibilityUIElementAtk.cpp  (AccessibilityUIElement::intValue) may not be accurate for this test.

Couple of modifications require to pass it:

1) by adding aria-valuenow="x" attribute in every heading, where x is equal to data-expected.
2) by updating an interfaceMask on ATK_VALUE interface where element's role is HeadingRole
3) by making isARIARange() method from AccessibilityNodeObject.cpp return true in case of meeting HeadingRole or UnknownRole.

This solution raises the following questions:
- Can we consider Heading element as a Range.
- It seems calling valueForRange is not appropriate for this test. Heading is not a Range type element.

[James Craig]

> 1) by adding aria-valuenow="x" attribute in every heading, where x is equal to data-expected.

Don't do that. It's expected that the level is determined by the tagname (or nesting) in HTML, and by the aria-level property in ARIA. Adding valuenow would be an invalid ARIA test case, because it's only allowed on range widgets, and heading inherits from structure, not widget.

http://www.w3.org/WAI/PF/aria/rdf_model.svg

> This solution raises the following questions:
> - Can we consider Heading element as a Range.

I woud say no.

> - It seems calling valueForRange is not appropriate for this test. Heading is not a Range type element.

The test just calls the intValue property, so I don't think there is a problem with the test. Value for a heading is it's level. That said, it seems like a logical suggestion that intValue should not call valueForRange(). Perhaps rename the accessor method value(), or use a different method in this case.

[James Craig]

Updating the title to be more accurate.

From: "accessibility/heading-level.html is failing" 
To: "GTK does not expose heading level correctly."

[Mario Sanchez Prada]

*** Bug 121556 has been marked as a duplicate of this bug. ***

[Mario Sanchez Prada]

(In reply to comment #3)
> Updating the title to be more accurate.
> 
> From: "accessibility/heading-level.html is failing" 
> To: "GTK does not expose heading level correctly."

Actually, GTK is exposing the heading level already as a "level" object attribute for the AtkObject. The only problem is that DRT/WKTR are not reading that value when we call to intValue(), which at the moment only works for accessible objects implementing the AtkValue interface.

However, it seems clear to me (and I hope Joanie will agree), that headings should not implement the AtkValue interface, since they don't match the use case for that (e.g. what would be the 'minimum increment', 'maximum value' and 'minimum value'?), so perhaps the best option is just to tweak our DRT/WKTR to do something similar to what chromium decided to do back in the day in SVN r139658: to consider headings as special cases in the intValue() function, so we check the "level" object property for the heading objects.

[Mario Sanchez Prada]

Created attachment 212140 [details]
Patch proposal

This patch would implement that idea and, ideally, will make the test pass.

However, adding those mappings in DRT/WKTR unveiled another issue that does not quite allow us to get it right yet. This is the output I'm getting now, with this patch applied on top of another patch for bug 121602 and bug 121558 (as the test will crash now without that):

    This tests that headings have a level matching the implicit default value or explicitly defined aria-level value.
    
    On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
    
    
    PASS: level is 1.
    FAIL: level is 0 for <h2 class="ex" data-expected="2" id="ex1">X</h2>, expected 2.
    FAIL: level is 0 for <h3 class="ex" data-expected="3" id="ex2">X</h3>, expected 3.
    FAIL: level is 0 for <h4 class="ex" data-expected="4" id="ex3">X</h4>, expected 4.
    FAIL: level is 0 for <h5 class="ex" data-expected="5" id="ex4">X</h5>, expected 5.
    FAIL: level is 0 for <h6 class="ex" data-expected="6" id="ex5">X</h6>, expected 6.
    FAIL: level is 0 for <h6 class="ex" data-expected="1" aria-level="1" id="ex6">X</h6>, expected 1.
    FAIL: level is 0 for <h5 class="ex" data-expected="2" aria-level="2" id="ex7">X</h5>, expected 2.
    FAIL: level is 0 for <h4 class="ex" data-expected="3" aria-level="3" id="ex8">X</h4>, expected 3.
    FAIL: level is 0 for <h3 class="ex" data-expected="4" aria-level="4" id="ex9">X</h3>, expected 4.
    FAIL: level is 0 for <h2 class="ex" data-expected="5" aria-level="5" id="ex10">X</h2>, expected 5.
    FAIL: level is 0 for <h1 class="ex" data-expected="6" aria-level="6" id="ex11">X</h1>, expected 6.
    FAIL: level is 0 for <h6 class="ex" role="heading" data-expected="1" aria-level="1" id="ex12">X</h6>, expected 1.
    FAIL: level is 0 for <h5 class="ex" role="heading" data-expected="2" aria-level="2" id="ex13">X</h5>, expected 2.
    FAIL: level is 0 for <h4 class="ex" role="heading" data-expected="3" aria-level="3" id="ex14">X</h4>, expected 3.
    FAIL: level is 0 for <h3 class="ex" role="heading" data-expected="4" aria-level="4" id="ex15">X</h3>, expected 4.
    FAIL: level is 0 for <h2 class="ex" role="heading" data-expected="5" aria-level="5" id="ex16">X</h2>, expected 5.
    FAIL: level is 0 for <h1 class="ex" role="heading" data-expected="6" aria-level="6" id="ex17">X</h1>, expected 6.
    FAIL: level is 0 for <div class="ex" role="heading" data-expected="1" aria-level="1" id="ex18">X</div>, expected 1.
    FAIL: level is 0 for <div class="ex" role="heading" data-expected="2" aria-level="2" id="ex19">X</div>, expected 2.
    FAIL: level is 0 for <div class="ex" role="heading" data-expected="3" aria-level="3" id="ex20">X</div>, expected 3.
    FAIL: level is 0 for <div class="ex" role="heading" data-expected="4" aria-level="4" id="ex21">X</div>, expected 4.
    FAIL: level is 0 for <div class="ex" role="heading" data-expected="5" aria-level="5" id="ex22">X</div>, expected 5.
    FAIL: level is 0 for <div class="ex" role="heading" data-expected="6" aria-level="6" id="ex23">X</div>, expected 6.
    PASS successfullyParsed is true
    
    TEST COMPLETE
    
    #EOF


The interesting part of this is that the proper value will be returned when calling intValue() *as long as* we do not hide the heading at the end of each iteration in the for loop for the test:

    <script>
    [...]
        var examples = document.querySelectorAll('.ex');
        for (var i = 0, c = examples.length; i < c; i++) {
            var el = examples[i];
            el.id = 'ex' + i;
            var axElement = accessibilityController.accessibleElementById(el.id);
            var result = document.getElementById('console');

            // Test AXLevel.
            if (axElement.intValue == parseInt(el.getAttribute('data-expected'))) {
                result.innerText += "PASS: level is " + axElement.intValue + ".\n";
            } else {
                result.innerText += "FAIL: level is " + axElement.intValue + " for " + el.outerHTML + ", expected " + parseInt(el.getAttribute('data-expected')) + ".\n";
            }
            el.style.display = 'none'; // Hide each example after verification.
        }
    }
    </script>


If we remove the "el.style.display = 'none';" line we will get the proper values on each call to intValue() as expected, but leaving it there will show the weird behaviour we could see above. After some investigation, I found that the problem seems to be that such an stament forces to update the layout, ultimately causing (at least on GTK/EFL) to invalidate the AtkObjects we had wrapped inside the AccessibilityUIElement objects, hence early returning for every object after the first one in intValue(), due to the "if (!m_element || !ATK_IS_OBJECT(m_element)) return 0.0f;" statement.

So, it seems to me like there's yet another weird bug happening that we need to get fixed before being able to actually fix this patch, even if the best solution is just the proposed one of considering heading roles in the implementation of DRT/WKTR's intValue() functions. I'll try to work on that asap and will file a bug blocking this one when ready when I know better what's going on.

[Joanmarie Diggs]

(In reply to comment #5)
 
> However, it seems clear to me (and I hope Joanie will agree), that headings should not implement the AtkValue interface

I do agree. Most enthusiastically. :)

[Mario Sanchez Prada]

(In reply to comment #6)
> [...]
> So, it seems to me like there's yet another weird bug happening that we 
> need to get fixed before being able to actually fix this patch, even if
> the best solution is just the proposed one of considering heading roles
> in the implementation of DRT/WKTR's intValue() functions. I'll try to
> work on that asap and will file a bug blocking this one when ready when
> I know better what's going on.

It seems that the problem was caused by a call to coreObject->helpText() in webkitAccessibleGetAttributes, in the ATK wrapper, which was causing a text computation under the element just for the sake of exposing the 'aria-help' attribute, which ended up altering the a11y tree and getting this weird result.

Fortunately, we can just remove that 'aria-help' mapping from there since it's wrong in the first place, and once we do that this tests will pass gracefully, assuming that the patch attached here is approved.

For the removal of the 'aria-help' mapping I filed bug 121675 and provided a patch there, so now setting the dependency here and moving on.

[Mario Sanchez Prada]

Created attachment 212466 [details]
Patch proposal

(In reply to comment #8)
> [...]
> It seems that the problem was caused by a call to coreObject->helpText() in 
> webkitAccessibleGetAttributes, in the ATK wrapper, which was causing a text
> computation under the element just for the sake of exposing the 'aria-help'
> attribute, which ended up altering the a11y tree and getting this weird
> result.
> 
> Fortunately, we can just remove that 'aria-help' mapping from there since
> it's wrong in the first place, and once we do that this tests will pass
> gracefully, assuming that the patch attached here is approved.
> 
> For the removal of the 'aria-help' mapping I filed bug 121675 and provided
> a patch there, so now setting the dependency here and moving on.

It seems that this accessibility/heading-level.html test is not crashing anymore but just failing, since removing that call to coreObject->helpText() from there do not cause the segfault to happen anymore in the context of this test. So, I'm now proposing a new patch that is basically the previous one + the removal of the test from TestExpectations file.

However, a problem related to trying to use the WebCore's accessibility API when the render tree is not stable (needing a layout update) still remains, since the layout test proposed with the current patch for bug 121558 still crashes in GTK. But that's a separate issue anyway (even if it was detected through this bug in the first place), so I'd rather keep it separate and work on it there instead.

So, please review this patch for this specific thing. I'll keep working in that other potential crash in bug 121558. Thanks!

[chris fleizach]

Comment on attachment 212466 [details]
Patch proposal

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

r- for some style considerations. thanks

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:618
> +        GValue value = { 0, { { 0 } } };

can you use G_VALUE_INIT here?

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:621
> +            return 0.0f;

style guidelines say not to append "f" to literals

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:632
> +        if (!ok)

this should be if (ok)... then you can let the failure case fall through to the final return value

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:633
> +            return 0.0f;

ditto

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:636
> +    return 0.0f;

ditto

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:789
> +            return 0.0f;

ditto about .f

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:799
> +        double headingLevelValue = headingLevel.toDouble(&ok);

ditto about comments from DRT

[Mario Sanchez Prada]

Created attachment 212545 [details]
Patch proposal

(In reply to comment #10)
> (From update of attachment 212466 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212466&action=review
> 
> r- for some style considerations. thanks
> 
Sorry about those. It just a consequence of "expanding" the current function, which contained the errors already and I did not realize about it.

Now attaching a new patch. I'll be also filing a new bug to normalize those files so we don't use that anymore, to avoid issues/confusino when people modify these files in the future.

[Mario Sanchez Prada]

Committed r156551: <http://trac.webkit.org/changeset/156551>

[Krzysztof Czech]

*** Bug 112000 has been marked as a duplicate of this bug. ***