Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Select helpers unable to resolve slightly-complex expressions #1468

Closed
dougbu opened this issue Oct 28, 2014 · 2 comments
Closed

Select helpers unable to resolve slightly-complex expressions #1468

dougbu opened this issue Oct 28, 2014 · 2 comments
Assignees
Milestone

Comments

@dougbu
Copy link
Member

dougbu commented Oct 28, 2014

The expression-based select helpers (@Html.DropDownListFor() and @Html.ListBoxFor()) are able to handle only simple property-path expressions (e.g. model.Property1.Property2) when passed a non-null selectList argument. Any more complex expression generates a <select/> with no selected <option/>, despite a non-null expression result. This is confusing and limits the expression-based select helpers to what their string-based equivalents support.

This issue affects the <select/> tag helper as well. It always passes a non-null selectList argument to the underlying generator.

For example

  @Html.ListBoxFor(model => model, selectList)

works fine and will select the correct <option/> element in the generated HTML. But

  @{
    var collection = Model;
    @Html.ListBoxFor(model => collection, selectList)
  }

selects no <option/> elements in the generated HTML. Indexer expressions encounter the same issue.

Worse the expression is actually evaluated against the ViewDataDictionary content, potentially leading to incorrect expressions. None of the expression-based HTML helpers should use ViewDataDictionary at all. (Well almost none: The select helpers get their fallback selectList value from that dictionary.)

Workaround is to place the IEnumerable<SelectListItem> value in the ViewDataDictionary and pass selectList: null to the select helper. This uses the ViewDataDictionary entry and causes the helper to correctly evaluate the expression. But this workaround isn't discoverable and does not help the <select/> tag helper.


Root cause: The generator code falls back to ViewData.Eval() when determining the selected values. That method supports a very restricted set of expressions -- much less than can be done with a lambda.

@dougbu dougbu added the bug label Oct 28, 2014
@danroth27 danroth27 added this to the 6.0.0-rc1 milestone Oct 29, 2014
dougbu added a commit that referenced this issue Nov 7, 2014
- value may remain in the `FormContext` beyond `</select>` end tag but will
  be cleaned up at the `</form>` end tag of the containing `<form/>` element
 - `SelectTagHelper` called prior to helpers for contained `<option/>`s and
   not again later
- adjust mock setups to handle new `GenerateSelect()` call
- add assertions for expected `FormContext.FormData` entry

nit: mention #1468 in a test comment
dougbu added a commit that referenced this issue Nov 7, 2014
- value may remain in the `FormContext` beyond `</select>` end tag but will
  be cleaned up at the `</form>` end tag of the containing `<form/>` element
 - `SelectTagHelper` called prior to helpers for contained `<option/>`s and
   not again later
- adjust mock setups to handle new `GenerateSelect()` call
- add assertions for expected `FormContext.FormData` entry

nit: mention #1468 in a test comment
@rynowak
Copy link
Member

rynowak commented Nov 20, 2014

See SelectTagHelperTest.GeneratesExpectedDataSet (look for the TODO) when this is fixed for some test cases to enable.

@danroth27 danroth27 modified the milestones: 6.0.0-rc1, 6.0.0-beta3 Jan 13, 2015
dougbu added a commit that referenced this issue Mar 1, 2015
- see commit 9d5364c
- never correct to pass a `Func<object>` to `GetExplorerForExpression()`
- `<label/>` tests succeeded because that tag helper doesn't use expression result
- `<select/>` tests succeeded because that tag helper gets result from `ViewData`
 - does not use `ModelExplorer` due to #1468

nit: update variable names `metadata` -> `modelExplorer`
dougbu added a commit that referenced this issue Mar 2, 2015
- #1468
- Always use `ModelExplorer` in `<select/>`, `DropDownListFor()` and `ListBoxFor()` cases
 - allows evaluation of more-complex expressions
- Use `ViewData.Model` in `DropDownList()` and `ListBox()` template cases
 - `ViewData` was previously ignored in these cases

nit: change `ViewDataDictionary.Eval()` to return `Model` if `expression` is `null` or empty
- now `throw` on `null` or empty `expression` name in `ViewDataEvaluator.Eval()`
- simplifies some of the higher-level code
 - no change to `selectList` fallback; `Model` incorrect for that case
 - no change to `GenerateRadioButton()`; would change behaviour unrelated to #1468
  - this helper uses incorrect `ViewData` lookup text, see #1487
dougbu added a commit that referenced this issue Mar 2, 2015
- #1468
- Always use `ModelExplorer` in `<select/>`, `DropDownListFor()` and `ListBoxFor()` cases
 - allows evaluation of more-complex expressions
- Use `ViewData.Model` in `DropDownList()` and `ListBox()` template cases
 - `ViewData` was previously ignored in these cases

nit: change `ViewDataDictionary.Eval()` to return `Model` if `expression` is `null` or empty
- now `throw` on `null` or empty `expression` name in `ViewDataEvaluator.Eval()`
- simplifies some of the higher-level code
 - no change to `selectList` fallback; `Model` incorrect for that case
 - no change to `GenerateRadioButton()`; would change behaviour unrelated to #1468
  - this helper uses incorrect `ViewData` lookup text, see #1487
@dougbu
Copy link
Member Author

dougbu commented Mar 2, 2015

Fixed in commit ae4cafc

@dougbu dougbu closed this as completed Mar 2, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants