Bug Tracker

Opened 6 years ago

Closed 6 years ago

#13539 closed feature (fixed)

Stop overriding Sizzle.attr

Reported by: gibson042 Owned by:
Priority: low Milestone:
Component: attributes Version: 2.0b2
Keywords: Cc:
Blocked by: Blocking:

Description

jQuery.attr replaces Sizzle.attr and adds value in the following ways:

  1. safely no-ops on non-element input
  2. falls back to jQuery.prop in the absence of getAttribute
  3. sets an attribute when value is defined
  4. allows jQuery.attrHooks to override standard getters/setters for HTML attributes
  5. normalizes booleans, value (qua defaultValue), and oldIE attroperty confusion

However, replacing Sizzle.attr comes with some consequences:

  • Sizzle uses it instead of its own function for resolving selectors
  • Code is duplicated between the two libraries
  • Several fixes that should have made it to Sizzle are not there because we patched jQuery

I'd like to stop overriding the Sizzle function and instead let jQuery defer to it for getting attribute values. This will necessitate moving the jQuery fixes into Sizzle (point 5), and will also mean that .is and .attr can disagree if someone defines a get attrHook (or alternatively, that we should do away with get hooks).

Does this idea have legs?

Change History (5)

comment:1 Changed 6 years ago by dmethvin

It would be worth investigating how to clean that up. A lot of the hooks were put in for cases that we can hopefully put behind us. I don't think we should be encouraging anyone outside the jQuery team to add any hooks, they have too many implications.

How crazy does this get now that we have two actively maintained versions and two selector engine modules?

comment:2 Changed 6 years ago by gibson042

Component: unfiledattributes
Milestone: None
Priority: undecidedlow

2.x has no get attrHooks, so this should just mean tweaks in Sizzle and deletes in the 1.x attributes module.

comment:3 Changed 6 years ago by timmywil

Maybe this has legs for 2.x, but not for 1.x. Sizzle's attribute handling is not meant to be comprehensive. It was designed with the intention that any library could override it and fill in all of the edge cases. Sizzle.attr is small. It should stay small and separating them in jQuery doesn't mean less duplicated code, it means more. I'm not even sure it should be done for 1.x.

comment:4 Changed 6 years ago by timmywil

Status: newopen
Version: git2.0b2

I meant 2.x on that last. If Sizzle.attr has exactly the same results as jQuery.attr in 2.0 right now, then it would be possible to not use jQuery.attr. Still, the point on the duplicated code is in the wrong list. And as I was saying, points 1 and 3 are good things. As a side note, I'm not sure .is and .attr need to disagree. Use attrHooks at your own risk. A blindly-implemented custom attrHook will probably screw up .attr too.

comment:5 Changed 6 years ago by Richard Gibson

Resolution: fixed
Status: openclosed

Fix #13539: Utilize Sizzle hooks. Close gh-1215. (cherry picked from commit 4ef516903e6e48bce388ca47c1ed88a447199fa1)

Changeset: 5d1dfe747403c093cc0e837651dcf80027387fc6

Note: See TracTickets for help on using tickets.