New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add better fallbacks to SelectElement.value #255

Merged
merged 3 commits into from Nov 12, 2017

Conversation

Projects
None yet
2 participants
@cschramm
Copy link
Contributor

cschramm commented Sep 22, 2017

If a browser encounters a select element without any selected option element, it automatically pre-selects the first one. This change reflects that in FormElement.form_values.

Another option would be to reflect it in SelectElement.value (having the same effect on FormElement.form_values) but I guess that would be a rather huge breaking change considering code that expects SelectElement.value to return None if nothing is selected in the markup. On the other hand, it would match what you get when you check the select element's value e.g. in JavaScript.

@scoder

This comment has been minimized.

Copy link
Member

scoder commented Sep 22, 2017

If a browser encounters a select element without any selected option element, it automatically pre-selects the first one.

Thanks, probably a good idea. Test is missing, though. And I'm also not entirely sure if that's the right place to change it. There isn't any defaulting in that method currently...

@cschramm

This comment has been minimized.

Copy link
Contributor

cschramm commented Sep 22, 2017

If you're fine with breaking existing usages, I'd also favor doing it in SelectElement.value. Shall we take that direction?

To throw some standards at it, I think this sentence from the current W3C Recommendation applies:

If nodes are inserted [...], then, if the select element's multiple attribute is absent, the select element's display size is 1, and no option elements in the select element's list of options have their selectedness set to true, the user agent must set the selectedness of the first option element in the list of options in tree order that is not disabled, if any, to true.

The current W3C Proposed Recommendation also adds:

If nodes are inserted [...], then, if the select element’s multiple attribute is absent, [and] two or more option elements in the select element’s list of options have their selectedness set to true, [...] the user agent must [...] set the selectedness of all but the last option element with its selectedness set to true in the list of options in tree order to false.

I think the for loop in SelectElement.value should get reversed for that, as it currently returns the value of the first selected option instead of the last one.

Regarding the case where there is no option element that could be selected, the recommendation says:

The value IDL attribute, on getting, must return the value of the first option element in the list of options in tree order that has its selectedness set to true, if any. If there isn't one, then it must return the empty string.

We should thus return the empty string (opposed to None) in that case.

The code would be something like this (not tested):

if self.multiple:
    return MultipleSelectOptions(self)
options = _options_xpath(self)
try:
    selected_option = next(itertools.chain(
        (el for el in reversed(options) if el.get('selected') is not None),
        (el for el in options if el.get('disabled') is None)))
except StopIteration:
    return ''
value = selected_option.get('value')
if value is None:
    value = (selected_option.text or '').strip()
return value

@scoder

This comment has been minimized.

Copy link
Member

scoder commented Sep 30, 2017

Difficult decision. In any case, we need a simple and backwards compatible way for users to adapt their code if we decide to break it.

@cschramm cschramm force-pushed the cschramm:select-fallback branch 2 times, most recently from 4917676 to ae2cd26 Oct 5, 2017

@cschramm cschramm changed the title Add fallback for select value to form_values Add better fallbacks to SelectElement.value Oct 5, 2017

@cschramm

This comment has been minimized.

Copy link
Contributor

cschramm commented Oct 5, 2017

Implemented in SelectElement.value with tests and changelog entry. I decided against using the empty string in case there are no options as that would add the select to the form_values while browsers actually ignore it in that case.

@cschramm

This comment has been minimized.

Copy link
Contributor

cschramm commented Oct 5, 2017

OK, gotta fix the tests... 😄

@cschramm cschramm force-pushed the cschramm:select-fallback branch from ae2cd26 to 9229e44 Oct 5, 2017

Add better fallbacks to SelectElement.value
If a browser encounters a select element without any selected option element, it automatically pre-selects the first one. If multiple options are selected, all but the last one get deselected.

@cschramm cschramm force-pushed the cschramm:select-fallback branch from 9229e44 to b25b7c9 Oct 5, 2017

@scoder

This comment has been minimized.

Copy link
Member

scoder commented Nov 12, 2017

Thanks!

@scoder scoder merged commit f3a3375 into lxml:master Nov 12, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment