Skip to main content

Bug Tracker

Side navigation

#11316 closed enhancement (fixed)

Opened February 10, 2012 09:59AM UTC

Closed March 03, 2012 03:13AM UTC

Last modified March 03, 2012 03:27AM UTC

Consider looking through valHooks by element type first, then by nodeName instead of the other way around

Reported by: mathias Owned by: mathias
Priority: low Milestone: 1.7.2
Component: attributes Version: 1.7.1
Keywords: Cc:
Blocked by: Blocking:
Description

jQuery#val contains this line of code: http://jsapi.info/jquery/1.7.1/val#L2317

hooks = jQuery.valHooks[ elem.nodeName.toLowerCase() ] || jQuery.valHooks[ elem.type ];

This is later repeated on line 2361: http://jsapi.info/jquery/1.7.1/val#L2361.

How about switching these around, making it:

hooks = jQuery.valHooks[ elem.type ] || jQuery.valHooks[ elem.nodeName.toLowerCase() ];

This would allow plugin authors to set e.g. jQuery.valHooks.input without affecting the valHooks for checkbox etc. Here’s a more detailed use case: https://github.com/mathiasbynens/jquery-placeholder/issues/52

Note that there’s only one item in jQuery.valHooks that’s based on the node name: jQuery.valHooks.select. All others are element types.

Attachments (0)
Change History (9)

Changed February 10, 2012 10:22AM UTC by mathias comment:1

Related question: why is "value" (the string literal) used here instead of value (the variable name)? http://jsapi.info/jquery/git/val#L2372

if ( hooks && "get" in hooks && (ret = hooks.get( elem, "value" )) !== undefined ) {
  return ret;
}

Changed February 10, 2012 03:27PM UTC by timmywil comment:2

component: unfiledattributes
milestone: None1.7.2
priority: undecidedlow
status: newopen
type: featureenhancement

I agree with this change.

As for your comment, the variable value would not be appropriate, but the string "value" doesn't need to be passed there either. It is there for convenience (a reference to the property name being retrieved), but is not used in any of the internal hooks. I think I did that right after writing attrHooks, which always passes the attribute name. We could remove it.

Changed February 10, 2012 03:45PM UTC by mathias comment:3

description: `jQuery#val` contains this line of code: http://jsapi.info/jquery/git/val#L2370 \ \ {{{ \ hooks = jQuery.valHooks[ elem.nodeName.toLowerCase() ] || jQuery.valHooks[ elem.type ]; \ }}} \ \ How about switching these around, making it: \ \ {{{ \ hooks = jQuery.valHooks[ elem.type ] || jQuery.valHooks[ elem.nodeName.toLowerCase() ]; \ }}} \ \ This would allow plugin authors to set e.g. `jQuery.valHooks.input` without affecting the valHooks for `checkbox` etc. Here’s a more detailed use case: https://github.com/mathiasbynens/jquery-placeholder/issues/52 \ \ Note that there’s only one item in `jQuery.valHooks` that’s based on the node name: `jQuery.valHooks.select`. All others are element types.`jQuery#val` contains this line of code: http://jsapi.info/jquery/1.7.1/val#L2317 \ \ {{{ \ hooks = jQuery.valHooks[ elem.nodeName.toLowerCase() ] || jQuery.valHooks[ elem.type ]; \ }}} \ \ This is later repeated on line 2361: http://jsapi.info/jquery/1.7.1/val#L2361. \ \ How about switching these around, making it: \ \ {{{ \ hooks = jQuery.valHooks[ elem.type ] || jQuery.valHooks[ elem.nodeName.toLowerCase() ]; \ }}} \ \ This would allow plugin authors to set e.g. `jQuery.valHooks.input` without affecting the valHooks for `checkbox` etc. Here’s a more detailed use case: https://github.com/mathiasbynens/jquery-placeholder/issues/52 \ \ Note that there’s only one item in `jQuery.valHooks` that’s based on the node name: `jQuery.valHooks.select`. All others are element types.

Changed February 10, 2012 03:48PM UTC by mathias comment:4

Replying to [comment:2 timmywil]:

I agree with this change.

Glad to hear! (Also very happy that you set milestone to 1.7.2 — yay!)

As for your comment, the variable value would not be appropriate, but the string "value" doesn't need to be passed there either. It is there for convenience (a reference to the property name being retrieved), but is not used in any of the internal hooks. I think I did that right after writing attrHooks, which always passes the attribute name. We could remove it.

Oh, I see! The second argument confused me and made me think of set instead. I was just wondering :)

Changed February 10, 2012 04:17PM UTC by mathias comment:5

As discussed on IRC, it looks like jQuery.valHooks.button’s get and set take two and three arguments, respectively. For that reason it’s probably best to just leave the "value" stuff in there.

Pull request: https://github.com/jquery/jquery/pull/678

Changed February 10, 2012 04:19PM UTC by dmethvin comment:6

Should we consider this a breaking change? If so it should be deferred to 1.8. I honestly doubt there are a lot of external hooks but people are always surprising us.

Changed February 10, 2012 04:24PM UTC by timmywil comment:7

milestone: 1.7.21.8
owner: → mathias
status: openassigned

I highly doubt it will cause a raucous, but I suppose since we're in beta, we should push to 1.8. Thanks Dave.

Changed March 03, 2012 03:13AM UTC by Mathias Bynens comment:8

resolution: → fixed
status: assignedclosed

Fix #11316. Find valHooks first by element type, then by nodeName.

Reverses the previous search order.

Changeset: 2803a5e6f2704691fdd8ce4d34fe961d0192a0fb

Changed March 03, 2012 03:27AM UTC by dmethvin comment:9

milestone: 1.81.7.2