search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Invalid markup if field prefix or suffix contain html tags #5735

Open indigoxela opened 2 years ago

indigoxela commented 2 years ago

Description of the bug

In an otherwise unrelated issue we discovered a flaw with field prefix and suffix ending up with invalid markup.

Steps To Reproduce

Just as an example: the image field settings form:

  1. Go to admin/structure/types/manage/card/fields/field_image
  2. Switch to source display
  3. Check for the markup around id edit-instance-settings-max-dimensions

Actual behavior

HTML tags span / div nested in an invalid way.

broken-markup

Expected behavior

Valid markup.

Additional information

This must be around for quite some time, but we didn't realize so far.

Another example is the Field UI global settings (cardinality). I'm pretty sure that there are even more.

klonos commented 2 years ago

@indigoxela rehashing here what I said in https://github.com/backdrop/backdrop-issues/issues/1349#issuecomment-1221609901

...the issue with the invalid markup could be just a matter of using #prefix vs. #field_prefix and #suffix vs. #field_suffix properly. I often forget that #field_prefix/#field_suffix things even exist and use#prefix/#suffix most of the time (if not always). So chances are that others make the same mistake + then people that review code also miss it.

Perhaps we need a best-practice policy, where we discourage using markup in #prefix/#suffix and use the respective #field_* variants for that instead(?) ...or the other way around 🤷🏼

klonos commented 2 years ago

...added the "documentation" label here, so that we may update https://docs.backdropcms.org/form_api with some notes.

klonos commented 2 years ago

...as it turns out, #field_prefix and #field_suffix are already (always) wrapped in <span> tags. See theme_form_element():

  $prefix = isset($element['#field_prefix']) ? '<span class="field-prefix">' . $element['#field_prefix'] . '</span> ' : '';
  $suffix = isset($element['#field_suffix']) ? ' <span class="field-suffix">' . $element['#field_suffix'] . '</span>' : '';

This is definitely something worth mentioning in the Form API documentation!

Another thing worth documenting is that there apparently is no easy way to add classes/attributes to the <span> tags of #field_prefix and #field_suffix 🤔

indigoxela commented 2 years ago

...as it turns out, #field_prefix and #field_suffix are already (always) wrapped in tags

Right, I wasn't completely aware of that, too. And probably did it also wrong (in contrib or custom code).

So IMO the first step is to fix how core uses the field prefix and suffix. My suspicion: there's a reason why some core fields do it that way. Probably there was no other solution for the problem to solve?

And of course, the documentation should be clearer about what prefix or suffix are supposed to contain (and what not).

klonos commented 2 years ago

...documentation should be clearer about what prefix or suffix are supposed to contain (and what not).

Agreed 👍🏼 ...actually, there are some hints currently. In the Form API documentation #prefix/#suffix are described as:

Text or markup to include before/after the form element.

...whereas for #field_prefix/#field_suffix the respective description doesn't mention markup:

Text or code that is placed directly in front of/after a field.

It seems that markup is not explicitly mentioned for the #field_* properties.

klonos commented 2 years ago

...odd how #prefix/#suffix are listed in https://www.drupal.org/docs/drupal-apis/form-api/form-render-elements, but #field_prefix/#field_suffix aren't. Were these introduced later in D8 (and Backdrop) but not in D7 or something?

Anyway, here's some further descriptions for those that we should add in our documentation:

(string) Prefix/Suffix to display before/after the HTML input element. Should be translated, normally. If it is not already wrapped in a safe markup object, will be filtered for XSS safety.

bugfolder commented 2 years ago

I added some commentary to https://docs.backdropcms.org/form_api#field_prefix, also https://docs.backdropcms.org/form_api#prefix. Further suggestions/requests solicited, of course.

argiepiano commented 2 years ago

Thanks @bugfoder. The text seems very clear to me. Check a typo - "$suffix" instead of "#suffix".

bugfolder commented 2 years ago

Typo fixed.

indigoxela commented 2 years ago

I did a quick check: we inherited this markup mess from Drupal, which has the same problem.

Markup as wrapper for items using field_prefix and field_suffix is used in several places in core:

My suspicion: it's done that way because there's no other way to achieve the desired result.

So IMO we should:

  1. Verify if there are alternative for field_prefix/field_suffix for these cases
  2. If so, implement these alternatives in core
  3. If not, figure out what's missing in core to achieve these results
  4. If there's no alternative yet, either provide one or drop the automatic span around prefix/suffix entirely

Feedback is welcome!

klonos commented 2 years ago

...we inherited this markup mess from Drupal, which has the same problem.

Did you find any d.o issues while looking into this @indigoxela? Curious to see what possible solutions our Drupal brethren might have thought of.

...or drop the automatic span around prefix/suffix entirely

I was thinking that we could perhaps introduce a hook that would provide the defaults, but also allow affecting the markup used. I haven't fully fleshed out how/if that can happen.

klonos commented 2 years ago

I added some commentary to https://docs.backdropcms.org/form_api#field_prefix, also https://docs.backdropcms.org/form_api#prefix. Further suggestions/requests solicited, of course.

Thank you @bugfolder 🙏🏼

I've asked this question in our internal developers Slack channel:

I have searched in Drupal documentation and in articles/posts around the internet, yet it is still unclear to me in which cases you'd use #prefix/#suffix vs #field_prefix/#field_suffix ...does anyone know? For context + some relevant findings/documentation, see: Invalid markup if field prefix or suffix contain html tags #5735

I got the following replies:

Been a while but from what I remember #prefix is a more general theme render array property, FAPI inherits the render array so the theme system renders it as normal. #field_prefix is a themed implementation of a property specifically for FAPI, so it’s scope is more limited to just visible fields generally.

prefix/suffix add markups outside of the element template. field_prefix adds the markup inside the element template, usually right before the element tag.

We should validate these (they may only apply to D8/9), and if helpful add them to our documentation as well.

indigoxela commented 2 years ago

prefix/suffix add markups outside of the element template. field_prefix adds the markup inside the element template, usually right before the element tag.

This is also correct for Backdrop, but I don't think this solves anything per se. :wink:

argiepiano commented 2 years ago

Verify if there are alternative for field_prefix/field_suffix for these cases

Coming to this late and have not done my "homework", but how about just replacing those faulty #field_prefix and #field_suffix with #prefix and #suffix? Wouldn't that solve the faulty markup? Personally I've always used #prefix and #suffix for markup. I wasn't even aware of the other ones.

indigoxela commented 2 years ago

To verify if there are alternatives in core, I played with function image_field_instance_settings_form.

My conclusion: this abuse of field_prefix/field_suffix happened because currently there's no easy way to get nested inline elements as this item would need.

indigoxela commented 2 years ago

^^ Wow, simultaneously. :laughing:

argiepiano commented 2 years ago

Switching to prefix/suffix doesn't work, either (completely messes form display)

haha ok this answers it. Thanks @indigoxela

indigoxela commented 2 years ago

Probably the easiest solution would be to add a new CSS class like "item-inline-block", which allows individual form items inside a container to be displayed - inline-block.

Another solution would be to style these items, that currently abuse field_prefix/field_suffix, specifically. (Not my favorite, as several form items seem to need that sort of styling.)

indigoxela commented 2 years ago

Here we go, a PR based on the idea from my previous comment.

See it in action on:

There's some more questionable usage with wrapper markup, but these are span and don't seem to cause invalid markup.

Another documentation issue: #wrapper_attributes are used by all form items, not only the ones listed there. (See here).

indigoxela commented 4 months ago

Lack of interest for years now (it seems) and a merge conflict, that would need resolving... A good opportunity to close the PR. The bug report still seems valid to me, though, so not closing the issue.