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

Most string-based HTML helpers use unprefixed ViewDataDictionary entries if called from templates #1487

Closed
dougbu opened this issue Oct 30, 2014 · 3 comments
Assignees
Milestone

Comments

@dougbu
Copy link
Member

dougbu commented Oct 30, 2014

The string-based helpers not mentioned in #1485 (@Html.TextArea() etc.) also do not behave as expected in template views. But this problem occurs only if an entry exists in the ViewDataDictionary matching the (un-prefixed) expression name. So unexpected values will be used in the generated HTML -- luckily, in a somewhat narrow case.

E.g. action contains

ViewData["Age"] = 39;
return View(new User()
{
  Age = 26,
  Dependent = new User()
  {
    Age = 13,
  },
});

Index.cshtml contains

@Html.DisplayFor(model => model.Dependent, templateName: "Dependent")

DisplayTemplates/Dependent.cshtml contains:

<td>
    @Html.Label(expression: "Age", labelText: null, htmlAttributes: new { @class = "control-label", })
</td>
<td>
    '@Html.Value(name: "Age")'
</td>

Result is:
capture

Expect to see "13" displayed between the quotes.

@dougbu
Copy link
Member Author

dougbu commented Oct 30, 2014

This problem was inherited from MVC 5.2.

Fix would probably require a general change in how we evaluate expressions that may be in ViewDataDictionary or its Model when !string.IsNullOrEmpty(ViewData.TemplateInfo.HtmlFieldPrefix). In that case, the full name of a dictionary entry and its name relative to the current Model are different. Should pass both names to ViewDataEvaluator.Eval(), allowing that helper to evaluate the full name against ViewDataDictionary entries. Then, if that fails, the helper would evaluate the relative name against ViewDataDictionary.Model. This preserves the current precedence of ViewDataDictionary entries over properties of ViewDataDictionary.Model.

The current code mixes ViewDataDictionary lookups with evaluating properties in the Model. This works fine in many cases, but not in this specific situation.

Fix isn't particularly risky because we have good but still-improving (see @pranavkm's work on #453) unit tests touching on ViewDataEvaluator.Eval(), Also have functional tests and samples demonstrating the behaviour of various HTML helpers.

@danroth27 danroth27 added this to the 6.0.0-rc1 milestone Nov 18, 2014
@yishaigalatzer yishaigalatzer modified the milestones: 6.0.0-beta3, 6.0.0-rc1 Jan 11, 2015
@yishaigalatzer yishaigalatzer modified the milestones: 6.0.0, 6.0.0-rc1 Feb 20, 2015
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
@Eilon Eilon removed this from the 6.0.0-beta5 milestone Apr 24, 2015
@Eilon Eilon added this to the 6.0.0-beta6 milestone May 8, 2015
@dougbu
Copy link
Member Author

dougbu commented Jun 6, 2015

Problem also affects how client validation attributes are found. For example with the following in an Int32.cshtml template,

@{
  // Don't do this; causes template to ignore `ViewData.Model` and overwrite existing `ViewData`.
  ViewData[string.Empty] = "two";
  ViewData[Html.ViewData.TemplateInfo.HtmlFieldPrefix] = 23;
}

@Html.TextBox(expression: string.Empty);

The generated HTML will be

<input id="z0__Number" name="[0].Number" type="text" value="23" />

The HTML helper finds the correct value but the incorrect ModelMetadata. (Client-side validation should require the int value but no data-val-required attribute is generated.)

dougbu added a commit that referenced this issue Jun 8, 2015
- test additional cases _close_ to these bugs as well

for #1485
- show odd `@Html.CheckBox()`, `@Html.Hidden()` behaviour in unit tests
- show odd `@Html.TextBox()` behaviour in functional tests (templates)

for #1487
- show odd `@Html.Value()` behaviour in unit tests
- show odd `@Html.RadioButton()`, `@Html.TextArea()` behaviour in functional tests
- show lack of validation attributes for `@Html.RadioButton()`, `<select>` tag helper

for #2662
- show odd `@Html.RadioButton(string.Empty)` behaviour in functional tests

for #2664
- show failures with `@Html.ListBox()` in unit tests

nits:
- test `IHtmlHelper` methods, not extensions
- use `ViewData`, not `ViewBag` in `HtmlGeneration_FormController`
- name test methods a bit more consistently
- rename `HtmlHelperValueExtensionsTest` to `HtmlHelperValueTest`
dougbu added a commit that referenced this issue Jun 9, 2015
- builds on dougbu/testing.1485.1487.2662.2664 branch, unblocking some #1487 tests
- use new property to correctly determine `isTargetEnum` in `GetCurrentValues()`
 - avoid `ArgumentNullException` in all cases where raw values are `enum` but target is not
- also use new property instead of private `GetElementType()` methods where possible

nits:
- move properties above methods in `ModelMetadata`
- avoid accidentally-incorrect "Remove Unnecessary Usings"
dougbu added a commit that referenced this issue Jun 9, 2015
- test additional cases _close_ to these bugs as well

for #1485
- show odd `@Html.CheckBox()`, `@Html.Hidden()` behaviour in unit tests
- show odd `@Html.TextBox()` behaviour in functional tests (templates)

for #1487
- show odd `@Html.Value()` behaviour in unit tests
- show odd `@Html.RadioButton()`, `@Html.TextArea()` behaviour in functional tests
- show lack of validation attributes for `@Html.RadioButton()`, `<select>` tag helper

for #2662
- show odd `@Html.RadioButton(string.Empty)` behaviour in functional tests

for #2664
- show failures with `@Html.ListBox()` in unit tests

nits:
- test `IHtmlHelper` methods, not extensions
- use `ViewData`, not `ViewBag` in `HtmlGeneration_FormController`
- name test methods a bit more consistently
- rename `HtmlHelperValueExtensionsTest` to `HtmlHelperValueTest`
dougbu added a commit that referenced this issue Jun 9, 2015
- test additional cases _close_ to these bugs as well

for #1485
- show odd `@Html.CheckBox()`, `@Html.Hidden()` behaviour in unit tests
- show odd `@Html.TextBox()` behaviour in functional tests (templates)

for #1487
- show odd `@Html.Value()` behaviour in unit tests
- show odd `@Html.RadioButton()`, `@Html.TextArea()` behaviour in functional tests
- show lack of validation attributes for `@Html.RadioButton()`, `<select>` tag helper

for #2662
- show odd `@Html.RadioButton(string.Empty)` behaviour in functional tests

for #2664
- show failures with `@Html.ListBox()` in unit tests

nits:
- test `IHtmlHelper` methods, not extensions
- use `ViewData`, not `ViewBag` in `HtmlGeneration_FormController`
- name test methods a bit more consistently
- rename `HtmlHelperValueExtensionsTest` to `HtmlHelperValueTest`
dougbu added a commit that referenced this issue Jun 9, 2015
- builds on dougbu/testing.1485.1487.2662.2664 branch, unblocking some #1487 tests
- use new property to correctly determine `isTargetEnum` in `GetCurrentValues()`
 - avoid `ArgumentNullException` in all cases where raw values are `enum` but target is not
- also use new property instead of private `GetElementType()` methods where possible

nits:
- move properties above methods in `ModelMetadata`
- avoid accidentally-incorrect "Remove Unnecessary Usings"
dougbu added a commit that referenced this issue Jun 9, 2015
- test additional cases _close_ to these bugs as well

for #1485
- show odd `@Html.CheckBox()`, `@Html.Hidden()` behaviour in unit tests
- show odd `@Html.TextBox()` behaviour in functional tests (templates)

for #1487
- show odd `@Html.Value()` behaviour in unit tests
- show odd `@Html.RadioButton()`, `@Html.TextArea()` behaviour in functional tests
- show lack of validation attributes for `@Html.RadioButton()`, `<select>` tag helper

for #2662
- show odd `@Html.RadioButton(string.Empty)` behaviour in functional tests

for #2664
- show failures with `@Html.ListBox()` in unit tests

