Inputs with col-XX generate invalid Bootstrap markup

[ggam]

With the following XHTML:

<b:inputText value="test" col-xs="6" label="Message" label-col-xs="6"/>

I'd expect it to generate something like:

<div class="form-group">
    <label for="message1" class="col-xs-6">Message</label>
    <div class="col-xs-6">
        <input type="text" class="form-control" id="message1"></input>
    </div>
</div>

But it's generating the following instead:

<div id="j_idt6:j_idt7" class="form-group ">
    <label for="input_j_idt6:j_idt7" class=" col-xs-6 control-label">Message</label>
    <input id="input_j_idt6:j_idt7" name="input_j_idt6:j_idt7" class="form-control" value="test" type="text"></div>

You can see that only the label has col-xs-X defined.

If I only set the size for the input, like <b:inputText value="test" col-xs="6" label="Message" />, I get the following:

<div class="col-xs-6" id="j_idt6:j_idt7">
    <div class="form-group ">
        <label for="input_j_idt6:j_idt7" class=" control-label">Message</label>
        <input id="input_j_idt6:j_idt7" name="input_j_idt6:j_idt7" class="form-control" value="test" type="text">
    </div>
</div>

form-group acts like row for forms, so it doesn't seem to be correct to wrap the row with a column. Also, the input size shouldn't be ignored when setting the label width. This is needed in order to allow for more complex form designs (such as https://jsbin.com/jikiyuwuve/edit?html,output). Although more customization options will be needed for that. I'll open an issue for that later.

[stephanrauh]

The markup is wrong because you didn't add <form horizontal="true">. (Or at least I hope so because it was so hard and painful to get it right :)).

Have a look at http://www3.bootsfaces.net/Showcase/forms/horizontalForms.jsf for more details.

Should we emit an error message, or should we silently correct the html code the way you suggest? In that case, I'd like to postpone it to BootsFaces 1.2 because it's a breaking change and because it's very difficult to get it right. There are so many options (AJAX vs. non-AJAX, facets, horizontal forms, stacked forms, inline forms, and last not least input fields without the col-* attributes).

[ggam]

Hi, You are right, he form class is effectively needed for added styles. But anyway, there are two problems with the markup not complaining with BS rules (at least to my understanding): - We are generating a column around a form-group. Since the form-group acts as a row, columns must be always inside it. This is needed for cases when two inputs are to be placed on the same line (while using col-xx). From what I see on the internet, that column would be a div wrapping the input. - When the label size is specified, only it has the col-xx attribute. That makes the input go full length without any option to place more inputs on the same row (form-group). Yes, it's a breaking change. But it depends on how you look at it. For me, if it generates wrong Bootstrap markup, that's a priority to fix. And any behaviour depending on "wrong" markup can't be taken for granted. If not breaking BC in this cases is really a concern, I'd propose to use a context param to revert to the original markup. But that's also more maintenance work... A middleground aproach (given the times wr have) would be to add an option to inputs to just generate the input, without any added form-group markup, leaving the user the option to customize as needed. But, for that case, I was going to propose the addition of a new b:formGroup component that generates just the form group div and prevents its generation for inputs that were direct children. In other words, inputs would check if their parent is a b:formGroup and in that case, they would not generate the form-group div. That would allow for multi input rows without custom html and just using BS styles. Not sure however how hard could it be. El jue., 6 de abril de 2017 20:30, Stephan Rauh <notifications@github.com> escribió:

…

The markup is wrong because you didn't add <form horizontal="true">. (Or at least I hope so because it was so hard and painful to get it right :)). Have a look at http://www3.bootsfaces.net/Showcase/forms/horizontalForms.jsf for more details. Should we emit an error message, or should we silently correct the html code the way you suggest? In that case, I'd like to postpone it to BootsFaces 1.2 because it's a breaking change and because it's very difficult to get it right. There are so many options (AJAX vs. non-AJAX, facets, horizontal forms, stacked forms, inline forms, and last not least input fields without the col-* attributes). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#682 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACAucAj8TQmNdYmOtdKoLuqpbyhku4i6ks5rtS9VgaJpZM4M1w1Q> .

[stephanrauh]

Let's think about the good solution first. If we find out we don't have enough time, we can still opt for the quick-and-dirty solution.

According to StackOverflow and my own test form-group only acts as a row if place within a horizontal-form. I don't know if that changes anything (probably not), but let's keep that in mind.

Basically, the simplest bugfix that comes to mind is to switch the form-group div and the col-* div. I think we can do that at low risk.

What happens if the form-group happens to be within a b:row? Probably not much, but we should check that, too.

I also agree with your proposal to add a b:formGroup tag. The current solution is too inflexible. b:formGroup sounds like a welcome addition.

Would you like to prepare a pull request?

[stephanrauh]

Components to modify and test (please add components if something's missing!):

Regular (stacked) forms:

• b:inputText
• b:inputText with appending / prepending facet
• b:colorPicker
• b:colorPicker with appending / prepending facet
• b:selectOneMenu
• b:selectOneMenu with appending / prepending facet
• b:selectMultiMenu
• b:selectMultiMenu with appending / prepending facet
• b:datepicker
• b:datepicker with appending / prepending facet
• b:dateTimePicker
• b:dateTimePicker with appending / prepending facet

Inline forms:

• b:inputText
• b:inputText with appending / prepending facet
• b:colorPicker
• b:colorPicker with appending / prepending facet
• b:selectOneMenu
• b:selectOneMenu with appending / prepending facet
• b:selectMultiMenu
• b:selectMultiMenu with appending / prepending facet
• b:datepicker
• b:datepicker with appending / prepending facet
• b:dateTimePicker
• b:dateTimePicker with appending / prepending facet

Horizontal forms:

• b:inputText
• b:inputText with appending / prepending facet
• b:colorPicker
• b:colorPicker with appending / prepending facet
• b:selectOneMenu
• b:selectOneMenu with appending / prepending facet
• b:selectMultiMenu
• b:selectMultiMenu with appending / prepending facet
• b:datepicker
• b:datepicker with appending / prepending facet
• b:dateTimePicker
• b:dateTimePicker with appending / prepending facet

Components wrapped inside a <b:formGroup> tag:

• b:inputText
• b:inputText with appending / prepending facet
• b:colorPicker
• b:colorPicker with appending / prepending facet
• b:selectOneMenu
• b:selectOneMenu with appending / prepending facet
• b:selectMultiMenu
• b:selectMultiMenu with appending / prepending facet
• b:datepicker
• b:datepicker with appending / prepending facet
• b:dateTimePicker
• b:dateTimePicker with appending / prepending facet

[ggam]

@stephanrauh Exactly, the solution would be to:

• Basically switch the form-group and col-xs-X position
• Render the col-X for the input even when the label already has a size.

I doubt nobody is relying on the current markup so I think we can safely change it.

I'll try to make a PR for the b:formGroup component over the weekend. Doesn't seem to be difficult. I'll ask for help if I need it.

[stephanrauh]

This bug is also related to #614 and #493.

@NicolaIsotta Please have a look at PR #685. @ggam currently implements a <b:formGroup> widget. Does this solve your issue #493, too?

[stephanrauh]

ToDos left:

• testing
• documenting <b:formGroup>

[NicolaIsotta]

@stephanrauh I guess this fixes #493
But, @ggam , I'm afraid this breaks #621 : b:formGroup should retrieve error style classes from his children.

[ggam]

Nice catch! If we put the has-error class on the new form-group position, that will "invalidate the whole form. I think that putting the has-error class on a div wrapping the input suffices to solve it. So we can have: Div form group Div has error Input /div Input (valid and on the same group) /div I'll try it now and make a PR El sáb., 8 de abril de 2017 22:07, NicolaIsotta <notifications@github.com> escribió:

…

@stephanrauh <https://github.com/stephanrauh> I guess this fixes #493 <#493> But, @ggam <https://github.com/ggam> , I'm afraid this breaks #621 <#621>. b:formGroup should retrieve error style classes from his children. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#682 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACAucE8knwMbrIgXCPpmqCC0yVmrLgypks5rt-j7gaJpZM4M1w1Q> .

[stephanrauh]

These components have become so powerful, improving them always feels like playing Mikado :).

@ggam Thanks for offering a new PR!

[ggam]

@NicolaIsotta I've started to implement a solution on #689

[ggam]

@NicolaIsotta plese have a look at #689. I think I've fixed it now.

[NicolaIsotta]

It seems ok at first glance. I'll test it once it's merged.

[stephanrauh]

I've merged the PR. Happy testing!

[stephanrauh]

I'll close this issue for the moment. I keep listening to the comments. If bugs occur during testing, I'm ready to re-open the issue.