jquery / jquery

jQuery JavaScript Library
https://jquery.com
MIT License
58.93k stars 20.62k forks source link

Attributes: Only convert true, false & an empty string #5451

Closed mgol closed 2 months ago

mgol commented 2 months ago

Summary

The HTML spec defines boolean attributes: https://html.spec.whatwg.org/#boolean-attributes that often correlate with boolean properties. If the attribute is missing, it correlates with the false property value, if it's present - the true property value. The only valid values are an empty string or the attribute name.

jQuery tried to be helpful here and treated boolean attributes in a special way in the .attr() API:

  1. For the getter, as long as the attribute was present, it was returning the attribute name lowercased, ignoring the value.
  2. For the setter, it was removing the attribute when false was passed; otherwise, it was ignoring the passed value and set the attribute - interestingly, in jQuery >=3 not lowercased anymore.

The problem is the spec occasionally converts boolean attributes into ones with additional attribute values with special behavior - one such example is the new "until-found" value for the hidden attribute. Our setter normalization means passing those values is impossible with jQuery.

This patch takes a conservative approach, with backwards compatibility in mind, and makes the following changes:

  1. It explicitly only applies the logic to the few most popular attribute names.
  2. The getter only normalizes the empty string to the lowercased name, returning other attribute values unchanged - it seems unlikely that the empty string value will ever mean anything else than the attribute name repeated.
  3. The setter continues to treat false as mean attribute removal and true as setting the value to the attribute name. All the other inputs are aplied unchanged.

With this change, all existing jQuery unit tests were already passing.

Checklist

mgol commented 2 months ago

Size comparison:

main @f80e78ef3e7ded1fc693465d02dfb07510ded0ab
   raw     gz Filename
   -30    -29 dist/jquery.min.js
   -30    -26 dist/jquery.slim.min.js
   -30    -25 dist-module/jquery.module.min.js
   -30    -25 dist-module/jquery.slim.module.min.js

The PR - if this approach is accepted - still needs unit tests to confirm the new behavior. It is a bit reassuring that it didn't break any of our existing tests.

mgol commented 2 months ago

We've decided against this approach; closing.