? NPM Resource Changed ?

Success

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
26 Sep 2020

This PR solves a lot of bugs and brings feature parity to J3, keep reading

Summary of Changes

Bug fixes:

  • Changes the a-tag to button in the media modals
  • Restores width and height params in the media manager object (in 2 places)
  • Separates the image selection script from the media script (if you had an Utd-button without a media field the button was foobar)

New features (parity with J3, basically missing in J4):

  • Ability to enter ALT text for any image (used in the editors)
  • Ability to set if an image will be lazy loaded in the content area (eg inside an editor)
  • Ability to set ALT text for images inserted through drag and drop in tinyMCE
  • Ability to set if an image will be lazy loaded for images inserted through drag and drop in tinyMCE
  • New helper HTMLHelper::cleanImageURL, explainer below

Lazyloading

The concept is simple: it just works!

  • For any image inserted in an editor (tinyMCE, Codemirror, none, or any 3rd PT) the user has the ability to select if the image will be lazy loaded
  • For images used though the media field there's a helper to... wait for it, help you. By default the URL that is stored in the database has 2 extra URL parameters joomla_image_width and joomla_image_height that represent the width and height of the image. Passing the URL in the helper you'll get back an object with the clean url (without the 2 extra params mentioned before, and the attributes (array of width and height). So since we're already in an overridable file (layout) anyone can decide if the position of the image should or not be lazy loaded. Reading the fulltext layout is more explanatory than reading some text out of context: https://github.com/joomla/joomla-cms/blob/3427a9ca0bb265f72cb8da9a61c508a0a5859afa/layouts/joomla/content/full_image.php

Testing Instructions

Download the package from the bottom part of the PR in Github or do it through git command

Test the tinyMCE dnd
Make sure tinyMCE is your editor
Try to insert an image in an article with dnd, enter some alt text and leave the lazy loaded checked
Repeat but unselect the lasyloaded and leave the alt empty
Toggle the editor and check the produced HTML

Test the tinyMCE media button
Try to insert an image in an article with the media button, enter some alt text and leave the lazy loaded checked
Repeat but unselect the lasyloaded and leave the alt empty
Toggle the editor and check the produced HTML

Repeat the same for Codemirror and none

Custom Fields
setup a media custom field
setup a imagelist custom field
make sure the custom fields work

check the html, should have attributes: loading=lazy, width, height for all the images.

Media field
Insert an full image
Check the html, should have attributes: loading=lazy, width, height

Insert an intro image
Check the html, should have attributes: loading=lazy, width, height

Preview

XTD-Button:
Screenshot 2020-09-26 at 22 27 36

TinyMCE drag and drop
Screenshot 2020-09-26 at 22 28 12

Documentation Changes Required

Document how the media field url should be handled.
Provide some guidelines for styling the lazy loaded images, basically explain how to use the css property object-fit: scale-down, cover, fill, etc https://caniuse.com/object-fit to get the same responsive behaviour as without defined width,height

A good resource for width/height from Jen Simmons @jensimmons : https://speakerdeck.com/jensimmons/using-the-html-width-and-height-attributes-to-improve-web-page-loading

For devs and implementors:

Images selected with the media field will have by default some url params eg: images/joomla_black.png?joomla_image_width=225&joomla_image_height=50. Leaving the URL like that in your layouts is totally fine, Joomla is exercising the same technic for stylesheets and script to invalidate the cache (eg: src="/media/system/js/core.min.js?b276ec42f4135ff7408390a982e26604"). That said if you decide to skip couple of lines of code you're missing some significant performance gains from lazily loading the images. All it takes is:

  • Pass the URL to the helper:
$img = HTMLHelper::cleanImageURL($images->image_fulltext);

/**
 * $img->url // The clean URL, no extra params
 * $img->attributes // Array
 * $img->attributes['width']  // The width of the image
 * $img->attributes['height']  // The height of the image
 **/
  • Set the lazy loading attribute:
$img->attributes['loading'] = 'lazy';
  • Render the image:
<img src="<?php echo htmlspecialchars($img->url, ENT_COMPAT, 'UTF-8'); ?>" alt="" <?php echo ArrayHelper::toString($img->attributes); ?> />
avatar dgrammatiko dgrammatiko - open - 26 Sep 2020
avatar dgrammatiko dgrammatiko - change - 26 Sep 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 26 Sep 2020
Category JavaScript Administration com_media NPM Change Language & Strings Repository Layout Libraries Front End Plugins
avatar dgrammatiko dgrammatiko - change - 26 Sep 2020
Labels Added: ? NPM Resource Changed ?
avatar dgrammatiko dgrammatiko - change - 26 Sep 2020
The description was changed
avatar dgrammatiko dgrammatiko - edited - 26 Sep 2020
avatar SharkyKZ
SharkyKZ - comment - 26 Sep 2020

Why do you insist on abusing URLs?

avatar dgrammatiko
dgrammatiko - comment - 26 Sep 2020

Why do you insist on abusing URLs?

There is no abuse of the final URL. Also what is the problem with the URL? I'm using it to pass information that otherwise I have to somehow calculate in runtime which will defeat the purpose of any perf gains...

avatar SharkyKZ
SharkyKZ - comment - 26 Sep 2020

Really would be simpler and cleaner to add width/height fields for intro/fulltext images.

avatar dgrammatiko
dgrammatiko - comment - 26 Sep 2020

Really would be simpler and cleaner to add width/height fields for intro/fulltext images.

Nope, It requires 2 additional fields and 2 additional sql columns and will break any existing extension. Also it will be a mess trying to sync 3 fields in the JS.

avatar SharkyKZ
SharkyKZ - comment - 26 Sep 2020

We don't need to add any columns. All image related data is JSON encoded and stored in a single column.

avatar dgrammatiko
dgrammatiko - comment - 26 Sep 2020

We don't need to add any columns. All image related data is JSON encoded and stored in a single column.

You're welcome to do a PR with that. BTW the idea is that ALL images using the form field media should have the ability to use those attributes (system wide change). Curious to see how your approach will solve that...

avatar zero-24
zero-24 - comment - 27 Sep 2020

I have installed the package from the packager section using there com_content (+tinymce) I get the console error message
image

Also it seems the new "alternative text" stuff is missing?
image

Or I'm doing something wrong on my side?

avatar dgrammatiko
dgrammatiko - comment - 27 Sep 2020

@zero-24 I can't replicate with the branch code (tested with Safari, FF, Chrome), maybe something broken in the package?

avatar zero-24
zero-24 - comment - 27 Sep 2020

hmm will try again and come back to you :)

avatar dgrammatiko dgrammatiko - change - 27 Sep 2020
The description was changed
avatar dgrammatiko dgrammatiko - edited - 27 Sep 2020
avatar richard67
richard67 - comment - 27 Sep 2020

Update test on MySQLi and PostgreSQL is ok. Now am doing new installation tests for both database types.

Sorry, wrong PR.

avatar Quy
Quy - comment - 28 Sep 2020

Alt text/lazy load checkbox should be hidden if a folder is selected. To reproduce, click an image, then click a folder.

Click several images consecutively reduces the viewable area.

30784

avatar dgrammatiko
dgrammatiko - comment - 28 Sep 2020

Click several images consecutively reduces the viewable area.

This was supposed to be fixed with 1b9b603

Alt text/lazy load checkbox should be hidden if a folder is selected

Selecting a folder is not firing an event thus it cannot be handled. FWIW the alt text input + lazyload checkbox are not part of the media manager (not the component's responsibility how an image will be used in any extension anyway). If this is show stopper let's fix it here either wise open an issue after this PR is merged (if ever). The reason is that it requires some more tweaking in the media manager and I don't won't to do it here, the PR is already touching a lot of files as is...

avatar Quy
Quy - comment - 28 Sep 2020

This was supposed to be fixed with 1b9b603

I installed with the latest prebuilt package from here so it should have been included/fixed, but it is still happening.

https://ci.joomla.org/artifacts/joomla/joomla-cms/4.0-dev/30784/downloads/35808/

avatar dgrammatiko dgrammatiko - change - 28 Sep 2020
The description was changed
avatar dgrammatiko dgrammatiko - edited - 28 Sep 2020
avatar dgrammatiko
dgrammatiko - comment - 28 Sep 2020

I installed with the latest prebuilt package

It was my fault I tested the code locally but manage to push something different...

avatar zero-24
zero-24 - comment - 29 Sep 2020

hmm will try again and come back to you :)

Just re tested. It seems you have to "click" the image and it is not egnoth to just "select the image" could that might be a different event?
lazyloading_image

avatar dgrammatiko
dgrammatiko - comment - 29 Sep 2020

It seems you have to "click" the image and it is not egnoth to just "select the image"

That seems to be a media manager bug, or to be more accurate here the selected box should be controlled by js not css driven

avatar zero-24
zero-24 - comment - 29 Sep 2020

That seems to be a media manager bug, or to be more accurate here the selected box should be controlled by js not css driven

Ok can that be fixed here or better with the other PR for the media manager as mention above?

avatar dgrammatiko
dgrammatiko - comment - 29 Sep 2020

Ok can that be fixed here or better with the other PR for the media manager as mention above?

I would rather tackle this in another PR. The only things touched in Media manager here were the missing arguments for the event, changing the selection behaviour has a bigger scope (css + Vue) and I guess it needs it's own testing procedure. But I'm ok doing it here if people are willing to aggressively test media manager

avatar zero-24 zero-24 - test_item - 29 Sep 2020 - Tested successfully
avatar zero-24
zero-24 - comment - 29 Sep 2020

I have tested this item ✅ successfully on ac18bf2


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

avatar zero-24
zero-24 - comment - 29 Sep 2020

I would rather tackle this in another PR. The only things touched in Media manager here were the missing arguments for the event, changing the selection behaviour has a bigger scope (css + Vue) and I guess it needs it's own testing procedure. But I'm ok doing it here if people are willing to aggressively test media manager

I'm happy to do so. :)


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

avatar dgrammatiko
dgrammatiko - comment - 29 Sep 2020

@zero-24 @Quy the changes are not that bad:

in actions.js add

/**
 * Dispatch a window event expected in the media image select script
 *
 * @param detail {object}  The item details
 */
const fireWindowEvent = (detail) => {
  window.parent.document.dispatchEvent(
    new CustomEvent(
      'onMediaFileSelected',
      {
        bubbles: true,
        cancelable: false,
        detail: detail,
      }
    )
  );
}

and change export const toggleBrowserItemSelect = (context, payload) => { to

/**
 * Toggle the selection state of an item
 * @param context
 * @param payload
 */
export const toggleBrowserItemSelect = (context, payload) => {
    const item = payload;
    const isSelected = context.state.selectedItems.some(selected => selected.path === item.path);

    if (!isSelected) {
        // Unselect all other selected items, if the shift key was not pressed during the click event
        if (!(event.shiftKey || event.keyCode === 13)) {
          context.commit(types.UNSELECT_ALL_BROWSER_ITEMS);
        }

        context.commit(types.SELECT_BROWSER_ITEM, item);

        if (item.path && item.type === 'file') {
          fireWindowEvent({
            path: item.path,
            thumb: item.thumb,
            fileType: item.mime_type ? item.mime_type : false,
            extension: item.extension ? item.extension : false,
            width: item.width ? item.width : 0,
            height: item.height ? item.height : 0,
          });
        } else {
          fireWindowEvent({});
        }
    } else {
      context.commit(types.UNSELECT_BROWSER_ITEM, item);
      fireWindowEvent({});
    }

  // If more than one item was selected and the user clicks again on the selected item,
  // he most probably wants to unselect all other items.
  if (context.state.selectedItems.length > 1) {
    context.commit(types.UNSELECT_ALL_BROWSER_ITEMS);
    context.commit(types.SELECT_BROWSER_ITEM, item);
  }
};

in browser,vue change unselectAllBrowserItems(event) { to

            /* Unselect all browser items */
            unselectAllBrowserItems(event) {
                const clickedDelete = (event.target.id !== undefined && event.target.id === 'mediaDelete') ? true : false;
                const notClickedBrowserItems = (this.$refs.browserItems && !this.$refs.browserItems.contains(event.target)) || event.target === this.$refs.browserItems;
                const notClickedInfobar = this.$refs.infobar !== undefined && !this.$refs.infobar.$el.contains(event.target);
                const clickedOutside = notClickedBrowserItems && notClickedInfobar && !clickedDelete;
                if (clickedOutside) {
                    this.$store.commit(types.UNSELECT_ALL_BROWSER_ITEMS);

                    window.parent.document.dispatchEvent(
                        new CustomEvent(
                            'onMediaFileSelected',
                            {
                                "bubbles": true,
                                "cancelable": false,
                                "detail": {}
                            }
                        )
                    );
                }
            },

and lastly in item.js change handleClick(event) { to

        /**
         * Handle the click event
         * @param event
         */
        handleClick(event) {
          this.$store.dispatch('toggleBrowserItemSelect', this.item);
        },

But let's fix this in another PR

avatar zero-24
zero-24 - comment - 29 Sep 2020

But let's fix this in another PR

Fine for me 👍

avatar Quy
Quy - comment - 29 Sep 2020

If the alt text The "quote" test contains a double quote, then only the text up to it is saved.

CMS Content > Image
<img src="images/joomla_black.png" alt="The " width="225" height="50" loading="lazy" />

Insert > Image
<img src="images/joomla_black.png" alt="The &quot;quote&quot; test" width="225" height="50" />

avatar dgrammatiko
dgrammatiko - comment - 29 Sep 2020

If the alt text The "quote" test contains a double quote, then only the text up to it is saved.

That also wouldn't work in J3.x:

alt = this.fields.alt.value,

If it's only the " that needs escaping then the fix is really easy:

    altInputFn(e) {
       this.setAttribute('alt-value', e.target.value.replace(/"/g, '&quot;'));
     }

Edit: fixed with 92294f6

To test it open the browser console and paste

'Some "text" and some random quote: "'.replace(/"/g, '&quot;');

Would get
Screenshot 2020-09-29 at 18 07 53

Or test it with the D/L package

avatar dgrammatiko dgrammatiko - change - 2 Oct 2020
The description was changed
avatar dgrammatiko dgrammatiko - edited - 2 Oct 2020
avatar dgrammatiko
dgrammatiko - comment - 3 Oct 2020

@Quy can you please re test this one so it can moved to RTC and have the maintainers decide if this is acceptable or not?

avatar zero-24
zero-24 - comment - 3 Oct 2020

and have the maintainers decide if this is acceptable or not?

once tested i can hit merge here. 👍

avatar Quy
Quy - comment - 3 Oct 2020

There are a few todos to translate strings. Do in a separate PR?

avatar Quy
Quy - comment - 3 Oct 2020

Also there is Confirm text that I couldn’t get it to reproduce in a test. Please provide steps.

avatar dgrammatiko
dgrammatiko - comment - 3 Oct 2020

There are a few todos to translate strings

The strings are for TinyMCE, should be done in the crowdin

Also there is Confirm text that I couldn’t get it to reproduce in a test.

There is no Confirm anymore

avatar zero-24
zero-24 - comment - 3 Oct 2020

The strings are for TinyMCE, should be done in the crowdin

sure but they have to go via (J)Text right? Crowdin only takes the ini files for translation

avatar dgrammatiko
dgrammatiko - comment - 3 Oct 2020

sure but they have to go via (J)Text right? Crowdin only takes the ini files for translation

AFAIR Joomla has it's own translation for tinyMCE: https://github.com/joomla/joomla-cms/tree/4.0-dev/build/media_source/vendor/tinymce/langs

BTW there should be an npm package for these...

avatar zero-24
zero-24 - comment - 3 Oct 2020

AFAIR Joomla has it's own translation for tinyMCE: https://github.com/joomla/joomla-cms/tree/4.0-dev/build/media_source/vendor/tinymce/langs

Ok so the mention stings needs to be added to that original file and than pushed to the translators right?

avatar dgrammatiko
dgrammatiko - comment - 3 Oct 2020

Ok so the mention stings needs to be added to that original file and than pushed to the translators right?

Im not sure how these files are generated, I think there is a repo somewhere

avatar zero-24
zero-24 - comment - 3 Oct 2020

Im not sure how these files are generated, I think there is a repo somewhere

I'm not aware of any maybe @infograf768 or @Bakual can help here?

avatar Bakual
Bakual - comment - 4 Oct 2020

The tinyMCE translations are the official translations from tinyMCE I think it's on Transifex.
IIRC the TTs provide them to us and we manually update them when needed.

avatar zero-24
zero-24 - comment - 4 Oct 2020

Ok so what is the process to add new language strings to that project. I dont think we can push them via tinyMCE's Transifix :D

avatar Bakual
Bakual - comment - 4 Oct 2020

I haven't looked at this PR in detail. It sounds as the string isn't used by the editor itself? So those files would be the wrong place anyway. They can be loaded by our plugin with the INI file like we do in other places of the CMS.

avatar zero-24
zero-24 - comment - 4 Oct 2020

Thanks @Bakual

@dgrammatiko can you please update the code for that and load the strings from the ini files / via (J)Text?

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2020

@dgrammatiko can you please update the code for that and load the strings from the ini files / via (J)Text?

I'll do that in a bit. But if Joomla is using the default TinyMCE translations why do we have the files in the build folder instead of fetching them from their repo? I think something is of here. I still remember ppl telling me to use OUR files instead of the official ones

avatar dgrammatiko
dgrammatiko - comment - 4 Oct 2020

@zero-24 done

avatar zero-24
zero-24 - comment - 4 Oct 2020

@zero-24 done

Thanks!

I'll do that in a bit. But if Joomla is using the default TinyMCE translations why do we have the files in the build folder instead of fetching them from their repo? I think something is of here. I still remember ppl telling me to use OUR files instead of the official ones

TBH I dont know but I would say this is out of scope for that PR, can you open an issue for that? Do you have a suggestion where should it be better placed?

avatar Quy Quy - test_item - 5 Oct 2020 - Tested successfully
avatar Quy
Quy - comment - 5 Oct 2020

I have tested this item ✅ successfully on ac18bf2


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

avatar brianteeman
brianteeman - comment - 5 Oct 2020

@dgrammatiko can you please update the code for that and load the strings from the ini files / via (J)Text?

I'll do that in a bit. But if Joomla is using the default TinyMCE translations why do we have the files in the build folder instead of fetching them from their repo? I think something is of here. I still remember ppl telling me to use OUR files instead of the official ones

Historically that was because the TT said that the quality was very bad

avatar dgrammatiko
dgrammatiko - comment - 11 Oct 2020

@wilsonge can we have a verdict on this one?

7ec117e 17 Oct 2020 avatar dgrammatiko CS
dd54a9e 17 Oct 2020 avatar dgrammatiko xxx
43695fa 17 Oct 2020 avatar dgrammatiko CS
649f7ac 17 Oct 2020 avatar dgrammatiko nope
de12dfe 17 Oct 2020 avatar dgrammatiko CS
8d25203 17 Oct 2020 avatar dgrammatiko 2020
04ca61b 17 Oct 2020 avatar dgrammatiko CS
avatar zero-24
zero-24 - comment - 17 Oct 2020

@dgrammatiko I'm sorry that it took me that long to come back to you on this. It seems there are conflicts can you take a look? Will now test with the latest build from drone and log my results here.

avatar zero-24
zero-24 - comment - 17 Oct 2020

Ah seems my message and your merge just crossed. Thanks! :D

avatar dgrammatiko
dgrammatiko - comment - 17 Oct 2020

It's not ready yet, some dates are out of sync, gimme 5 mins

avatar dgrammatiko
dgrammatiko - comment - 17 Oct 2020

Pfff, this is ready for testing again...

avatar wilsonge
wilsonge - comment - 17 Oct 2020

Hm so from my perspective this functionally works. Two things. One major the other less so:

Major issue: Fulltext images no longer scale to fit their text area

Screenshot 2020-10-17 at 12 50 13

Screenshot 2020-10-17 at 12 49 47

Here's a before and after patch (this is because we're hardcoding the width/height attributes). This was a screenshot of my large monitor - obviously however this would cause issues if i had a more regular size image but wanted to render it on mobile.

More minor thing (not sure if this is a bug - or just something we need to document now): It's now impossible to insert an image without an alt text when using the media manager modal/drag n' drop. It always gets an empty alt attribute (or correctly the one inserted) - before obviously it was impossible to have an alt text

avatar dgrammatiko
dgrammatiko - comment - 17 Oct 2020

Major issue: Fulltext images no longer scale to fit their text area

As I stated in the documentation section of the description:

Provide some guidelines for styling the lazy loaded images, basically explain how to use the css property object-fit: scale-down, cover, fill, etc https://caniuse.com/object-fit to get the same responsive behaviour as without defined width,height

Basically this can be fixed with a one line of css (eg object-fit: scale-down targeting the specific class). The reason I din't introduced this was because people might have different tastes for this (the behaviour is quite different between fill, scale-down etc)

before obviously it was impossible to have an alt text

Actually there was a request from the a11y people to have always an empty alt tag, (that was before this PR introduces the ability to insert some alt text). So all I did was following their request. ref: #28928 (comment)

EDIT: Probably we can do something like:

[loading="lazy"] {
  object-fit: scale-down;
}

If someone can confirm that the specificity is ok (eg a class selector will override this). @ciar4n ?

avatar dgrammatiko
dgrammatiko - comment - 17 Oct 2020

@wilsonge there's an easier solution:

[loading="lazy"] {
  width: 100%;
  height: auto;
}

Live demo: https://codepen.io/dgrammatiko/pen/WNxwYRR

avatar brianteeman
brianteeman - comment - 17 Oct 2020

Actually there was a request from the a11y people to have always an empty alt tag,

I think there is some confusion here. An image should always have an alt description unless it is purely decorative. Only if it is purely decorative should you use alt="". so almost always an image should have an alt description.

The problem with defaulting to "" if there is no alt description entered is that this mistake of not having a description is now hidden from any accessibility checker and the user will think erroneously that they are creating accessible content.

It should be a conscious decision to have an alt empty (alt="") and not a default.

In some other CMS this is done by having a checkbox like in this mockup

image

avatar dgrammatiko
dgrammatiko - comment - 17 Oct 2020

It should be a conscious decision to have an alt empty (alt="") and not a default.

@brianteeman the issue raised was about the drag n drop in tinyMCE and when I was reworking the broken code #28928 (comment) @chmst asked me to default to empty tag. That was before this PR. With this PR we have a modal asking for some alt text (which can be left empty and thus we end up with an empty alt tag). If you want me to add one more check box that will remove also the empty tag I can do that, although I don't think it's advisable...

avatar brianteeman
brianteeman - comment - 17 Oct 2020

I do not agree that we should default to an empty tag ie alt="" for the reasons I stated before

avatar dgrammatiko
dgrammatiko - comment - 17 Oct 2020

I do not agree that we should default to an empty tag ie alt="" for the reasons I stated before

But we are not, this was the case before this PR. The empty tag is getting rendered only if a user will not supply some alt text. I think my bad explanation was caused the confusion here...

avatar brianteeman
brianteeman - comment - 17 Oct 2020

we are defaulting to alt="" - I checked
A user usually does not supply some text because they are lazy or do not realise the importance of it.

If they then run an accessibility check on their site they will not be notified about a missing alt text on an image so they will never learn to improve and add the text

Using alt="" should be a conscious decision and not a default

avatar dgrammatiko
dgrammatiko - comment - 17 Oct 2020

Using alt="" should be a conscious decision and not a default

It seems that the a11y people need to come up with one solid answer here, I can not satisfy different and contradictory requirements. Anyways, if this is an issue please provide a workflow (that people will not blame me for introducing it) and I'll code it

avatar brianteeman
brianteeman - comment - 17 Oct 2020

Well I have suggested a workflow in the image above.

avatar dgrammatiko
dgrammatiko - comment - 17 Oct 2020

Well I have suggested a workflow in the image above.

We are talking about different things then. The issue George raised was about these 2 cases:
Screenshot 2020-10-17 at 15 16 40

Screenshot 2020-10-17 at 15 17 01

So basically George me and Christiane we are talking about content images and you are talking about the media field...

avatar brianteeman
brianteeman - comment - 17 Oct 2020

No that was just an example. The point about the alt="" being a bad default is valid everywhere

avatar brianteeman
brianteeman - comment - 17 Oct 2020

In other words this line of code is wrong
const altValue = alt="${dialogData.altText || ''}";

avatar dgrammatiko
dgrammatiko - comment - 17 Oct 2020

In other words this line of code is wrong

This will only append alt tag if there is some text

const altValue = `${dialogData.altText ? 'alt="${dialogData.altText}"' : ''`;

But this will not enforce alt tags and realistically will keep the majority of sites produced with Joomla to fail any a11y test ( we all know that people don't write alt texts)

avatar brianteeman
brianteeman - comment - 17 Oct 2020

It forces an empty alt description which is wrong. They should fail. We want them to fail. Only then will people start to make their sites accessible and enter alt text

avatar dgrammatiko
dgrammatiko - comment - 17 Oct 2020

It forces an empty alt description which is wrong. They should fail. We want them to fail. Only then will people start to make their sites accessible and enter alt text

I will not implement anything more here, you (or the a11y team) are welcome to refactor the code to fit your needs. Putting myself between arguing concepts never had a good outcome for me...

avatar zero-24
zero-24 - comment - 17 Oct 2020
[loading="lazy"] {
  width: 100%;
  height: auto;
}

hmm this seems to overwrite the inline width and heigth settings?

before that CSS

image

after that CSS

image

[loading="lazy"] {
object-fit: scale-down;
}

with that it looks better but for some reasone there is now an empty gap?

image

I will not implement anything more here, you (or the a11y team) are welcome to refactor the code to fit your needs. Putting myself between arguing concepts never had a good outcome for me...

Agree. As this is an topic before this PR here and also beyond this PR I would suggest to branch that issue out to its own issue so we can discuss all sites and than implement one solution for all cases.

avatar brianteeman
brianteeman - comment - 17 Oct 2020

Agree. As this is an topic before this PR here and also beyond this PR I would suggest to branch that issue out to its own issue so we can discuss all sites and than implement one solution for all cases.

Sorry but that is not true. This is a change in behaviour introduced in this PR

avatar zero-24
zero-24 - comment - 17 Oct 2020

Sorry but that is not true. This is a change in behaviour introduced in this PR

Well agree but it introduced the same behavior than we have in other places here too (when there is nothing we add alt=""). To change that i would suggest to do a dedicated issue so this can be adressed at all places.

That are both screenshots without this PR here applied

image

image

avatar brianteeman
brianteeman - comment - 17 Oct 2020

Those are all correct uses.
This is NEW behaviour being added

avatar zero-24
zero-24 - comment - 17 Oct 2020

Those are all correct uses.

You disagree your self, by suggesting this checkbox a few posts above right? #30784 (comment)

This is NEW behaviour being added

Yes but implmented exactly in the same way than the image / media picker works right now. No alt text = alt=""
image

So my suggestion is when that behavior should be changed (e.g. adding that tick box as in your scteenshot: #30784 (comment)) than that should be done in a dedicated Issue / PR in my opinion.

avatar brianteeman
brianteeman - comment - 17 Oct 2020

Our bad that neither I nor the accessibility team spotted that error. Its a good example of what I am saying about the error hiding the problem with an accessibility check and the only way to find it is to read all the code but PLEASE dont introduce another one.

avatar zero-24
zero-24 - comment - 17 Oct 2020

Our bad that neither I nor the accessibility team spotted that error. Its a good example of what I am saying about the error hiding the problem with an accessibility check and the only way to find it is to read all the code but PLEASE dont introduce another one.

It is there since 3.x? https://github.com/joomla/joomla-cms/blob/staging/layouts/joomla/content/full_image.php#L21

So can we please agree that this is a general problem to be discussed and fixed but nothing that should hold back this PR (lazyloading) as this here does exactly that what we do in all other places of the current code base too.

avatar brianteeman
brianteeman - comment - 17 Oct 2020

if only we had a group of people who checked accessibility

avatar zero-24
zero-24 - comment - 17 Oct 2020

if only we had a group of people who checked accessibility

In this specific case it was even a member of the accessibility team who suggested that change, right? So they even have checked this PR. I can not comment on what is the right solution but please when that is an issue that should be fixed lets move this discussion to a dedicated issue, involve that team and please unblock this PR.

avatar dgrammatiko
dgrammatiko - comment - 18 Oct 2020

Update:

  • with commit 044c91b I reverted inserting an empty alt tag is no text provided.

  • About the insertion or not or whatever, please create an issue, I'm not willing to solve this here, just because one person is objecting...

  • About the styling, @zero-24 the selector definitely shouldn't be global, the example given code above should be way more specific on what images will be targeting (intro, fulltext, content images [inside the content area], etc). This needs to be solved in the template and as I stated before I don't won't to preoccupy people with any solution, I just provided some possible ways to tackle this

  • @wilsonge this PR already revealed some mishaps in the Media manager emitted events, some styling issues and some a11y. Realistically these can not be tackled in one PR. I already provided the code to fix MM and have more code about the media field but all that is stack awaiting this one to be merged. So can we either bite the bullet and then start fixing all the remaining issues as follow ups or close this?

avatar brianteeman
brianteeman - comment - 18 Oct 2020

@dgrammatiko thank you
I already created the issue this morning #31138

avatar dgrammatiko
dgrammatiko - comment - 25 Oct 2020

@wilsonge @zero-24 is there any decision on this one?

avatar zero-24
zero-24 - comment - 25 Oct 2020

@wilsonge @zero-24 is there any decision on this one?

You have my full support for this PR to get it merged. 👍

avatar wilsonge
wilsonge - comment - 1 Nov 2020

I think this is fine. I don't love the concept of having the query params in the saved item - I'd rather have it saved directly into the JSON blob in the DB - but modifying JForm to validate this sounds problematic - so I guess it will have to do. Can we get a couple of tests here and I'm happy for this to be merged - as we're fixing several issues in the existing lazy loading implementation

avatar zero-24 zero-24 - test_item - 1 Nov 2020 - Tested successfully
avatar zero-24
zero-24 - comment - 1 Nov 2020

I have tested this item ✅ successfully on 182a275

Logging my test from above here. Thanks a lot @dgrammatiko 👍


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

avatar SharkyKZ
SharkyKZ - comment - 1 Nov 2020

but modifying JForm to validate this sounds problematic

It's not problematic. Only need to add width/height fields to the form and get their IDs in media field.

avatar zero-24
zero-24 - comment - 1 Nov 2020

It's not problematic. Only need to add width/height fields to the form and get their IDs in media field.

Well than lets please fix this in a follow up PR so we get the baseline working and than can improve the other parts too.

avatar dgrammatiko
dgrammatiko - comment - 2 Nov 2020

It's not problematic. Only need to add width/height fields to the form and get their IDs in media field.

Unless I'm totally wrong I think it's kinda hard to have one form field to spawn different values (eg value for the srcand also values for width and height). If you have a solution that the hidden fields for width and height are part of the media field I'm happy to include it here

avatar SharkyKZ
SharkyKZ - comment - 3 Nov 2020

Why does this have to be one field? And why would width/height fields be hidden?

avatar dgrammatiko
dgrammatiko - comment - 3 Nov 2020

And why would width/height fields be hidden?

Their value aren't up to the user to decide, it's coming from the actual image dimensions. Also adding more inputs increases frustration for users and destroys UX, especially if those inputs are just there to pass data (that is automatically filled) to the PHP...

Why does this have to be one field?

According to @wilsonge 's comment, to which I totally agree:

I'd rather have it saved directly into the JSON blob in the DB

Actually FWIW the media field is totally wrong. It should be a single module (eg the field has to control all the related inputs (alt, position, etc) that saves a JSON blob as George suggests but for me I think it should be either broken up to 4 fields (image, audio, video, document) to reflect all the possible media that the media manager can hold or as one field (probably less bloated) with a possible exported JSON blob like:

  {
    "type": "image", // or audio, video, document
    "extension": "webp",
    "src": "path/to/file.webp",
    "attributes": {
      "width": 600,
      "height": 300,
      "alt": "some alt text",
      "caption": "some caption text",
      // Attributes for audio/video/docs can be populated here
      "poster": "path/to/poster.jpg"
    }
  }

Of course for the front end this data needs to be coupled with the relevant jLayouts ( eg parent jLayout mediaField.php will switch to the correct children layout depending on the type)
Only problem with this approach is that is a really hard B/C break (although we can create a new field named, eg mediaObject) and deprecate the media field, although this realistically won't motivate the 3rd PD to embrace lazyloading. In sort there are many possible ways forward but the idea to have sparse, loosely related inputs that are up to 3rd PD to implement is not for the best interest of the project, it's totally against any theory of modularity and it's definitely not the way to implemented in 2020.

avatar SharkyKZ
SharkyKZ - comment - 4 Nov 2020

If we're adding image dimensions, there is no reason not to allow users to set them. Pre-populating the fields is fine. Forcing some arbitrary values is not.

We already store images as JSON in most places. So adding additional fields is not a problem and is in line with what we already have. This would be the cleanest, mostly B/C safe solution for the time being.

Something similar to an enhanced media already exists in Joomla\CMS\Form\Field\AccessiblemediaField, although not a great technical example. The problem with something like this is it would likely be too opinionated. For instance, your given example doesn't contain a class attribute which we do need in some places. And it has a caption which we may not need in other places. And making available attributes configurable just adds more complexity for the devs.

avatar dgrammatiko
dgrammatiko - comment - 4 Nov 2020

If we're adding image dimensions, there is no reason not to allow users to set them.

In the same pattern then someone can select a file test.jpg and then could change the extension to .png and that file might not exist and you get a broken field. The dimensions are not editable from the user, their data is extracted from the actual image size. Allowing this to be manipulated from users is totally stupid, adds more frustration, destroys UX and finally could have a disastrous effect if misused.

We already store images as JSON in most places. So adding additional fields is not a problem and is in line with what we already have. This would be the cleanest, mostly B/C safe solution for the time being.

What exists in the db right now is something that designed many many years ago. Keep building on top of a wrong foundation is not a good call. Just a reminder the JSON that exists in the db fields comes from an era that mysql didn't have support for json columns and most importantly Joomla didn't have Custom Fields...

For instance, your given example doesn't contain a class attribute which we do need in some places. And it has a caption which we may not need in other places.

Disagree. Class is something I omitted (I did omit a lot of the possible data there because that was a quick example not an exhaustive list) but this and many others could/should be there. Also if the field is cleverly coded should allow a great degree of customisability.

And making available attributes configurable just adds more complexity for the devs.

Safe defaults and documentation is my answer here.

Anyways I think that whatever the next steps are this is good enough for the B/C part of the media field. If a new concept comes it can coexist and the media field can be deprecated. But all this conversation, I think it needs to move away from this issue. The current implementation brings lazyloading to the current media field (which is opt out, for those that want to) in a way that is globally available. And to close this I urge you to search for type="media" and then check how many of these fields have a JSON representation in the db. With your idea to add sparse, unrelated fields for the dimensions we have to fix a lot of these fields. So, basically what you're suggesting is not B/C (in the sense that it doesn't work without further work) and needs a lot of work to be implemented and finally, doesn't really motivate 3rd PD to move there (nobody will go and add random fields in their forms, that's a fact)
Screenshot 2020-11-04 at 12 20 15

avatar SharkyKZ
SharkyKZ - comment - 5 Nov 2020

In the same pattern then someone can select a file test.jpg and then could change the extension to .png and that file might not exist and you get a broken field. The dimensions are not editable from the user, their data is extracted from the actual image size. Allowing this to be manipulated from users is totally stupid, adds more frustration, destroys UX and finally could have a disastrous effect if misused.

All of this is wrong. I'm just going to repeat myself. Pre-populating values is fine, forcing them is not. Users may use dimension attributes to scale images or omit them altogether. This is not up to you to decide.

Keep building on top of a wrong foundation is not a good call.

This is better than introducing a new, out of place data structure. The upgrade path to proposed JSON structure is simpler if we keep using what we have now and the code is more performant. By adding attributes to URLs you'd have to both map existing fields and keep parsing URLs, unless someone decides that we should update user data to match new structure on upgrade to major version.

So, basically what you're suggesting is not B/C (in the sense that it doesn't work without further work)

That's not what B/C means at all. Having to add something to make a new feature work is not a B/C break. B/C here concerns existing user data. That said, adding new fields would be B/C safe in most places. Admittedly, this PR as it stands is mostly B/C too. But, in your own words, building on top of a wrong foundation is not a good call.

avatar infograf768
infograf768 - comment - 6 Nov 2020

@dgrammatiko
also #31319 is a release blocker as alt, float and caption are missing

avatar dgrammatiko
dgrammatiko - comment - 6 Nov 2020

also #31319 is a release blocker as alt, float and caption are missing

This PR solves the ALT part

avatar dgrammatiko
dgrammatiko - comment - 16 Nov 2020

@wilsonge any decision on this one?

avatar wilsonge wilsonge - close - 17 Nov 2020
avatar wilsonge wilsonge - merge - 17 Nov 2020
avatar wilsonge wilsonge - change - 17 Nov 2020
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2020-11-17 18:18:26
Closed_By wilsonge
avatar wilsonge
wilsonge - comment - 17 Nov 2020

Yeah I still don't love it but i think it is a step in the right direction. FWIW i love the look of that generic json blob you posted.

Kinda agree with sharky we probably need to allow users to prepopulate the width/heights so they aren't maxed out. But I don't think that's a massive addon to this PR either.

avatar zero-24
zero-24 - comment - 17 Nov 2020

Massive thanks to @dgrammatiko and @wilsonge here 👍

avatar dgrammatiko
dgrammatiko - comment - 17 Nov 2020

Yeah I still don't love it but i think it is a step in the right direction

Well, I would also be happier with a json blob but we had a text field to play with. Maybe someone smarter than me could do the PHP part to allow json blob or fall back to the url text

Kinda agree with sharky we probably need to allow users to prepopulate the width/heights so they aren't maxed out

I disagree but I could implement this in a PR (we still miss class, caption, the new checkbox Brian added etc for parity with v3 and the other fields)

@zero-24 you're part of this the code for the custom fields was yours!!! Also not to mention that you were the one that triggered all these change, so big thanks

Also a big thank you to everybody involved here or any of the previous PRs

avatar dgrammatiko
dgrammatiko - comment - 18 Nov 2020

this PR already revealed some mishaps in the Media manager emitted events, some styling issues and some a11y.

For reference PRs #31427 and #31425 complete the remaining todos and #31667 fixes the display of the lazy loaded images for Cassiopeia

avatar dgrammatiko
dgrammatiko - comment - 24 Nov 2020

One extra bit here, the video Improving The Page Loading Experience To Reduce Layout Shift by Jen Simmons @jensimmons (webkit) https://www.youtube.com/watch?v=YM3KszYmn58 I've already included the slides but maybe video is more friendly

avatar infograf768
infograf768 - comment - 20 Dec 2020

@dgrammatiko
Could not find the use of Text::script('JFIELD_MEDIA_CONFIRM_TEXT');

avatar dgrammatiko
dgrammatiko - comment - 20 Dec 2020

@infograf768 good catch, that's redundant

avatar joeforjoomla
joeforjoomla - comment - 1 Feb 2021

This thing of adding a query string joomla_image_width=225&joomla_image_height=50 to all images across the whole CMS is obscene. I wonder how it is possible that this thing got merged. @dgrammatiko honestly i hope that you will stop soon to contribute to Joomla, i've no words for you.

avatar dgrammatiko
dgrammatiko - comment - 1 Feb 2021

@joeforjoomla what's wrong with the added params in the URL?

avatar richard67
richard67 - comment - 1 Feb 2021

honestly i hope that you will stop soon to contribute to Joomla, i've no words for you.

@joeforjoomla Regardless if your points are right or wrong, but this kind of personal attack is not right.

avatar gostn
gostn - comment - 2 Feb 2021

@joeforjoomla personal attacks are right if they come by @richard67.

avatar richard67
richard67 - comment - 2 Feb 2021

@gostn Where have I attacked you or someone else personally? Can you show me that?

avatar gostn
gostn - comment - 2 Feb 2021

I understand you can't remember because you decided, what you wrote was not a personal attack.

avatar infograf768
infograf768 - comment - 2 Feb 2021

I am now closing this to prevent further trolling and personal attacks.

Add a Comment

Login with GitHub to post a comment