627379 - Add test for @hidden attribute

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

Filing to keep this on radar (post FF4).

Comment 1

[alexander :surkov (:asurkov)]

I don't think we need any special support of @hidden attribute on a11y side. 

So let's add mochitest for this. Please add treeupdate/test_hidden.html similar to test_visibility.html in same folder.

Comment 2

[Mark Capella [:capella]]

With this request, is the purpose of creating test_hidden.html to have a place to add later testcases associated with style="visibility: hidden", or will the initial version of test_hidden.html also include new testcases?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #2)
> With this request, is the purpose of creating test_hidden.html to have a
> place to add later testcases associated with style="visibility: hidden", or
> will the initial version of test_hidden.html also include new testcases?

I think all we should do here is to add two tests for setAttribute("hidden", "true") and removeAttribute("hidden");

test_visibility.html is quite complicated since visibility style handling is not trivial. Just copy some invoker and fit it to your needs. You can handle reorder event on container and then check the container tree.

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v1)

Well ... here's a very rough draft. I say "rough" because there's a lot here I'm not sure about in terms of satisfying this request.

I do know that if I fail to do the removeAttribute("hidden") after doing the setAttribute("hidden", "true") the test FAILS:

Passed: 11
Failed: 2
Todo: 0

failed | Different amount of expected children of ['div@id="t1_parent" node', address: [object HTMLDivElement], role: section, address: [xpconnect wrapped nsIAccessible]]. - got 0, expected 1

failed | Different amount of expected children of ['div@id="t1_container" node', address: [object HTMLDivElement], role: section, address: [xpconnect wrapped nsIAccessible]]. - got 0, expected 1

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 602199 [details] [diff] [review]
Patch (v1)

Review of attachment 602199 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/treeupdate/test_hidden.html
@@ +43,5 @@
> +            ] }
> +          ] };
> +        testAccessibleTree(aContainerID, tree);
> +
> +        getNode(aParentID).style.visibility = "hidden";

you should set hidden attribute here instead

@@ +75,5 @@
> +
> +    function doTest()
> +    {
> +      document.getElementsByTagName("div")[3].setAttribute("hidden", "true");
> +      document.getElementsByTagName("div")[3].removeAttribute("hidden");

this should be part of invokers

@@ +79,5 @@
> +      document.getElementsByTagName("div")[3].removeAttribute("hidden");
> +
> +      gQueue = new eventQueue();
> +
> +        gQueue.push(new test1("t1_container", "t1_parent"));

wrong indentation

@@ +109,5 @@
> +  <div id="t1_container">
> +    <div id="t1_parent">
> +      <div id="t1_parent" style="visibility: visible">XYZZY</div>
> +    </div>
> +  </div>

that's enough to have:
<div id="contianer"><input id="child"></div>

change @hidden attribute on input and test accessible tree for div

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v2)

I believe I made all the changes you suggested in Patch (v2), but then also changed a few other things (experimenting). This version as-is tests clean, though oddly the passed/failed/to-do summary dissappears.

Commenting out the line ... document.getElementsByTagName("input")[0].removeAttribute("hidden"); ... causes the passed/failed/to-do summary to re-appear but with one failed test ... failed | Different amount of expected children of ['div@id="container" node', address: [object HTMLDivElement], role: section, address: [xpconnect wrapped nsIAccessible]]. - got 1, expected 0

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 602252 [details] [diff] [review]
Patch (v2)

Review of attachment 602252 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/treeupdate/test_hidden.html
@@ +25,5 @@
> +
> +    /**
> +     * Hide parent while child stays visible.
> +     */
> +    function test1(aContainerID)

please give function a proper name like setHiddenAttr

@@ +38,5 @@
> +          { SECTION: [
> +          ] };
> +        testAccessibleTree(aContainerID, tree);
> +
> +        document.getElementsByTagName("input")[0].setAttribute("hidden", "true");

use getNode("input") to get a node, it's shorter and faster since use getElementById

@@ +39,5 @@
> +          ] };
> +        testAccessibleTree(aContainerID, tree);
> +
> +        document.getElementsByTagName("input")[0].setAttribute("hidden", "true");
> +        document.getElementsByTagName("input")[0].removeAttribute("hidden");

I think setting and unsetting attribute may do nothing, you should keep removeAttribute("hidden") in another invoker

Comment 8

[Mark Capella [:capella]]

Attachment: Patch (v3)

This seems to fit the bill ... I took your suggestions and some further clues from test_doc.html ...

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #8)
> Created attachment 602263 [details] [diff] [review]
> Patch (v3)
> 
> This seems to fit the bill ... I took your suggestions and some further
> clues from test_doc.html ...

it fits the part of bill :) please add another invoker to test removeAttribute("hidden")

Comment 10

[Mark Capella [:capella]]

Ok! I actually had the code in for it, but saw something I wasn't sure about so I pulled it to get to a clean checkpoint ... will add back, address issues, and re-post asap -- mark

Comment 11

[Mark Capella [:capella]]

Attachment: Patch (v4)

Not sure what bothered me before, but this works ...

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 602267 [details] [diff] [review]
Patch (v4)

Review of attachment 602267 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/treeupdate/test_hidden.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +
> +<head>
> +  <title>change @hidden attribute on input and test accessible tree for div</title>

@hidden attribute testing

@@ +26,5 @@
> +    /**
> +     * Set @hidden attribute
> +     */
> +
> +    function setHiddenAttr(aContainerID, aChildID)

no empty line between a comment and function please

@@ +32,5 @@
> +      this.eventSeq = [
> +        new invokerChecker(EVENT_REORDER, getNode(aContainerID))
> +      ];
> +
> +      this.invoke = function invoke()

please prefix function names by constructor name like
this.invoke = function setHiddenAttr_invoke()

@@ +34,5 @@
> +      ];
> +
> +      this.invoke = function invoke()
> +      {
> +        getNode(aChildID).setAttribute("hidden", "true");

please test the tree before you change attribute to make sure the input in the tree

@@ +69,5 @@
> +
> +      this.finalCheck = function finalCheck ()
> +      {
> +        var tree =
> +          {  };

why do you test empty object as a tree?

@@ +96,5 @@
> +
> +      gQueue.push(new setHiddenAttr("container", "child"));
> +      gQueue.push(new removeHiddenAttr("container", "child"));
> +
> +      gQueue.invoke();    // SimpleTest.finish() will be called in the end

single space after gQueue.invoke(); and comment

@@ +112,5 @@
> +  <a target="_blank"
> +     title="change @hidden attribute on input and test accessible tree for div"
> +     href="https://bugzilla.mozilla.org/show_bug.cgi?id=627379">
> +     Mozilla Bug 627379
> +  </a>

you may not add a bug here since all we do is to add mochitests but if you want to then please specify the bug summary

Comment 13

[Mark Capella [:capella]]

Attachment: Patch (v5)

Ever optimistic ...

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 602285 [details] [diff] [review]
Patch (v5)

Review of attachment 602285 [details] [diff] [review]:
-----------------------------------------------------------------

excellent!

::: accessible/tests/mochitest/treeupdate/test_hidden.html
@@ +43,5 @@
> +
> +        getNode(aChildID).setAttribute("hidden", "true");
> +      }
> +
> +      this.finalCheck = function setHiddenAttr_finalCheck ()

no need whitespace between function name and ()

@@ +76,5 @@
> +
> +        getNode(aChildID).removeAttribute("hidden");
> +      }
> +
> +      this.finalCheck = function removeHiddenAttr_finalCheck ()

same

Comment 15

[alexander :surkov (:asurkov)]

landed https://hg.mozilla.org/integration/mozilla-inbound/rev/0c183373c89e

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0c183373c89e