Last Comment Bug 627379 - Add test for @hidden attribute
: Add test for @hidden attribute
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Mark Capella [:capella]
:
Mentors:
http://dev.w3.org/html5/spec/Overview...
Depends on:
Blocks: html5a11y
  Show dependency treegraph
 
Reported: 2011-01-20 08:19 PST by David Bolter [:davidb]
Modified: 2012-03-04 05:06 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (3.99 KB, patch)
2012-03-01 16:43 PST, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v2) (3.53 KB, patch)
2012-03-01 20:31 PST, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v3) (3.52 KB, patch)
2012-03-01 22:55 PST, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v4) (4.20 KB, patch)
2012-03-01 23:49 PST, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v5) (4.36 KB, patch)
2012-03-02 01:15 PST, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Review

Description David Bolter [:davidb] 2011-01-20 08:19:20 PST
Filing to keep this on radar (post FF4).
Comment 1 alexander :surkov 2012-02-08 04:11:01 PST
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] 2012-02-28 08:23:37 PST
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 2012-02-28 09:04:41 PST
(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] 2012-03-01 16:43:26 PST
Created attachment 602199 [details] [diff] [review]
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 2012-03-01 19:02:41 PST
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] 2012-03-01 20:31:53 PST
Created attachment 602252 [details] [diff] [review]
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 2012-03-01 20:40:55 PST
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] 2012-03-01 22:55:38 PST
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 ...
Comment 9 alexander :surkov 2012-03-01 23:27:41 PST
(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] 2012-03-01 23:31:17 PST
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] 2012-03-01 23:49:23 PST
Created attachment 602267 [details] [diff] [review]
Patch (v4)

Not sure what bothered me before, but this works ...
Comment 12 alexander :surkov 2012-03-02 00:10:16 PST
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] 2012-03-02 01:15:12 PST
Created attachment 602285 [details] [diff] [review]
Patch (v5)

Ever optimistic ...
Comment 14 alexander :surkov 2012-03-02 03:07:53 PST
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 16 Ed Morley [:emorley] 2012-03-04 05:06:47 PST
https://hg.mozilla.org/mozilla-central/rev/0c183373c89e

Note You need to log in before you can comment on or make changes to this bug.