Addition of type="button" to directives

[ghost]

uifDialog, uifPeoplePicker, uifFacePile all use <button> elements, but do not set the type attribute to button.

The issue can be overcome with uifButton by adding the attribute to the markup, this is right as we are leaving the behavior to the consumer. However, for the directives above, we do not want a postback to occur - so must add the attribute.

Expected Behavior

Clicking buttons on one of the above directives should not issue a postback where the directives are within a <form> tag

Actual Behavior

Postback occurs due to the lack of type attribute

@andrewconnell - Raising this as an issue formally as having thought about it, I believe the correct thing to do is add type="button to any directives where we need to control the behavior of the buttons e.g. clicking the 'add' button in FacePile.

• n.b. uifPanel, uifCallout and uifSearchBox already use type="button"

[andrewconnell]

OK... let's make sure we get more eyes on this. I think I'm in agreement with you, but implementing this could have some significant impacts so just want more eyes on it. What say you @ngOfficeUIFabric/ngofficeuifabric-members ?

[newq]

The only significant impact would be that uifPeoplePicker etc. would be finally useable in a form. Please fix this issue.

[andrewconnell]

But in the case of that one directive, couldn’t that directive be updated to add type=button to the button it generates?

[newq]

Personally it would help me if it got added just for the uifPeoplePicker directive. Still the type=button should be added to those kinds of buttons in other directives as otherwise it causes troubles with how form submission works in Angular.

[andrewconnell]

Wait @newq... <uif-button> isn't even used in the <uif-peoplePicker>... it's only used in <uif-callout> & <uif-messageBanner> directives.

We're crossing the streams in this issue... let's stay focused on just adding type="button" to directives that use the <button> tag.

So let's start this over... any reason why we shouldn't add this to those directives mentioned in @tobiaswest83 's original post for this issue?

[ghost]

Hmm, @andrewconnell - my list was created by looking for directives which use <button>, not <uif-button>. In both cases a consumer of the library would get issues with using these directives in a <form>

The list of directives with some kind of <button> are now below, the fix in both cases is the same...

• uifDialog (uses <button>)
• uifPeoplePicker (uses <button>)
• uifFacePile (uses <button>)
• uifCallout (uses <uif-button>)
• uifMessageBanner (uses <uif-button>)

Love the Ghostbusters reference though :laughing:

[newq]

@andrewconnell Okay, I'm sorry. I might be confused on what exactly this issue is supposed to be discussing. Adding type=button to button or to uif-button. For now I'm just strongly in favor of adding it to button elements used in directives like the people picker.

[andrewconnell]

Thinking about this more... we've got a few different things to think about... I'm going to cross the streams again because I think it's all part of the same discussion now. Let's start with an agreement that something needs to be addressed... we just need to figure out how.

FWIW, I looked at Angular Material... but it doesn't apply to us. They don't use element directives, they use attribute (CSS class) directives for their buttons... so the developer is always creating a <button> and including type=button themselves.

Should uif-button Always Render type=button?

Right now it doesn't, but there are plenty of cases where it should. The challenge is if we change this default, it would force it on everyone. However, we should add some sort of failsafe where you can exclude the type attribute from being added... something like <uif-button uif-isTypeExcluded=true></> which would default to false.

Other ideas for the attribute name:

• isTypeExcluded (default=false)
• typeAttrExcluded (default=false)
• isButtonTypeExcluded (default=false)
• isButtonTypeAttrExcluded (default=false)
• includeButtonTypeAttr (default=true)
• includeTypeAttr (default=true)

I vote to do this... also vote for isButtonTypeAttrExcluded... just hate how long it is

Should Directives with Buttons use uif-button?

Right now there's a mix... as @tobiaswest83 said in his comment above, two directives use uif-button and three use button. If we updated all of them to use uif-button & did the suggestion above (default rendered buttons to include type=button, it would fix the issue.

However, what about cases where you don't want that type included? You'd have to add the same new attribute used on the button (hence when picking the attribute name above, I think button should be in the attribute to be clear on directives that inherit from it, what it will be doing.

I vote for this.

Thought?

[newq]

In my opinion uifButton could have a type attribute that's patched through to the actual button that is created. Then directives like the uifPeoplePicker could just use a uifButton where type=button is set in the directives template and uifButton would behave like a button would be expected to behave.

[ghost]

Thoughts here @andrewconnell

Should Directives with Buttons use uif-button?

I don't think we should only be using uif-button. Taking uif-panel as an example, the markup for the close button is quite divorced from what uif-button renders and would break stylings...plunk to follow laterif need be.

Should uif-button Always Render type=button?

No, it shouldn't and actually for the reasons above (that using button and uif-button are both valid) i don't think we should make any change to uif-button. Consumers of uif-button should be able to add type="button" without any extra legwork (as they do already), and all of the directives that use uif-button within ngOfficeUIFabric should have type="button" added to them as a matter of course, and I believe this would not have any impact on consumers of the library, since we don't want forms to submit on any of the actions in directives listed above.

[waldekmastykarz]

If we want to keep flexibility what if uifButton would by default render type="button". In cases you wanted to use a different value all you would have to do is to specify your own value explicitly, so:

<uif-button />  --> <button type="button">

<uif-button type="submit"> --> <button type="submit">

The only scenario this wouldn't support, is if you wanted to avoid rendering the type attribute altogether but that's error prone as the result depends pretty much on the surrounding markup - exactly what led to this discussion 😄

[ghost]

yo @waldekmastykarz, not sure we're ready to close this one. We're all in agreement that we shouldn't add type="button" to uifButton, this is more about the inclusion of that attribute in directives which use uifButton

edit: you reopened it anyway :smile:

[waldekmastykarz]

yeah, closed it by accident. my bad 😊

[waldekmastykarz]

@tobiaswest83 if, as you said, all directives that use uif-button should also include type="button" why not include it ourselves as a safe default if no type is explicitly specified and simplify the markup?

[ghost]

@waldekmastykarz, that approach would work and gets my vote. We'd still need to fix non uifButtons but there are only a handful

[andrewconnell]

Resolution based on this discussion - no changes to uif-button, but anywhere it's used within directives, it should include a type="button". So... we need to add issues for each directive that needs this change. Closing this issue as work should​ continue on those...

[andrewconnell]

Resolution: this now