nits:
- test `IHtmlHelper` methods, not extensions
- use `ViewData`, not `ViewBag` in `HtmlGeneration_FormController`
- name test methods a bit more consistently
- rename `HtmlHelperValueExtensionsTest` to `HtmlHelperValueTest`
dougbu added a commit that referenced this issue Jun 9, 2015
- builds on dougbu/testing.1485.1487.2662.2664 branch, unblocking some #1487 tests
- use new property to correctly determine `isTargetEnum` in `GetCurrentValues()`
 - avoid `ArgumentNullException` in all cases where raw values are `enum` but target is not
- also use new property instead of private `GetElementType()` methods where possible

nits:
- move properties above methods in `ModelMetadata`
- avoid accidentally-incorrect "Remove Unnecessary Usings"
dougbu added a commit that referenced this issue Jun 10, 2015
- test additional cases _close_ to these bugs as well

for #1485
- show odd `@Html.CheckBox()`, `@Html.Hidden()` behaviour in unit tests
- show odd `@Html.TextBox()` behaviour in functional tests (templates)

for #1487
- show odd `@Html.Value()` behaviour in unit tests
- show odd `@Html.RadioButton()`, `@Html.TextArea()` behaviour in functional tests
- show lack of validation attributes for `@Html.RadioButton()`, `<select>` tag helper

for #2662
- show odd `@Html.RadioButton(string.Empty)` behaviour in functional tests

for #2664
- show failures with `@Html.ListBox()` in unit tests

nits:
- test `IHtmlHelper` methods, not extensions
- use `ViewData`, not `ViewBag` in `HtmlGeneration_FormController`
- name test methods a bit more consistently
- rename `HtmlHelperValueExtensionsTest` to `HtmlHelperValueTest`
dougbu added a commit that referenced this issue Jun 10, 2015
- #2664
- use new property to correctly determine `isTargetEnum` in `GetCurrentValues()`
 - avoid `ArgumentNullException` in all cases where raw values are `enum` but target is not
- stop skipping tests blocked by #2664, exposing a couple more #1487 issues
- use new property instead of private `GetElementType()` methods where possible
 - cleans up some duplicate code
 - also remove redundant use of `IsCollectionType` and `ElementMetadata`

nits:
- move properties above methods in `ModelMetadata`
- avoid accidentally-incorrect "Remove Unnecessary Usings"
dougbu added a commit that referenced this issue Jun 10, 2015
- #1485, #1487
 - handle `TemplateInfo.HtmlFieldPrefix` in `ViewDataEvaluator.Eval()`
  - attempt lookup in the `ViewDataDictionary` using full name then evaluate
    relative `expression` against `viewData.Model`
  - handle `null` or empty `expression` special case in this method (remove `throw`s)
 - always pass relative `expression` name into `Eval()`
  - remove `null` or empty `expression` handling from higher-level code
  - in a couple of cases, special-case returned `ViewDataInfo`
- #2662
 - remove incorrect guard from `DefaultHtmlGenerator.GenerateRadioButtion()`
- add doc comments for the core methods that have changed
- enable unit tests skipped due to one of above bugs
 - fix one (yeah, just one) other test with incorrect expectations
- remove functional test comments about the above bugs and update expectations

nits:
- move some comments describing `ViewDataEvaluator` methods above the methods
dougbu added a commit that referenced this issue Jun 17, 2015
- #1485, #1487
 - handle `TemplateInfo.HtmlFieldPrefix` in `ViewDataEvaluator.Eval()`
  - attempt lookup in the `ViewDataDictionary` using full name then evaluate
    relative `expression` against `viewData.Model`
  - handle `null` or empty `expression` special case in this method (remove `throw`s)
 - always pass relative `expression` name into `Eval()`
  - remove `null` or empty `expression` handling from higher-level code
  - in a couple of cases, special-case returned `ViewDataInfo`
- #2662
 - remove incorrect guard from `DefaultHtmlGenerator.GenerateRadioButtion()`
- add doc comments for the core methods that have changed
- enable unit tests skipped due to one of above bugs
 - fix one (yeah, just one) other test with incorrect expectations
- remove functional test comments about the above bugs and update expectations

nits:
- move some comments describing `ViewDataEvaluator` methods above the methods
@dougbu
Copy link
Member Author

dougbu commented Jun 17, 2015

27283ec

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