Skip to content
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

Bug - Conditional Attributes #16333

Closed
legistek opened this issue Mar 25, 2018 · 14 comments
Closed

Bug - Conditional Attributes #16333

legistek opened this issue Mar 25, 2018 · 14 comments
Labels
area-blazor Includes: Blazor, Razor Components

Comments

@legistek
Copy link

Title

Attributes wrapped in a @ expression always get applied.

Functional impact

Cannot make attributes - such as disabled - conditional on a C# expression/property.

Minimal repro steps

Try the following CSHTML:

<button @{ if (false) { <text>disabled</text> } }>
    I shouldn't be disabled
</button>

Expected result

Button should not be disabled because the conditional evaluates to false.

Actual result

The button is actually disabled.

Further technical details

There is also no obvious way to add or remove the disabled attribute through C# code without going into internals.

Notably, specifying CSS properties with @ expressions inside a <style> block works great.

@jamie-lord
Copy link

This works:

<button disabled=@Test>I shouldn't be disabled</button>

@functions {
    public string Test { get; set; } = "disabled";
}

@legistek
Copy link
Author

Ok and what if I want to then reenable it through code?

The value of the disabled attribute is ignored by browsers. So the mere presence of the attribute causes it to be disabled.

@campersau
Copy link
Contributor

campersau commented Mar 26, 2018

The generated code for @pmoorelegistek example looks crazy:

 if (false) {  } builder.OpenElement(18, "button");
builder.AddAttribute(19, "disabled", "");
builder.AddContent(20, "\n    I shouldn\'t be disabled\n");
builder.CloseElement();

Simple booleans also don't work:

cshtml:

<input type="checkbox" checked="@(1==0)" />

generated code:

builder.OpenElement(27, "input");
builder.AddAttribute(28, "type", "checkbox");
builder.AddAttribute(29, "checked", 1==0);

rendered:

<input type="checkbox" checked="False">

It looks like the problem for this is here:

https://github.com/aspnet/Blazor/blob/c7e35fdbeb7aed56e71018315f6d75e6d8be62f5/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeBuilder.cs#L135-L139

@danroth27
Copy link
Member

We still need to add support for conditional attributes. Normally the syntax in Razor would be <button disabled=@isDisabled>.

@legistek
Copy link
Author

Ah ok. Yes normally that would be the syntax and in fact it appears that would work with Blazor now with most attributes. The problem is a little unique to disabled because merely having the attribute present - regardless of value - disables the control. So the attribute needs to be completely removed to re-enable the control. Thanks for being on this!

@miroslavp
Copy link

miroslavp commented Mar 26, 2018

Can anyone tell me if this is normal? When I type
<div class='some-class @((someBool && anotherBool) ? "class-one" : "class-two")' />
I get an error message "Cannot implicitly convert type 'string' to 'bool'
But if I change it to:
<div class='some-class @(((someBool && anotherBool) ? "class-one" : "class-two"))' />
then it is OK :
What's wrong with the first evaluation?

@legistek
Copy link
Author

legistek commented Mar 26, 2018

@miroslavp the only way I was able to get conditional classing working was the following:

class="@(this.Class)"

where I would then compose the value for this.Class using C# string building.

That's consistent with your second example. Assume you need the first set of parentheses at minimum, e.g., @(some_string).

Since you're using the ? and : operators, usually in my experience you need to wrap that whole expression in parentheses as well. For example:
var str = $"{(true ? "yes!" : "no")}";

So that makes sense you'd need the 2 sets of parentheses as in your second example.

@ssewell
Copy link

ssewell commented Mar 26, 2018

@miroslavp the double parenthesis is to be expected for the conditional class syntax. Check out this site, it has some great documentation: https://learn-blazor.com/pages/data-binding/

@SteveSandersonMS
Copy link
Member

It's not implemented yet, but my expectation was that we'd have the following special cases:

  • If the value you're binding is a bool with value False, then we don't apply the attribute at all.
    • It's a breaking change, but I think it's still what we want. So if your goal was literally to produce a DOM attribute like something="False", then you'd need to write something=@MyBoolValue.ToString() to create that effect.
  • If the value you're binding is a reference type with value null, then we don't apply the attribute at all. Because it's not like it's useful to create a DOM attribute with value "null".

This would make it easy to do things like:

<input type="checkbox" bind=SomeBoolProperty />
and
<div class=@(SomeBoolProperty ? "some-class" : null)>...</div>

Can anyone think of any drawbacks to this design?

@rstropek
Copy link

Looks great. Would it be possible to extend this a little bit and support something like Angular's class bindings? <div class.highlight=@(SomeBoolProperty)>...</div> would add/remove the class based on the boolean property. I find these bindings quite easy to understand and useful when working with Angular.

@RyoukoKonpaku
Copy link
Contributor

RyoukoKonpaku commented Mar 29, 2018

It could be like a C# dictionary I presume? maybe @class["Class to apply"]="Condition Here"
+1 @rstropek idea as it's cleaner than putting alot of conditional operators on the class attribute especially on cases when adding a class when it satisfies something e.g. EnumValue for status label.

@ssewell
Copy link

ssewell commented Mar 29, 2018

Exactly. We need to cover the use case if we have both a conditional and non-conditional class we need to apply.

For example we have the element:
<input class="form-control" type="text" />

...to which we wish to apply the class "active" when the boolean isActive is true.

@legistek
Copy link
Author

All good ideas. You could also have a special @disabled(bool) expression since it's such a special - albeit extremely common - case, if you don't want to do anything too far reaching.

@rynowak
Copy link
Member

rynowak commented Mar 30, 2018

@SteveSandersonMS

I'm going to start on this next with the following additional clarifications. We can certainly do more, but I think we don't have a reason NOT to do the following.

  • If the value you're binding is a bool with value False, then we don't apply the attribute at all.
  • If the value you're binding is a reference type with value null, then we don't apply the attribute at all. Because it's not like it's useful to create a DOM attribute with value "null".
  • (added) if the value is a bool with value True, then write the attribute as minimized `

The last point might require some work in BrowserRenderer. I'm ok with leaving it out if there's a good reason to not do it, but it would then be fully consistent with Text-mode Razor.

So if your goal was literally to produce a DOM attribute like something="False", then you'd need to write something=@MyBoolValue.ToString() to create that effect.

Text-mode Razor does the same. You can use an explicit string if you really want to see the value.


I also want to see if we can do something to get rid of the ternaries, or maybe build something specialized around class, but I don't think that's important for the first cut.

@mkArtakMSFT mkArtakMSFT transferred this issue from dotnet/blazor Oct 27, 2019
@mkArtakMSFT mkArtakMSFT added the area-blazor Includes: Blazor, Razor Components label Oct 27, 2019
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

No branches or pull requests