medialize / ally.js

JavaScript library to help modern web applications with accessibility concerns
http://allyjs.io/
MIT License
1.53k stars 83 forks source link

getter for labels of a form control #42

Open rodneyrehm opened 8 years ago

rodneyrehm commented 8 years ago

add a getter to conveniently retrieve the <label> elements for any given labelable elements

aFarkas commented 8 years ago

a start:

//no dom position/order / no live nodelist, not even a true nodelist
['HTMLButtonElement', 'HTMLInputElement', 'HTMLMeterElement', 'HTMLOutputElement', 'HTMLKeygenElement', 'HTMLProgressElement', 'HTMLSelectElement', 'HTMLTextAreaElement'].forEach(function(element){
    var slice = [].slice;
    if(window[element] && !('labels' in window[element].prototype)){
        Object.defineProperty(window[element].prototype, 'labels', {
            get: function(){
                if(this.type == 'hidden'){return null;}
                var hFor, labels;
                var id = this.id;
                var implicitLabel = this.closest('label');
                if(implicitLabel && (hFor = implicitLabel.attributes['for']) && hFor.value != id){
                    implicitLabel = null;
                }

                if(id){
                    labels = slice.call(this.ownerDocument.querySelectorAll('label[for="'+ id +'"]'));
                } else {
                    labels = [];
                }

                if(implicitLabel && labels.indexOf(implicitLabel) == -1){
                    labels.push(implicitLabel);
                }

                return labels;
            },
            enumerable: true,
            configurable: true,
        });
    }
});
rodneyrehm commented 8 years ago

nice! did you write this off the top of your head, or did you have it lying around already?

  1. Is it ok to polyfill/shim this, since we can't return a NodeList? I'd have gone with a regular function (that needs to be imported wherever it's used). Then exposing these things onto the built-ins and or jQuery would be a separate module.
  2. doesn't the implicitLabel have to be sorted into labels according to its order in the DOM?
  3. as for the querySelectorAll() I'd have used CSS.escape like here.
  4. nice catch with the parental <label for="different-id">. Did you test label/control association, or is the coded behavior an assumption?

Do you want to prepare a PR for this, or should I just throw it in whenever I get to it?

aFarkas commented 8 years ago

It's part of webshim (5 year old project). Which used $.attr and $.prop as a save wrapper. Here is the jQuery code.

  1. Due to the fact, that you have talked about writing a getter, I assumed you want to polyfill it. It wouldn't be a pure polyfill tough. But easy to change.
  2. As I said in the comment above. Neither (live) nodelist nor sorted DOM order. If you use the jQuery version you have it for free, otherwise compareDocumentPosition.
  3. Good catch, as said 5 year old project, probable 3 year old code.
  4. The algorithm follows the spec.

I think, it would be better if you do it, because I don't know the project very well (Didn't know, that you have CSS.escape in there...). I'm just watching you ;-).

rodneyrehm commented 8 years ago

It's part of webshim (5 year old project).

Didn't even search the web for an implementation before adding this issue - stupid me could've thought of that first… Shame webshim requires jQuery and is not modular.

otherwise compareDocumentPosition.

I think we're able to avoid that and simplify the code by simply setting [for=$id] on the parental <label> and have querySelectorAll() figure it out, no? Although I'm not sure how nice it is to transparently modify the DOM that way…

I think, it would be better if you do it […]

ok, will look into it for 1.1 - thanks for the help so far!

I'm just watching you ;-)

That's not at all creepy ;)

aFarkas commented 8 years ago

I think we're able to avoid that and simplify the code by simply setting [for=$id] on the parental

No ;-). It does not only alter the dom, it invalidates the layout and this is not a predictable behavior, of such an API.

rodneyrehm commented 8 years ago

No ;-). It does not only alter the dom, it invalidates the layout and this is not a predictable behavior, of such an API.

does it invalidate layout or style? As far as I know this would "only" cause a style operation no a layout operation (unless style changed). I see your point though, the long compareDocumentPosition it is.

Isn't a similar hack used by jQuery's $.fn.find() to combat that context issue querySelectorAll() has/had?

aFarkas commented 8 years ago

, the long compareDocumentPosition it is.

If you need this feature.

Isn't a similar hack used by jQuery's $.fn.find() to combat that context issue querySelectorAll() has/had?

Yes, it seems so and it invalidates style:

Compare on this site (github):

$('.header > div').offset();

$('> div', document.querySelector('.header')).offset();
rodneyrehm commented 8 years ago

, the long compareDocumentPosition it is.

If you need this feature.

I assume screen readers announce the labels in DOM order, so maintaining that order makes sense and seems the most intuitive - regardless of actual need - don't you think?

Yes, it seems so and it invalidates style:

I did not think of that when proposing the "just change id" idea, thanks for the input!

aFarkas commented 8 years ago

I assume screen readers announce the labels in DOM order

My research on this is 4-6 years old. Most only announced just one label, either the first or the last. Which of both depends on the browser (not the screenreader) you use. (My research was with FF, IE8 (maybe also 9) and Jaws, NVDA and another Screenreader (mostly used in germany, but forgot it's name)).

But I guess this has changed since then.

If you are on windows (I guess not). This is a fantastic a11y tool (It only shows, what the app exposes, not what the screenreader does with it or whether the screenreader works around missing informations): https://msdn.microsoft.com/en-us/library/ms696079.aspx

rodneyrehm commented 8 years ago

To be honest, I haven't done any real screen reader tests yet (nor was I planning on doing that any time soon). Looks like this project could use some manpower in that field hint hint ;)

aFarkas commented 8 years ago

I already have a project, that takes my time... Let's talk in amsterdam.

rodneyrehm commented 8 years ago

I already have a project, that takes my time...

everyone does… hm…

Let's talk in amsterdam.

oh, you're coming? looking forward to a chat then! :)