fix: Boolean binding to some FAST control attributes does not behave as expected

[rajsite]

🐛 Bug Report

Some boolean attributes on FAST controls do not behave how mdn or FAST template binding expect boolean attributes to behave.

This issue impacts all boolean attributes configured for a default of true such as dialog's modal property discussed in the issue and could potentially be avoided for picker's filter-selected and filter-query attributes which is in alpha.

💻 Repro or Code Sample

The Fast Dialog's modal attribute cannot be set to false as expected with <fast-dialog></fast-dialog>. You have to use the unexpected <fast-dialog modal="false"></fast-dialog> which does not follow boolean attribute conventions in browser or work with FAST's own boolean attribute template binding.

See the following StackBlitz where trying to bind to the modal boolean attribute of Dialog the binding does not work as expected: https://stackblitz.com/edit/fast-boolean-binding-dialog-modal?file=index.ts

🤔 Expected Behavior

I expect to be able to use boolean attributes as documented and expected in web applications, where absence of an attribute corresponds to a false value and presence of an attribute corresponds to a true value.

😯 Current Behavior

I cannot set some boolean attributes to false with expected attribute conventions. The unexpected attribute strings "false" and "true" do appear to give boolean behavior in these cases.

💁 Possible Solution

• Clearly document boolean properties that do not behave normally and must be configured with the strings "true" or "false" instead of normal boolean attribute behavior.

• It would be a breaking API change to correct this behavior and make it behave as expected. One option would be to change these properties from boolean attributes to string attributes where the attribute values "enabled", "true", "", and null correspond to boolean value true and the attribute values "disabled" and "false" correspond to boolean value false. Then deprecate usages of the string values other than "enabled" or "disabled" to encourage the use of those attribute strings in docs.

• Require that all future boolean attributes default to the value false. Attributes may need a negated name for good defaults. ie if default modal true is desired then the attribute should instead be called nonmodal and correspond to JS property modal.

🔦 Context

This behavior was not intuitive and something we had to document clearly and make our users aware of using elements built on FastFoundation.

🌍 Your Environment

"@microsoft/fast-element": "^1.4.1",
"@microsoft/fast-foundation": "^2.12.0",

[chrisdholt]

Thanks @rajsite for this and the specific repro. I don't think this is expected; rather, I think this is a bug. The boolean operator should work in that scenario and it looks like the code shows it should be supported:

• @EisenbergEffect as FYI

[rajsite]

@chrisdholt

I think the issue is that the expected way to handle boolean attributes where lack of the attribute means the value false and presence of the attribute means the value true is incompatible with a boolean having default value true.

ie the way I would express that modal is false in HTML would be

<fast-dialog></fast-dialog>

And the way I would express modal is true in HTML would be

<fast-dialog modal></fast-dialog>

However the first case coerces to true because the "default" of the boolean is set to true.

Another way to think about it is defaults for boolean attributes don't apply because the value is always well-defined. There is no default value / default values for boolean attributes should be ignored.

[EisenbergEffect]

That the default value of true is conflicting does seem to make sense to me. Should we have a practice (and documentation) that any boolean attribute be set to false by default?

[chrisdholt]

That the default value of true is conflicting does seem to make sense to me. Should we have a practice (and documentation) that any boolean attribute be set to false by default?

I think this would make sense. The potential alternative would be that if we had something which needed to be true we would have to ensure that we explicitly check when binding to the host that it IS true and not just having a defined value - but I'd prefer to not have to have some kind of special syntax for scenarios like that.

[chrisdholt]

On this - I'd like to see if this specific scenario can be mitigated without needing to break it. I have an idea of how to address it.

[EisenbergEffect]

I'm not sure how to address the problem with the component without technically breaking it. 😢 Love to hear your idea.

[chrisdholt]

I'm not sure how to address the problem with the component without technically breaking it. 😢 Love to hear your idea.

None of them work. :(

Let's discuss the best way to address this...

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[chrisdholt]

Will be picked up with FAST Foundation vNext work

[chrisdholt]

Closing per the above (soon to be merged in with the feature branch)

[rajsite]

@chrisdholt @EisenbergEffect In the fast-element-2 branch it looks like there are a couple more instances of a boolean attribute defaulting to true from a quick non-exhaustive search:

Another in the dialog (trapFocus):

• https://github.com/microsoft/fast/blob/features/fast-element-2/packages/web-components/fast-foundation/src/dialog/dialog.ts#L51

And a couple others components:

• https://github.com/microsoft/fast/blob/features/fast-element-2/packages/web-components/fast-foundation/src/picker/picker.ts#L117
• https://github.com/microsoft/fast/blob/features/fast-element-2/packages/web-components/fast-foundation/src/picker/picker.ts#L127
• https://github.com/microsoft/fast/blob/features/fast-element-2/packages/web-components/fast-foundation/src/tabs/tabs.ts#L150

1. Will those be updated to default to false as well?
2. Is there a type change / or runtime check for the decorator to assert that the value does not default to true to catch that mistake earlier? Or should it be documented like mentioned here: fix: Boolean binding to some FAST control attributes does not behave as expected #5320 (comment)

[chrisdholt]

@chrisdholt @EisenbergEffect In the fast-element-2 branch it looks like there are a couple more instances of a boolean attribute defaulting to true:

Another in the dialog (trapFocus):

• https://github.com/microsoft/fast/blob/features/fast-element-2/packages/web-components/fast-foundation/src/dialog/dialog.ts#L51

And a couple others components:

• https://github.com/microsoft/fast/blob/features/fast-element-2/packages/web-components/fast-foundation/src/picker/picker.ts#L117
• https://github.com/microsoft/fast/blob/features/fast-element-2/packages/web-components/fast-foundation/src/picker/picker.ts#L127
• https://github.com/microsoft/fast/blob/features/fast-element-2/packages/web-components/fast-foundation/src/tabs/tabs.ts#L150

1. Will those be updated to default to false as well?
2. Is there a type change / or runtime check for the decorator to assert that the value does not default to true to catch that mistake earlier? Or should it be documented like mentioned here: fix: Boolean binding to some FAST control attributes does not behave as expected #5320 (comment)

I'll take a look - trap focus seems like it's going to be a difficult one to figure out into false in scenarios where it should "just work". I'll know more about the others when I look.

[rajsite]

I'll take a look - trap focus seems like it's going to be a difficult one to figure out into false in scenarios where it should "just work". I'll know more about the others when I look.

Yea in those scenarios we have inverted the name, ie I could see changing "trap-focus" => "avoid-trapping" so by default it traps.

[EisenbergEffect]

We should definitely fix this so that all boolean attributes default to false.

[chrisdholt]

We should definitely fix this so that all boolean attributes default to false.

Boolean fix for dialog trapFocus is linked above.

[chrisdholt]

We should definitely fix this so that all boolean attributes default to false.

Tabs is up at the link above

[rajsite]

@chrisdholt @EisenbergEffect These updates are great! In the vein of potentially breaking changes making it in for FE2 do you think getting noImplicitAny and strictPropertyInitialization can also be targeted for that branch? #5338

It's been a recurring issue for us that components property types don't align with their values, usually because they are not initialized and can be undefined, ie https://github.com/microsoft/fast/blob/master/packages/web-components/fast-foundation/src/tab/tab.ts#L19

[chrisdholt]

@chrisdholt @EisenbergEffect These updates are great! In the vein of potentially breaking changes making it in for FE2 do you think getting noImplicitAny and strictPropertyInitialization can also be targeted for that branch? #5338

It's been a recurring issue for us that components property types don't align with their values, usually because they are not initialized and can be undefined, ie https://github.com/microsoft/fast/blob/master/packages/web-components/fast-foundation/src/tab/tab.ts#L19

We'll be making some "progressive" breaks as part of the beta. I'll look for low hanging fruit to see if we could sneak it, but it's coming in hot for Thursday release day.

[chrisdholt]

@rajsite I think perhaps one solution we could do for an initial pass here would be - @EisenbergEffect?

public foo: boolean;

public foo?: boolean;

[rajsite]

@rajsite I think perhaps one solution we could do for an initial pass here would be - @EisenbergEffect?

public foo: boolean;

public foo?: boolean;

For attribute-bound booleans I think it's pretty safe to initialize them to false (since that does not change the attribute value in the DOM).

For other types (string, number) making them optional would be a good first step, as the type definition then better aligns with the actual value when accessed at runtime.

[chrisdholt]

@rajsite I think perhaps one solution we could do for an initial pass here would be - @EisenbergEffect?

public foo: boolean;

public foo?: boolean;

For attribute-bound booleans I think it's pretty safe to initialize them to false (since that does not change the attribute value in the DOM).

For other types (string, number) making them optional would be a good first step, as the type definition then better aligns with the actual value when accessed at runtime.

Agree here - my biggest concern is string initialization, etc and typically because those often aren't actually required, where a boolean should almost always have an initial value.

[rajsite]

Agree here - my biggest concern is string initialization, etc and typically because those often aren't actually required, where a boolean should almost always have an initial value.

Makes sense. My ultimate end goal is getting as close as possible to API compatibility with native elements for our fast-foundation based elements. Or not achieving that, having strict types to help our devs out.

So for some properties like the type on a vanilla button with html <button></button> returns 'submit' while the type on a fast foundation based button returns undefined for the html template <fast-button></fast-button> which doesn't align with expectations or specs (assuming we want to get to api compatibility).

But definitely agree that it takes case-by-case consideration and a good step for string types is marking them optional so in our code bases which are configured for strict type checking we are at least checked by the TS compiler. Then follow-ups can be done to initialize where it makes sense like button type, etc.

[chrisdholt]

Agree here - my biggest concern is string initialization, etc and typically because those often aren't actually required, where a boolean should almost always have an initial value.

Makes sense. My ultimate end goal is getting as close as possible to API compatibility with native elements for our fast-foundation based elements. Or not achieving that, having strict types to help our devs out.

So for some properties like the type on a vanilla button with html <button></button> returns 'submit' while the type on a fast foundation based button returns undefined for the html template <fast-button></fast-button> which doesn't align with expectations or specs (assuming we want to get to api compatibility).

But definitely agree that it takes case-by-case consideration and a good step for string types is marking them optional so in our code bases which are configured for strict type checking we are at least checked by the TS compiler. Then follow-ups can be done to initialize where it makes sense like button type, etc.

I'm going to go and try and set these for booleans, but I don't think we can turn it off completely. I just ran locally and immediately hit an issue with id in Accordion Item - I think because the HTMLElement interface is set to string and not string | undefined/null. So, the best we can probably do for now is set the boolean values.

[chrisdholt]

@EisenbergEffect @rajsite PR is here for setting default values for boolean attrs: #6023

[rajsite]

@chrisdholt do you know the state of #6023? It seems like it was closed a couple weeks ago but I'm having trouble finding a discussion on the outcome.

[EisenbergEffect]

I believe that the outcome wast to always make default boolean values false so that this issue doesn't happen. However, that's a breaking change so it would need to come in our upcoming major version. @chrisdholt can correct me if I'm wrong.

[chrisdholt]

@chrisdholt do you know the state of #6023? It seems like it was closed a couple weeks ago but I'm having trouble finding a discussion on the outcome.

Yeah I closed this as it was so far out of date. It should be light work to be honest as these are all "false", I just need to get a branch setup. In some cases, code will likely need to be changed to ensure that some of these which bind to ARIA attributes don't add ARIA where it's not expected (there are a few of these cases).

We did the work I believe in Foundation already to remove all defaulting to true, so the values all should default to undefined right now. We can certainly default to false, but requires what's noted above to be "done".

[rajsite]

Makes sense to be part of ffv3 as a breaking change, thanks!