Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Media fields accessibility - second try #27712

Merged
merged 59 commits into from
May 30, 2020
Merged

Conversation

astridx
Copy link
Contributor

@astridx astridx commented Jan 30, 2020

Pull Request for Issue #27571 .

Please see #27650 and #27712 (comment) for test instructions.

In this Pull Request I tried to implement a suggestion from this comment - I use the Subform Field.

Edit: 10th of February
Test Instructions - You do NOT need composer or npm

  1. Create a Custom Field of type media and see, that this field did not include an alternative text in front end. I named this custom field old.
  2. Apply this patch and create a new custom field of type media. See hat all parameters, for example directory, are working. I named this custom field new.
  3. See, that the custom fields new is still shown correctly in front end. When you edit an article, the text field for the alternative text is not show for the old custom field at the begin. But when you delete the image temporarily the text field appear and you can use it.
  4. Use the new form field regardless of custom field
    a) Add
		<field
			name="image2"
			type="accessiblemedia"
			label="COM_CONTACT_FIELD_PARAMS_IMAGE_LABEL"
			directory="banners"
		/>

somewhere for example in line 276 of /administrator/components/com_contact/forms/contact.xml.
b) If you use my example add

	`<?php echo $this->form->renderField('image2'); ?>`

in /administrator/components/com_contact/tmpl/contact/edit.php line 55.
c) See that you can use the field.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Jan 30, 2020
@astridx astridx changed the title Media fields accessibility - second try [4.0] Media fields accessibility - second try Jan 30, 2020
@brianteeman
Copy link
Contributor

Awesome!!! - Works perfectly for me.

Two suggestions

  1. Dont make it optional to add the alternative text when defining the field - you should always have one so no need for that option
  2. You could instead have an option to make the alternative text a required field - not sure if that will be possible as it would only be required if you have an image

@brianteeman
Copy link
Contributor

In the generated code when the field is presented you have a small bug as the generated code is

image

@brianteeman
Copy link
Contributor

Sorry there is another small issue when using the field in the front end

Before

image

After

image

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Jan 30, 2020

If the idea is to implement this just in media custom field, Joomla\CMS\Form\Field\AccessiblemediaField is not required. The form can be added directly in the plugin like here

$parent_fieldset = $parent_field->appendChild(new DOMElement('form'));
.

Otherwise AccessiblemediaField should not have a dependency on media field plugin.

{
$fieldNode->setAttribute('type', 'accessiblemedia');
}

$fieldNode->setAttribute('hide_default', 'true');
Copy link
Contributor

Choose a reason for hiding this comment

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

This has to be set on the media field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SharkyKZ Are you sure. If so, I don't understand it correctly. I want to overwrite this line:

$node->setAttribute('type', $field->type);

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean hide_default attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. That is not part of this PR. I think we should do it in a separate PR, right?

Copy link
Contributor

@SharkyKZ SharkyKZ Jan 30, 2020

Choose a reason for hiding this comment

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

It is part of this PR. $fieldNode is now a Subform field but hide_default property belongs on the media field which is now inside the subform.

Personally I'd just remove AccessiblemediaField and use Subfields as example. This is basic code for implementing the subform with media and alt fields:

// This is now the subform field.
$fieldNode->setAttribute('type', 'subform');

// Add form to subform.
$form = $fieldNode->appendChild(new DOMElement('form'));

// This is the media field.
$media = $form->appendChild(new DOMElement('field'));
$media->setAttribute('type', 'media');
$media->setAttribute('name', 'filename');
$media->setAttribute('hide_default', 'true');

// Thie is the alt field.
$alt = $form->appendChild(new DOMElement('field'));
$alt->setAttribute('type', 'text');
$alt->setAttribute('name', 'alt');

Copy link
Contributor

Choose a reason for hiding this comment

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

The same is needed for other parameters like directory. It's currently not working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your are right. I now understand what you mean.

@astridx
Copy link
Contributor Author

astridx commented Jan 30, 2020

@SharkyKZ The idea is to use a different form field if an alternative value can be entered. Not media, but accessiblemedia. And accessiblemedia inherits from subform.
This is not a nice object-oriented. I know. accessiblemedia should be a child of the media field semantically correct.

@astridx
Copy link
Contributor Author

astridx commented Jan 30, 2020

@brianteeman In the generated code when the field is presented you have a small bug as the generated code is

Thank you, done.

@astridx
Copy link
Contributor Author

astridx commented Jan 30, 2020

@brianteeman Sorry there is another small issue when using the field in the front end

