[webkit-reviews] review requested: [Bug 106924] [GTK] GTK does not expose heading level correctly. Was: accessibility/heading-level.html is failing : [Attachment 212140] Patch proposal

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has asked  for review:
Bug 106924: [GTK] GTK does not expose heading level correctly. Was:
accessibility/heading-level.html is failing
https://bugs.webkit.org/show_bug.cgi?id=106924

Attachment 212140: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=212140&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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.