The default bug view has changed. See this FAQ.

Add test for @hidden attribute

RESOLVED FIXED in mozilla13

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: davidb, Assigned: capella)

Tracking

(Blocks: 1 bug)

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=js], URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
Filing to keep this on radar (post FF4).

Comment 1

5 years ago
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.
Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=js]
(Assignee)

Comment 2

5 years ago
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

5 years ago
(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.
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 years ago
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

5 years ago
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
(Assignee)

Comment 6

5 years ago
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

5 years ago
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
(Assignee)

Comment 8

5 years ago
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

5 years ago
(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")
(Assignee)

Comment 10

5 years ago
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
(Assignee)

Comment 11

5 years ago
Created attachment 602267 [details] [diff] [review]
Patch (v4)

Not sure what bothered me before, but this works ...
Attachment #602199 - Attachment is obsolete: true
Attachment #602252 - Attachment is obsolete: true
Attachment #602263 - Attachment is obsolete: true

Comment 12

5 years ago
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
(Assignee)

Comment 13

5 years ago
Created attachment 602285 [details] [diff] [review]
Patch (v5)

Ever optimistic ...

Comment 14

5 years ago
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
Attachment #602285 - Flags: review+

Updated

5 years ago
Attachment #602267 - Attachment is obsolete: true

Updated

5 years ago
Summary: Expose @hidden → Add test for @hidden attribute

Comment 15

5 years ago
landed https://hg.mozilla.org/integration/mozilla-inbound/rev/0c183373c89e
https://hg.mozilla.org/mozilla-central/rev/0c183373c89e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.