Problem/Motivation
Webform allows the addition of custom CSS classes to various webform elements. When you want to add multiple CSS classes to an element you can separate them by spacing. The problem is that these multiple CSS classes will be added to an element attributes class array item as a single string. This is a problem if later in custom preprocessors or templates you want to use attribute.hasClass() or similar methods - this will not work because you can not check for a single CSS class.
Proposed resolution
If possible all CSS classes strings should be converted to the array before adding to element attributes class array item.
Remaining tasks
- Evaluate solution with webform actions elements.
- Apply solution to the rest of elements.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#10 | Screen Shot 2021-06-01 at 8.33.54 AM.png | 264.17 KB | jrockowitz |
#5 | 3216275-5.patch | 3.39 KB | jrockowitz |
| |||
#4 | 3216275-4.patch | 4.65 KB | jrockowitz |
| |||
#2 | add-css-classes-as-array-items-3216275-2.patch | 826 bytes | pivica |
|
Comments
Comment #2
pivica CreditAttribution: pivica at MD Systems GmbH commentedHere is a first patch that is doing a proposed change to webform actions elements. We are exploding the CSS class attribute value and then filter it to remove any duplicated spaces.
Comment #3
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedI completely agree with the goal of this ticket.
Maybe a universal utility to merge classes could help and account for when the class is an array of string or a simple string.
The new API could be \Drupal\webform\Utility\WebformHtmlHelper::mergeClass or \Drupal\webform\Utility\WebformClassHelper::merge;
It is tempting to go after making it easier to merge attributes into an element.
BTW, with a quick search for
['#attributes']['class']
(with a space to the right). I am only seeing this issue in \Drupal\webform\Element\WebformActionsComment #4
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedI was reviewing the patch and realized that the source of the problem is in the \Drupal\webform\Element\WebformElementAttributes element.
The real problem is the select other and plain text class names should be split and stored as an array of class names.
Does this solution make sense?
For existing implementations, class names will only be split after an element is updated. We probably need to create a change record to communicate this to users who will see their custom classes being split.
BTW, the attached patch might break some existing tests which will need to be fixed.
Comment #5
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedHere is a slightly simpler patch.
Comment #6
pivica CreditAttribution: pivica at MD Systems GmbH commented> The real problem is the select other and plain text class names should be split and stored as an array of class names.
> Does this solution make sense?
Yes, it makes sense and it is a better approach for fixing this, however, this will also change the storage format and maybe it is a problem for existing sites? Do we need an update function for existing sites?
Comment #7
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedWe could have to scan every single element and setting looking for class attributes and then splitting the last comma-delimited item. I am not sure this is feasible.
Comment #8
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedI think we can't write an update hook that splits the other class into individual classes because it is possible that the default classes for an element or a setting contain a class name that contains a space. For example, some of the element wrapper classes contain spaces like
messages messages--error
.Still, I think splitting the 'other' or plain-text class name makes a lot of sense.
Comment #9
pivica CreditAttribution: pivica at MD Systems GmbH commented> For example, some of the element wrapper classes contain spaces like messages messages--error.
I didn't quite understand why we can not split `messages messages--error` wrapper case, this are still two CSS classes, any reason why this can't be array also?
Comment #10
jrockowitz CreditAttribution: jrockowitz as a volunteer and at Webform module Open Collective, The Big Blue House commentedWe don't want to split
messages messages--error
because it is one of the default CSS options.@see /admin/structure/webform/config/elements
Comment #11
pivica CreditAttribution: pivica at MD Systems GmbH commented> We don't want to split messages messages--error because it is one of the default CSS options.
Ah, I see, yeah that would be tricky. Thank you for explaining it in more details.
Comment #12
jrockowitz CreditAttribution: jrockowitz as a volunteer and at Webform module Open Collective, The Big Blue House commentedI really hate the idea of putting off committing this ticket till Webform 7.x, which I am not sure when it will be released.
The only situation where this patch causes regression is when someone is parsing the last class assuming it is the 'other' value
Here is the before.
Here is the after.
Comment #13
jrockowitz CreditAttribution: jrockowitz as a volunteer and at Webform module Open Collective, The Big Blue House commentedI am postponing this change until Webform 6.1.x. I don't think it is possible to write an update hook.
Comment #15
jrockowitz CreditAttribution: jrockowitz as a volunteer and at Webform module Open Collective, The Big Blue House commented