Node.toggleView did not show Node when style="display: none"

[drjayvee]

node-view's _isHidden() relied on hidden attribute,
now also checks and display style (in line with _hide and _show)

[drjayvee]

node-view was changed to use the hidden DOM attribute to hide/show elements.

The html5 spec says:

The hidden attribute must not be used to hide content that could legitimately be shown in another presentation. For example, it is incorrect to use hidden to hide panels in a tabbed dialog, because the tabbed interface is merely a kind of overflow presentation — one could equally well just show all the form controls in one big page with a scrollbar. It is similarly incorrect to use this attribute to hide content just from one presentation — if something is marked hidden, it is hidden from all presentations, including, for instance, printers.

There are many discussions and blog posts about this topic.

I don't know what the rationale for using the hidden attribute was, but maybe we should rethink that? What do the YUI accessibility wizards think about this? Should I maybe open a ticket about this?

[juandopazo]

A couple of questions:

• Shouldn't the style check be using getComputedStyle?
• Also, shouldn't the attribute be set to "hidden" instead of "true"?
• And shouldn't the check in isHidden be using hasAttribute instead of getAttribute?

[drjayvee]

• Ah yes, that should be getComputedStyle!
• I'm pretty sure that's fine. The spec says it's a boolean attribute, so it's analogous to disabled or selected. I've also found code that uses set("disabled", true).
• I agree

What about my point about the hidden attribute being not quite kosher to use for hiding nodes? It's certainly an API change, because hide() now has a slightly different effect than it did before.

Maybe @gerardkcohen could weigh in here!

[drjayvee]

@juandopazo there is no hasAttribute on Y.DOM :/
I amended the last commit, hope that works.

[juandopazo]

You can use this._node.hasAttribute in this case.

My point is that according to the spec:

A number of attributes are boolean attributes. The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value.

If the attribute is present, its value must either be the empty string or a value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.

Which would mean <div hidden>foo</div> is a hidden node and so is <div hidden="">foo</div>.

[drjayvee]

node/tests/unit/node.html uses set('selected', 'selected'), but the files in node/tests/perf/ all use set('disabled', true). These do work.

However the DOM spec says:

Parameters
name of type DOMString
The name of the attribute to create or alter.
value of type DOMString
Value to set in string form.

I played around in Firebug, and the following all work to disable an input:

el.setAttribute('disabled', 'disabled');
el.setAttribute('disabled', '');

// and to remove
el.removeAttribute('disabled');

Let's go with the spec!

I'm still uncomfortable with using the hidden attribute at all here. Any thoughts?

[drjayvee]

I fixed the tests

[gerardkcohen]

Hey guys, just got back from vacation. Give me some time to get settled and go through this.

[drjayvee]

Hey @gerardkcohen, have you had a chance to take a look yet? I'd like to close this before it's my time for vacation! :)

[gerardkcohen]

The move to use thehidden attribute was strictly an accessibility one, and it is the best way to properly communicate to AT users that an element is hidden. It is semantic description of state, and browsers that support it will apply display: none and remove from the accessibility tree. It is also supported by all AT software. The other option would be to set display:none manually and then apply aria-hidden="true" but that requires 2 steps to manage.. The links that you referenced are all old and do not apply to today's accessibility rules. The explanation in the HTML5 spec is an example of where hidden should not be used, not an example that it should not be used. Hope that helps.

[drjayvee]

All right, then I guess this PR can be merged! @juandopazo I can haz merge?

[juandopazo]

Ping reviewers @ericf @rgrove

[rgrove]

Sadly, we're now deep in the dark and dreary winter of code freeze, so non-critical non-documentation changes have to wait until the thaw. I'll merge this as soon as that happens (July 16th, if the calendar is right: https://github.com/yui/yui3/wiki/Development-Schedule )

[rgrove]

Merged into dev-master.