WIP: Feedback for the new Form system

[micgro42]

While creating the farmer plugin with the new Form system, this is were I'm collecting my feedback to the new form system. This will most likely be extended over the next couple of days.

• It would be helpful if there were an Form::addLabel() method which utilizes the for-attribute as opposed to the current approach of placing the input into the label. The current approach is suboptimal because:
  • Bootstrap cannot handle it properly
  • a class added with addClass('class') is added both to the label and the input field
  • also see Form 2 #1265
• Form::addCheckbox and Form::addRadioButton actually add Textinputs
• addPasswordInput shows the input as clear text and not as bullets/stars/whatever
• Buttons (e.g. Submit, Reset, etc.) are completely missing?

[splitbrain]

I fixed the missing element types and added support for buttons. I also added a addLabel() method. You now can omit the label from the Input-Element and manage labels yourself.

It would still desirable to change how labels are generated automatically some day (eg. by autogenerating IDs).

[micgro42]

• <select> fields are missing. A way to add <option> tags to those fields would be useful.

[micgro42]

• If an attribute is set to an empty string via $option->attr('value','') the attribute is actually added without assigning it any value, eg: <option value> instead of the expected <option value="">

[micgro42]

• nested <fieldset></fieldset> are broken

[micgro42]

• It would be great if Form::findPositionByAttribute etc. would be somehow iterable. So that I can access all elements with a certain class or name or all disabled elements.

Maybe something like Form::findAllPositionsByAttribute which returns an array with all matching positions would be a good implementation.

Edit 2016-04-15: The approach suggest by @splitbrain below solves this:

$pos = -1;
while($pos = $form->findPositionByType('foo', $pos+1) !== false) {
   // do something with $pos;
}

[micgro42]

• TagCloseElement should correctly return '' on $closedTag->attr(null) and only throw an exception if that function is called with a non-null parameter.

[micgro42]

I have written a small function that attaches a class and error-messages to form elements based on their name. Not sure if it would be useful/general enough to be integrated into the new class system?

public function addErrorsToForm(\dokuwiki\Form\Form &$form, $errorArray) {
        for ($position = 0; $position < $form->elementCount(); ++$position) {
            if ($form->getElementAt($position) instanceof dokuwiki\Form\TagCloseElement) {
                continue;
            }
            if ($form->getElementAt($position)->attr('name') == '') continue;
            $elementName = $form->getElementAt($position)->attr('name');
            if (!isset($errorArray[$elementName])) continue;
            $form->getElementAt($position)->addClass('error');
            $form->addTagOpen('div',$position+1)->addClass('error');
            $form->addHTML($errorArray[$elementName],$position+2);
            $form->addTagClose('div',$position+3);
        }
    }

[mightymt]

I just came across a small problem with the form system and thought I'd add it here instead of opening a new issue.

In form_menufield the size attribute for the select tag is explicitly set to "1". However it would be useful if the value was taken from the passed $attrs array and "1" was only used as a fallback/default.

[splitbrain]

@micgro42 Re Form::findAllPositionsByAttribute something like the following should work:

$pos = -1;
while($pos = $form->findPositionByType('foo', $pos+1) !== false) {
   // do something with $pos;
}

Is that good enough or would adding a dedicated function still be worth it?

[splitbrain]

@micgro42 re the empty attributes. I just checked. Empty attributes are correctly added to the html source as attribute="". It's probably your Chrome/Firefox inspector "cleaning up" the source when displaying it.

[splitbrain]

@micgro42 regarding the fieldsets. Our old form mechanism did not support nested fieldsets. Opening a new fieldset closed any previous fieldset (similar to what we do in bureaucracy). Form::balanceFieldsets() currently reproduces this behavior. We could change that of course. The question is: should we?

[micgro42]

@splitbrain
Re: Iterating over the attributes: the way you suggested is good enough for me.

Re: Fieldsets: I am not passionate about the fieldsets, though I do think that nested fieldsets can make sense when one is semantically structuring more complex forms. However if it would be a significant change to the code or the dokuwiki look&feel, then the current version is ok as well. We can always add (pseudo-)fieldsets with addHTML() or addTagOpen and CSS, should we really need them.

[micgro42]

• Bug: In <textarea> fields linebreaks are duplicated on submission failure. Tested in Firefox and Chromium (Should I open a new ticket for this?)

[micgro42]

It appears that linebreaks in a textarea are \r\n which dokuwiki converts into \n\n, hence duplicating the linebreak.

[splitbrain]

fixed it. next time a new ticket would make sense.