Skip to content
Permalink
Browse files

Reduce boolean attribute list to only content attributes

- Removed IDL-only boolean attributes as well as officially deprecated attributes that we've not supported before
  • Loading branch information...
timmywil
timmywil committed May 6, 2011
1 parent f30cd41 commit a257e07ae48d6af98fda02669e15d96bf0b21bd3
Showing with 1 addition and 1 deletion.
  1. +1 −1 src/attributes.js
@@ -6,7 +6,7 @@ var rclass = /[\n\t\r]/g,
rtype = /^(?:button|input)$/i,
rfocusable = /^(?:button|input|object|select|textarea)$/i,
rclickable = /^a(?:rea)?$/i,
rboolean = /^(?:autobuffer|autofocus|autoplay|async|checked|compact|controls|declare|defaultmuted|defaultselected|defer|disabled|draggable|formnovalidate|hidden|indeterminate|ismap|itemscope|loop|multiple|muted|nohref|noresize|noshade|nowrap|novalidate|open|pubdate|readonly|required|reversed|scoped|seamless|selected|spellcheck|truespeed|visible)$/i,
rboolean = /^(?:autofocus|autoplay|async|checked|controls|declare|defer|disabled|draggable|formnovalidate|hidden|ismap|loop|multiple|muted|noresize|noshade|nowrap|novalidate|open|pubdate|readonly|required|reversed|scoped|seamless|selected|truespeed)$/i,
rinvalidChar = /\:/,
formHook, boolHook;

6 comments on commit a257e07

@aFarkas

This comment has been minimized.

Copy link
Contributor

aFarkas replied May 6, 2011

remove also pure content attributes like novalidate. the follwoing get-method

return elem[ propfix[name] || name] ? name : undefined;

will fail, becuase elem.novalidate should be undefined. There might be some others boolean attributes, which do not have a corresponding no idl attributes.

A similiar problem is here with all HTML5 attributes.

If a browser doesn't support this attribute the idl will be undefined. so the content attribute will be also undefined even if the attribute was set.

@aFarkas

This comment has been minimized.

Copy link
Contributor

aFarkas replied May 7, 2011

@timmywil

I think you should reduce the rboolean list to those, which are a) supported cross-browser and b) have both a content and an idl attribute.

Here is a list of suggested attributes:

/^(?:checked|disabled|multiple|readonly|selected)$/i,
  1. removed all html5 attributes, because they don't have prop in unsupported browsers, so treating those as boolean properties will fail to often. Additionally most attributes here are true reflecting attributes so attr will work in most of those cases, without special treatment (The only content attribute, which does not reflect is muted).
  2. removed the following html attributes, because they don't have an corresponding idl and will fail in all browsers:
    ismap, noresize, noshade, nowrap, novalidate (also html5), formnovalidate (also html5)
  3. removed attributes, because they are depreacated:
    truespeed (if you want this, you should add trueSpeed to propfix)
  4. removed attribute, because it isn't used and there is no need, to make it backward-compatible:
    declare
@timmywil

This comment has been minimized.

Copy link
Member

timmywil replied May 7, 2011

You may have me sold on 3-4. I agree with 1-2 as well except not having them in the list will cause the attributes to be set in a way that is not valid. Setting a non-existent IDL in IE is fine and will make the return values consistent even though nothing happens when you do set it to true. Point being I think we'll run into more problems without setting it (e.g. getting back empty string as the value for autofocus when it looks like <input autofocus>). Anyway, if we can remove support for setting the properties at all in some future release, we will want these in the regex so that the content attribute is set properly.

@timmywil

This comment has been minimized.

Copy link
Member

timmywil replied May 7, 2011

@aFarkas

This comment has been minimized.

Copy link
Contributor

aFarkas replied May 7, 2011

First, yes I'm info.corruptsystem/trixta/alex/aFarkas.

I haven't fully understand how you want to do it. But seems ok to me.

If you want to make this for back-compat reasons, I don't see a reason to add html5 elements to the list, because jquery never acted like this.

If you want to make a "feature" out of this (i.e.: attr is for attributes + boolean properties). It sounds good, but I don't like it, because this will "cement" the property handling by attr and this is/was something you wanted to get rid off.

You want to make something like this?:

if(bool){
    if( !name in elem ){
        elem.name = hasAttribute(elem, name);
    }
    return elem[name] ? name : undefined;

}

+Something offtopic:
Although, I'm annoying, trolling and critizing, I really and honestly appreciate your hard work!!!

@timmywil

This comment has been minimized.

Copy link
Member

timmywil replied May 7, 2011

The thing I want to get rid of is setting the property (attr should be for content attributes, not setting IDLs imo). However, setting boolean content attributes will always need special treatment when setting to true/false if we want it to be technically valid. We can either add support for the html5 attributes now or take it out entirely, but it has already been added in 1.6 (even though 1.6 came with some issues). So, I've made a fiddle for testing all the boolean attributes: http://jsfiddle.net/timmywil/xqnC2/ and there are definitely inconsistencies. Using the boolHook for content attributes which don't have an IDL in any browser will not work (nor will camelCase attributes that we're not willing to add to propFix). Also, I'd like things like async and required to return undefined in IE6 even when set to true (cause that's what happens natively and that's what it did before). Anyway, have a look at the next revision and see the fiddle for how the attributes will be treated.

PS - Healthy criticism is refreshing and helpful! You are no troll.

Please sign in to comment.
You can’t perform that action at this time.