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.

CommentFileSizeAuthor
#10 Screen Shot 2021-06-01 at 8.33.54 AM.png264.17 KBjrockowitz
#5 3216275-5.patch3.39 KBjrockowitz
#4 3216275-4.patch4.65 KBjrockowitz
#2 add-css-classes-as-array-items-3216275-2.patch826 bytespivica

Comments

pivica created an issue. See original summary.

pivica’s picture

pivica commented
Status: Active » Needs review
StatusFileSize
new add-css-classes-as-array-items-3216275-2.patch826 bytes

Here 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.

jrockowitz’s picture

jrockowitz commented

I 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\WebformActions

jrockowitz’s picture

jrockowitz commented
StatusFileSize
new 3216275-4.patch4.65 KB

I 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.

jrockowitz’s picture

jrockowitz commented
StatusFileSize
new 3216275-5.patch3.39 KB

Here is a slightly simpler patch.

pivica’s picture

pivica 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?

jrockowitz’s picture

jrockowitz commented

We 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.

jrockowitz’s picture

jrockowitz commented

I 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.

pivica’s picture

pivica 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?

jrockowitz’s picture

jrockowitz commented
StatusFileSize
new Screen Shot 2021-06-01 at 8.33.54 AM.png264.17 KB

We don't want to split messages messages--error because it is one of the default CSS options.
@see /admin/structure/webform/config/elements

pivica’s picture

pivica 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.

jrockowitz’s picture

jrockowitz commented

I 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.

class:
  - default
  - custom-one custom-two

Here is the after.

class:
  - default
  - custom-one 
  - custom-two
jrockowitz’s picture

jrockowitz commented
Title: Element custom CSS classes should be added as array items » [Webform 6.1.x] Element custom CSS classes should be added as array items
Version: 8.x-5.x-dev » 6.x-dev
Status: Needs review » Postponed

I am postponing this change until Webform 6.1.x. I don't think it is possible to write an update hook.

  • jrockowitz authored 5c1f05b on 6.x 
    Issue #3216275 by jrockowitz, pivica: [Webform 6.1.x] Element custom CSS...
jrockowitz’s picture

jrockowitz commented
Status: Postponed » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.