? ? ? ? Pending

User tests: Successful: Unsuccessful:

avatar astridx
astridx
30 Jan 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.

avatar astridx astridx - open - 30 Jan 2020
avatar astridx astridx - change - 30 Jan 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 30 Jan 2020
Category Administration Language & Strings Libraries Front End Plugins
avatar astridx astridx - change - 30 Jan 2020
Title
Media fields accessibility - second try
[4.0] Media fields accessibility - second try
avatar astridx astridx - edited - 30 Jan 2020
avatar brianteeman
brianteeman - comment - 30 Jan 2020

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
avatar brianteeman
brianteeman - comment - 30 Jan 2020

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

image

avatar brianteeman
brianteeman - comment - 30 Jan 2020

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

Before

image

After

image

avatar SharkyKZ
SharkyKZ - comment - 30 Jan 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.

avatar astridx
astridx - comment - 30 Jan 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.

avatar astridx astridx - change - 30 Jan 2020
Labels Added: ? ?
avatar astridx
astridx - comment - 30 Jan 2020

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

Thank you, done.

avatar astridx
astridx - comment - 30 Jan 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.

59791c8 30 Jan 2020 avatar astridx fixes
avatar Quy
Quy - comment - 30 Jan 2020

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

avatar joomla-cms-bot joomla-cms-bot - change - 31 Jan 2020
Category Administration Language & Strings Libraries Front End Plugins Administration com_content Language & Strings Layout Libraries Front End Plugins
avatar joomla-cms-bot joomla-cms-bot - change - 31 Jan 2020
Category Administration Language & Strings Libraries Front End Plugins com_content Layout Administration Language & Strings Layout Libraries Front End Plugins
avatar astridx
astridx - comment - 31 Jan 2020

Today I changed this PR.

I have introduced a new field named accessiblemedia.

In my opinion, an alt text belongs to a field that inserts an image.
And this is a way of making a field type available for this purpose without breaking backward compatibility. The media field remains. I actually wanted to expand the media field. That would be more semantically correct. But then I did expand the Subfield field. I found this easier.

I do not change anything in the custom field anymore. The accessiblemedia field is now automatically loaded here. And an alt text is inserted in any case. At least an empty one.

What is important to me: Now, the field can also be used in other places.

So, new test instructions:

  1. Create a Custom Field of type media and see, that you have to insert an alt text and that this text is displayed in the front end. (more detailed explanation here)

  2. Use the new form field
    a) Add

		<field
			name="image2"
			type="accessiblemedia"
			label="COM_CONTACT_FIELD_PARAMS_IMAGE_LABEL"
			hide_none="1"
		/>

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.

For my example up to know you can not display it in the front end, because it is not saved in the database. And for this, the array value has to cast into a string. But it works.

If you like my idea I will create a more detailed test instruction.

I thank you for the patience with me so far and look forward to further tips for improvements.

@SharkyKZ I looked at what you think should be implemented, the addition of the text field for the alt text in the media custom field method onCustomFieldsPrepareDom. But: I liked this version better. Do you think so too? You know your way around better with Joomla, so I would reconsider if you disagreed.

@brianteeman It's not nice if you submit a PR and nobody cares. I am always happy that you test and you will always find something with me :)

@Quy I added all your suggestions. Thank you.

avatar astridx astridx - change - 31 Jan 2020
The description was changed
avatar astridx astridx - edited - 31 Jan 2020
avatar brianteeman
brianteeman - comment - 31 Jan 2020

I will take another look tomorrow

avatar SharkyKZ
SharkyKZ - comment - 1 Feb 2020

Most code in layouts/joomla/form/field/media/accessiblemedia.php (except rendering part) should be in field class.

avatar brianteeman
brianteeman - comment - 7 Feb 2020

Sorry for the delay in getting to this. @astridx could you fix the conflicts please and then I will recheck

08b9709 9 Feb 2020 avatar astridx wip
avatar brianteeman
brianteeman - comment - 9 Feb 2020

Not sure if it is something I am doing wrong but after creating the field and then trying to use it to add an image to an article the article is saved BUT the image is not

avatar brianteeman brianteeman - test_item - 9 Feb 2020 - Tested successfully
avatar brianteeman
brianteeman - comment - 9 Feb 2020

I have tested this item ✅ successfully on 19e4ed1


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27712.

avatar astridx
astridx - comment - 9 Feb 2020

@brianteeman Thanks for your test. You are super fast.

Now I hope that my version is technically OK.

avatar SharkyKZ
SharkyKZ - comment - 9 Feb 2020

Looks better now but there are still some issues. Existing field values are not being handled.

572a900 9 Feb 2020 avatar astridx wip
1f67de2 9 Feb 2020 avatar astridx wip
6828c48 9 Feb 2020 avatar astridx wip
3c8a1f4 9 Feb 2020 avatar astridx wip
avatar astridx
astridx - comment - 10 Feb 2020

@SharkyKZ Thanks for your tips.

I have corrected backwards compatibility and extended the test instructions.

  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 field new is still shown correctly in front end. When you edit an article, the text field for the alternative text is not show 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
    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.

avatar astridx astridx - change - 10 Feb 2020
The description was changed
avatar astridx astridx - edited - 10 Feb 2020
avatar astridx astridx - change - 14 Feb 2020
The description was changed
avatar astridx astridx - edited - 14 Feb 2020
avatar astridx astridx - change - 15 Feb 2020
The description was changed
avatar astridx astridx - edited - 15 Feb 2020
avatar brianteeman
brianteeman - comment - 11 Mar 2020

Maybe someone can convince the accessibility team to test this

avatar astridx
astridx - comment - 12 Mar 2020

Should this PR get the label Release Blocker? I ask because the original issue #27571 had this label and is now closed.

avatar astridx astridx - change - 14 Mar 2020
Labels Added: ?
avatar astridx astridx - change - 24 Mar 2020
Labels Added: ? ?
Removed: ?
avatar astridx astridx - change - 29 Mar 2020
Labels Added: ?
Removed: ?
avatar astridx astridx - change - 13 Apr 2020
Labels Added: ?
Removed: ?
avatar astridx astridx - change - 18 Apr 2020
Labels Added: ?
Removed: ?
avatar astridx
astridx - comment - 18 Apr 2020

Concerning the error in the system checks:
I just run
libraries/vendor/bin/codecept run acceptance tests/Codeception/acceptance/administrator/components/com_media/
on my local machine on this branch and I get no error.

Perhaps this related to this: #28660

Screenshot from 2020 04 18 10 19 05

avatar astridx astridx - change - 18 Apr 2020
Labels Added: ?
Removed: ?
avatar astridx astridx - change - 18 Apr 2020
Labels Added: ?
Removed: ?
avatar astridx
astridx - comment - 18 Apr 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?

avatar SharkyKZ
SharkyKZ - comment - 20 Apr 2020

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.

avatar astridx
astridx - comment - 21 Apr 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.

avatar richard67
richard67 - comment - 10 May 2020

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

avatar astridx astridx - change - 10 May 2020
Labels Added: ?
Removed: ?
avatar astridx
astridx - comment - 10 May 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.

avatar astridx
astridx - comment - 10 May 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?

avatar richard67
richard67 - comment - 10 May 2020

@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.

avatar richard67
richard67 - comment - 10 May 2020

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

avatar astridx astridx - change - 23 May 2020
Labels Added: ?
Removed: ?
avatar astridx astridx - change - 30 May 2020
Labels Added: ?
Removed: ?
avatar wilsonge wilsonge - change - 30 May 2020
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2020-05-30 11:28:05
Closed_By wilsonge
Labels Added: ?
Removed: ?
avatar wilsonge wilsonge - close - 30 May 2020
avatar wilsonge wilsonge - merge - 30 May 2020
avatar wilsonge
wilsonge - comment - 30 May 2020

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

avatar Scrabble96
Scrabble96 - comment - 16 Jun 2020

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

avatar richard67
richard67 - comment - 16 Jun 2020

@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.

avatar Scrabble96
Scrabble96 - comment - 27 Aug 2020

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.

Add a Comment

Login with GitHub to post a comment