Thanks again. Please see if you think that's good.

@Quy
Copy link
Contributor

Quy commented Jan 30, 2020

No image is selected, yet it is displayed on the frontend as a broken image.

@astridx
Copy link
Contributor Author

astridx commented Apr 18, 2020

@SharkyKZ I want to make sure I understand you correctly in the comment here: #27712 (comment)

In my opinion is the check you would like to have in the plugin not wrong in the field and I have now expanded it.

--

If I understand you correctly, you think that this check should be done in the plugin, since it is only necessary in the plugin. It is not necessary in the Field.

--

In the case of the plugin it could be, that we have an "old" custom field, that was created using the MediaField (not the AccessiblemediaField - That will be the case, if Joomla Version 3 is updated to Version 4). In this case, the value does not contain an alt text. It is a simple string, for example: "images/joomla_black.png".
For showing everything (image and alt text) correctly in back end the AccessiblemediaField need da json string like this: "{"imagefile":"images\/banners\/banner.jpg","alt_text":""}" because it is a child of the SubformField.
(I did not extend the MediaField, because I could reuse more. And as a SubformField we always have the possibility to add other things that belong to the field in the same way.)

--

You're right, I created this check in the field in order to make the value of the custom fields backwards compatible. But: A check at this point will correct other possible errors. In my opinion the check is even better at this point.

--

Do I see that correctly, or are there other reasons to check the value in the custom field that I don't see?

@SharkyKZ
Copy link
Contributor

The check should be in the plugin because it looks like it is a workaround for the plugin. Library code should not have extension specific hacks. But if the intention is to have this field as a drop-in replacement for Joomla\CMS\Form\Field\MediaField in other places, maybe it's fine.

@astridx
Copy link
Contributor Author

astridx commented Apr 21, 2020

@SharkyKZ Thanks for the feedback.

The check should be in the plugin because it looks like it is a workaround for the plugin.

I don't see any way to do this in the plugin. But I like to be instructed when others prefer it.
In my opinion, it is even better as it is in this PR, because other possible errors are also caught here.

But if the intention is to have this field as a drop-in replacement for Joomla\CMS\Form\Field\MediaField in other places, maybe it's fine.

This field should be an alternative to the media field, where you don't have to add a text field for the alternative text in a form. Everything is together - it belongs together.
In my opinion it is not possible to replace the previous field with backward compatibility.

@richard67
Copy link
Member

@astridx Shall I resolve the conflict in file plugins/fields/media/tmpl/media.php for you?

@astridx
Copy link
Contributor Author

astridx commented May 10, 2020

@richard67 I think I managed it myself. But it would be good if you could check. I am not good at conflict resolution. I usually delete the branch and do it from scratch.

@astridx
Copy link
Contributor Author

astridx commented May 10, 2020

@zero-24 I just saw that you requested a change. But I don't really know what to change. Can it be that this request is no longer current or can you help me?

@richard67
Copy link
Member

@astridx Was an easy conflict and you solved it the right way. I was sure you can do that yourself, just offered my help because I saw if and thought maybe you are too busy with other things.

@richard67
Copy link
Member

@astridx Possibly @zero-24 meant this comment #27712 (comment) about the missing doc block?

@wilsonge wilsonge merged commit 08935a8 into joomla:4.0-dev May 30, 2020
@wilsonge
Copy link
Contributor

I'm merging this because it's definitely a step in the right direction and we can tweak this moving forwards as required. Thankyou very much @astridx

@Scrabble96
Copy link
Contributor

This option to add 'Alt text' needs to be added to the "Image List" field as well:
image-list

@richard67
Copy link
Member

@Scrabble96 Could you open a new issue for that it there is not already one found by a quick view? That would be helpful. Thanks in advance.

@Scrabble96
Copy link
Contributor

I opened the new issue (#29640) as requested, but no-one except me has commented and adding/editing any code for a PR is way beyond me. Thanks.

sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Nov 6, 2020
This PR continues the awesome work of @astridx in joomla#27712 by correcting the behaviour of the alt text and changing the strings to match the work done in joomla#31318
chmst pushed a commit that referenced this pull request Nov 7, 2020
* [4.0] Accessible media field ALt Text

This PR continues the awesome work of @astridx in #27712 by correcting the behaviour of the alt text and changing the strings to match the work done in #31318

* remove comment

* Update plugins/fields/media/tmpl/media.php

Co-authored-by: Quy <quy@fluxbb.org>

Co-authored-by: Quy <quy@fluxbb.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Accessibility Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants