Problem/Motivation

User can have access to the pages with form displays.

On such pages custom theme can be used instead of admin theme.

So it makes sense to have control on the image uploaded (in terms of responsive image)

With enabled responsive_image module we still have only Image widget for image type fields.

Image field widget

We need to have ability to use responsive image style here.

Several reasons why:
1. Obviously image style per breakpoints
2. We have to show webp image (instead of original format -> it's already working with webp enabled & responsive image enabled)

Steps to reproduce

1. Add Image field to any entity type.
2. Enable responsive_image module.
3. Go to Form display settings page and try to configure widget for this Image field.

Proposed resolution

Provide Responsive image widget in responsive_image module to be able to use responsive image style on form displays.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3382787

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

kostyashupenko created an issue. See original summary.

kostyashupenko’s picture

Issue summary: View changes
kostyashupenko’s picture

Issue summary: View changes
kostyashupenko’s picture

Title: Add responsive_image field widget in form display » Add responsive_image field widget in form display for image type field
kostyashupenko’s picture

Issue summary: View changes
kostyashupenko’s picture

Issue summary: View changes
kostyashupenko’s picture

Issue summary: View changes

sorlov made their first commit to this issue’s fork.

andypost’s picture

Related is for media

andypost’s picture

andypost’s picture

Status: Active » Needs work
Issue tags: +Needs tests

Looks nice but needs tests

kostyashupenko’s picture

Maybe makes sense to add this into 10.1.x instead of 11.x ?

I have tested patch from merge request -> it's working just like expected

andypost’s picture

Features are going to upcoming releases only

sorlov’s picture

Status: Needs work » Needs review

Fixed feedbacks and added test, so can be reviewed now

smustgrave’s picture

Status: Needs review » Needs work

Seemed to have a test failure.

smustgrave’s picture

Issue tags: -Needs tests

Removing tests tag as they were added

sorlov’s picture

Status: Needs work » Needs review

failed tests were from 11.x branch, rebased again, so should be fine now

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs change record

Tested MR 4628 and the formatter seems to work fine.

Tested by enabling Media library + responsive images
Changing image formatter to use new responsive_images one
Created a Media object without issue

Think for this new formatter we will need a change record so tagging for such.

Thanks!

vacho’s picture

I added a change record. Feel free to update.

vacho’s picture

Issue tags: -Needs change record
sorlov’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Tweaked the CR slightly for group it will impact. very very small change.

But LGTM!

vacho’s picture

StatusFileSize
new169.49 KB

Code looks good.

Also after apply i.e "wide" this responsive style was applied to the edit form.
Scree wide

catch’s picture

Issue tags: +Needs usa

One minor comment on the description text to try to remove some duplication. Leaving RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Between the comments from me and @lauriii, moving back to needs review.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Appears feedback has been addressed.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs usa +Needs usability review

https://git.drupalcode.org/project/drupal/-/merge_requests/4628#note_272118 on the MR wasn't resolved, and I think @lauriii's point is valid.

I also think this could use usability review, tried to tag it above for that above but apparently failed.

wim leers’s picture

sorlov’s picture

Status: Needs work » Needs review
catch’s picture

Moving this to needs product manager review for the question about adding a default responsive images style, since we don't ship any now, we'd need to make one in order to be able to reference it.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new2.06 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

sorlov’s picture

Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Needs work

Exciting to see that a simple addition to the config schema helped ensure this would not add a regression to Drupal core: https://git.drupalcode.org/project/drupal/-/merge_requests/4628/diffs#no... 🥳

@catch: But do we really need one? I think the test can just create a responsive image style 😅

sorlov’s picture

Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Needs work

Looks good! 👍

One remaining problem:

    $this->responsiveImageStyleStorage = $responsive_image_style_storage ?: \Drupal::service('entity_type.manager')->getStorage('responsive_image_style');

This should only ever be injected. Or is there otherwise a BC break I'm not seeing? 🤨

sorlov’s picture

Status: Needs work » Needs review

I have used exact same way as in ImageWidget, so if it is not correct, it should be updated on ImageWidget first

wim leers’s picture

Status: Needs review » Needs work

I have used exact same way as in ImageWidget, so if it is not correct, it should be updated on ImageWidget first

No, it should not because ImageWidget had to evolve in a non backwards compatibility breaking way in #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only). but this is new code, so we can get it right from the very start! 😊

I left suggestions to make that happen, it's really 99% ready, thanks for this nice addition!

sorlov’s picture

Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Needs work

AFAICT the newly create() method contains a bug now? I might be overlooking something though.

sorlov’s picture

Status: Needs work » Needs review

Updated create() method

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Was going to ping ux channel but think it would help to have a clear issue summary before hand please.

sorlov’s picture

Issue summary: View changes
Status: Needs work » Needs review

updated issue summary and fixed failed test

rkoller’s picture

Status: Needs review » Needs work
StatusFileSize
new29.17 KB

We discussed this issue at #3446447: Drupal Usability Meeting 2024-05-17. That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @rkoller, @shaal, @SKAUGHT, and @simhohell.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

While exploring the new responsive image form display widget we ran into one problem. On a standard 11.x-dev install with the responsive image module installed, and MR4628 applied, we had the following three options available for Preview responsive image styles:

  1. <no preview>
  2. Narrow
  3. Wide

Our assumption was that based on micro copy you would either get the preview image added in the dimensions of the chosen responsive image style or you have no preview depending on your choice. But problem was no matter what setting we set the resulting preview looked like this:

image fieldset with an image named corby_0.jpg added showing no preview

Our expectation, based on the micro copy, would have been that this would only be the result in case option 1, no preview, was chosen. So we were not sure if this is a bug, and in general we've asked us how this feature is intended to work? We would need some clarification in that regard before we are able to make any recommendations.

We will also continue our initial discussion we've started in regards of the micro copy. There was already a clear consensus to change the label Preview responsive image style to Responsive image style for preview. "Technically" a user isn't "previewing a responsive image style", instead a preview image based on that responsive image style is being generated. About the corresponding description, there was the consensus to clarify where the image is shown (aka that it is shown on the edit form), and that "preview responsive image" is too verbose as well as misleading (There is only a single image being used, based on the responsive image style - but again we need to know how the feature is supposed to work).
One radio button option is called Bar with progress meter while within the description it is referred to as a progress bar. Aside this inconsistency in terminology it was also noted that the first sentence of the description is imprecise, the throbber actually is communicating the status of the upload, it is telling the user if the upload is still underway or was already finished. But the microcopy about the progress indicator would apply to regular image widgets as well. So making adjustments in that regard would be more suitable for a followup issue to keep things consistent between image and responsive image widgets.

But we will finish the discussions about the micro copy as well as the functionality in general as soon as the feedback is in. Thank you in advance!

Not sure what the best status would be in this case? I went with needs work but to postpone with maintainer needs more infos felt wrong since i am not the maintainer nor person working on this issue. I'll go with needs work, but please adjust accordingly if that is not the right choice and there is a more suitable one. Apologies in that case.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.