Add option to allow HTML in labels

Before we manually rebase the code again and again, I created an issue fork and applied patch #55 by @smustgrave, for automatic rebasing and easier in Browser code reviews.

Thank you @Grevil!

So finally I think this needs test copies with
'fieldset_label_html' => TRUE,
and the expected results and checking #57.

If possible #57 should simply also be covered by a test, so we can ensure it works as expected.

Could not reproduce behaviour stated in #58, I tested the patch and it worked for (mostly) all field group widgets!

The icon was displayed cleanly for Details, Tab, Details Sitebar, Accordion Item and Fieldset. I don't have too much experience with the module, so I was unable to properly use the "Accordion" and "Tabs" Widget types. I am also unsure, if it should work with "HTML Element" as the icon will get wrapped inside other incompatible elements, leading to break the icon.

Somebody with a bit more knowledge about this module should decide if the option should also exist for "Accordion", "Tabs" and "HTML Element".

Here are screenshots, testing the "Tab" Widget:
Form Display Settings:

Form:

As of #64, this still needs automated tests.

Thanks @Grevil, perfect! :)

Somebody with a bit more knowledge about this module should decide if the option should also exist for "Accordion", "Tabs" and "HTML Element".

Thanks for the question, no that wouldn't make sense, as they are only wrappers with no label. HTML Element is special, I'd vote not to touch it here. It's a type itself and has special handling.

So only tests left to get this in! Whao!!

// By default tabs don't have titles but you can override it in the theme.

Just found that in code, I guess we shouldn't touch that definition either then.

OK, I could replicate the behaviour of #58 now! It seems to be specific to multiple "Tab" Elements being rendered inside a "Tabs" Element. And ONLY if the Tabs are rendered vertically. horizontally works as expected.

Alright, the issue with the vertically rendered tabs is an upstream core issue, which we currently can't do anything about it. The core "vertical-tabs.js" uses "textContent" to render the tab labels instead of "innerHTML".

Element.innerHTML returns HTML, as its name indicates. Sometimes people use innerHTML to retrieve or write text inside an element, but textContent has better performance because its value is not parsed as HTML.

See https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent?retire...

Additionally, the HTML gets escaped somewhere else (but that should be resolved in the core issue).

I will create a follow-up issue for this and link the core issue once created.

Here is a screenshot, where the tabs are rendered as details elements with the correct title for a split second:

And here the final rendered vertical tabs render element with the escaped and removed HTML in the title:

So only tests remain here!

Alright, I added appropriate tests, please review!

WHAO! So happy to see the progress here and the required tests being added!

I just reviewed the code, the tests and implementation LGTM!
I left some final comments, most of them are just around wording and it would be nice, if a native speaker (EN) could pick these up.

@Grevil: Perhaps you'd like to have a final look at the label-else-part comment and try if it works without the additional handling there?

Once these minor comments are solved, RTBC from my side! Yay! Thank you so much @Grevil!

@Anybody:

I guess the additional handling isn't needed in the ELSE part. That could just stay as before? Drupal / Twig will do the escaping? I couldn't find a place where we're changing the output logic, so it should still work?

The "Markup::create()" was originally suggested in #12, but it was just a workaround for "&" being escaped because of the "Html::escape" in the original patch 6.

I guess, before this patch, A lot of HTML elements were actually allowed, BUT not for horizontal tabs. Markup::create(Html::escape($this->getLabel())); will ensure, no HTML is allowed, when the checkbox isn't checked.

Re #74 thanks for the explanation @Grevil! That makes sense, so let's keep this fix for #12 to also fix such potential double escapings. I don't think it will do any harm.

OK, let's see how the test turn out!

All green!

Wohoo! RTBC! :)

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