Skip to content

Comments

Media fields accessibility#27650

Closed
astridx wants to merge 15 commits intojoomla:4.0-devfrom
astridx:media
Closed

Media fields accessibility#27650
astridx wants to merge 15 commits intojoomla:4.0-devfrom
astridx:media

Conversation

@astridx
Copy link
Contributor

@astridx astridx commented Jan 25, 2020

Pull Request for Issue #27571 .

Summary of Changes

I have integrated the possibility to enter an alternative text for a media field. This PR is a draft because it affects many other components. I haven't checked them all yet. I would like to know your opinion first.

Testing Instructions

  1. Apply this patch. The commands npm ci or/and composer i are not necessary.
  2. Create two custom fields of type media in the content component. Choose yes for the Alternative text for image option and no for the other.
    Articles  Fields   test   Administration(2)
    Articles  New Field   test   Administration
  3. Edit an article that contains the two custom fields. You only see the text field where you chose to enter an alternative text.
    Articles  New   test   Administration
  4. Look at the output in the front end. Once you see the alternative text you entered. The image, for which you could not enter any text, is also a11y compliant. Here is an empty alt text inserted.
  5. Since the media field is used in many places in Joomla, many places have to be adjusted. Therefore, this first 4 steps are not enough. Look for different places where the media field is used. For example, create a banner module with pictures, create a contact with a picture or an article with a picture. Always look at the output in the front end. Make sure the image is saved and dispayed correctly. Write it here in the issue if you find a place that is not correct.
  6. Make sure that the text field for entering an alternative text is not displayed in other fields than an custom field. So far, this alternative text has not yet been forwarded to the output in the front end, except for a custom field. Therefore, this would confuse an user. Usually there is an extra text field to use the alternative text.
    Articles  New   test   Administration(1)
    Perhaps we should consider doing this uniformly in the future via the media field.

Documentation Changes Required

https://docs.joomla.org/J3.x:Adding_custom_fields/Media_Field

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Jan 25, 2020
@brianteeman
Copy link
Contributor

Awesome. I will check it tomorrow

@SharkyKZ
Copy link
Contributor

This looks like a very complex solution given that media field can be complemented by an additional text field as is a already done in some components. The issue really only exists in the media custom field plugin and, IMO, should be fixed there.

@brianteeman
Copy link
Contributor

all seems ok to me - thanks.

Some styling improvements but thats all I saw


<field
name="alt_text"
type="list"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is just a yes/no field then I would make it a switcher

@astridx
Copy link
Contributor Author

astridx commented Jan 26, 2020

@SharkyKZ Tthank you for your opinion
You are right, it is complicated and requires some rework. That's why it's a draft at first.

But

  1. I haven't seen an easier way to insert the alternate text in the custom field. Maybe I'm missing something there.
  2. And then I think that the alternative text belongs to the media field and my draft corrects it.

@brianteeman I like your suggestion to convert the selection field into a switch. If my draft gets a little more approval I will definitely change it.

@SharkyKZ
Copy link
Contributor

I haven't seen an easier way to insert the alternate text in the custom field. Maybe I'm missing something there.

Custom field plugins have the ability to modify form's DOM content. So it's possible to change the media field into a subform field that contains media field and text field for alt attribute. This would still require data type checks to handle but at least they'd be limited to wherever custom fields are used.

And then I think that the alternative text belongs to the media field and my draft corrects it.

I don't necessarily disagree, but doing it this way is really messy. You need to account for both data types in every place its used. Currently this PR doesn't do that. The same thing would be needed by all 3rd party developers. It also seems wrong having to modify Joomla\CMS\HTML\HTMLHelper for this. In my opinion, this approach is not worth it.

Some other solutions to consider:

Modify media custom field plugin to use subform. Changes would be needed only in the custom field plugin and template overrides where fields are rendered manually.

Introduce a new field (e.g. Joomla\CMS\Form\Field\ImageField) and change type in media custom field plugin to this. Similar to using subform, changes limited to custom fields but eventually other extensions can be switched to this field.

Introduce a new field and a new custom field plugin. Making use of alt text requires users to edit fields in their content anyways, so might as well go a little further and have users create new fields to make use of the feature.

Final option, change data type only when the field is using alt text. This way existing extensions are not affected.

@brianteeman
Copy link
Contributor

I was thinking on the lines of a subform myself until I saw this proposal.

I can't/won't comment on the code quality/complexity/maintainability of either approach as I am not qualified to do so

@astridx
Copy link
Contributor Author

astridx commented Jan 30, 2020

@SharkyKZ Thank you for your suggestions. I tried to implement one of them.

Because of my new proposal #27712 I close this Pull Request.

@astridx astridx closed this Jan 30, 2020
@astridx astridx deleted the media branch August 9, 2020 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants