Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix destructive select methods #269

Merged
merged 2 commits into from Feb 13, 2012

Conversation

Projects
None yet
2 participants
Contributor

jasonmp85 commented Feb 12, 2012

Both select_option and unselect_option were modifying the selected attribute of their target <option> element. This attribute is meant to be used solely as the marker for which <option>(s) is (are) the default selection(s) in a (multi-)<select> element. The actual selectedness of an <option> should be tracked using its selected property. Read the spec for more info.

This change removes any code which modified the selected attribute of <option> elements, which leaves only the code that modifies the selected property. In addition, Node#value needed to be changed to return <option> elements whose selectedness is true rather than just those with a selected attribute.

The capybara project proper walks over all <option> elements and manually deselects those which are not the new selection. Reading the spec for the select element reveals this is unnecessary:

If the multiple attribute is absent and the element's display size is greater than 1, then the user agent should also allow the user to request that the option whose selectedness is true, if any, be unselected. Upon this request being conveyed to the user agent, and before the relevant user interaction event is queued (e.g. before the click event), the user agent must set the selectedness of that option element to false and then queue a task to fire a simple event that bubbles named change at the select element, using the user interaction task source as the task source.

Some failing test cases are introduced in the first commit; however, after the second commit all tests pass.

jasonmp85 added some commits Feb 12, 2012

Add failing unit test for selected attribute bug
capybara-webkit's driver implements `(un)select_option` by modifying
the target node's `selected` attribute directly. The [HTML5 spec][]
states that this attribute represents the _default selectedness_ of an
`<option>` element, so modifying it will necessarily break the behavior
of reset buttons in forms.

In addition, the existing code leaves this attribute set on existing
`<option>` elements, which can put the page into an invalid state.

[HTML5 spec]: http://dev.w3.org/html5/spec/Overview.html#the-option-element
Fix destructive methods for selecting options
Both `select_option` and `unselect_option` were modifying the
`selected` attribute of their target `<option>` element. This attribute
is meant to be used solely as the marker for which `<option>`(s) is
(are) the default selection(s) in a (multi-)`<select>` element. The
actual _selectedness_ of an `<option>` should be tracked using its
`selected` **property**. Read [the spec][] for more info.

This change removes any code which modified the `selected` attribute of
`<option>` elements, which leaves only the code that modifies the
`selected` property. In addition, `Node#value` needed to be changed to
return `<option>` elements whose selectedness is true rather than just
those with a `selected` attribute.

The tests introduced in the previous commit now pass.

[the spec]: http://dev.w3.org/html5/spec/Overview.html#the-option-element
Contributor

jasonmp85 commented Feb 12, 2012

Depending upon how you guys use git bisect, the fact that I put the failing test in a separate commit may be a problem. If this patch is accepted, I can squash the commits as necessary.

@jferris jferris merged commit 2fef844 into thoughtbot:master Feb 13, 2012

Owner

jferris commented Feb 13, 2012

Thanks; I merged this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment