NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar rjharishabh
rjharishabh
6 May 2021

Summary of Changes

Add Javascript code to change the other input slider accordingly

document.getElementById('jform_resize_w').value = parseInt(width, 10);
document.getElementById('jform_resize_h').value = parseInt(height, 10);

Testing Instructions

  • Dashboard > Content > Media > Try to edit an image using pencil icon > Go to Resize tab
  • Apply PR
  • Do npm i

Actual result BEFORE applying this Pull Request

resize-before

Expected result AFTER applying this Pull Request

media.mp4

Documentation Changes Required

Don't know

avatar rjharishabh rjharishabh - open - 6 May 2021
avatar rjharishabh rjharishabh - change - 6 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 6 May 2021
Category JavaScript Repository NPM Change
avatar rjharishabh rjharishabh - change - 6 May 2021
The description was changed
avatar rjharishabh rjharishabh - edited - 6 May 2021
avatar RickR2H RickR2H - test_item - 6 May 2021 - Tested successfully
avatar RickR2H
RickR2H - comment - 6 May 2021

I have tested this item successfully on ae90ba6

Confirmed bug and fix works!


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

avatar rjharishabh rjharishabh - change - 6 May 2021
Labels Added: NPM Resource Changed ?
avatar ChristineWk ChristineWk - test_item - 7 May 2021 - Tested successfully
avatar ChristineWk
ChristineWk - comment - 7 May 2021

I have tested this item successfully on ae90ba6


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

avatar richard67
richard67 - comment - 8 May 2021

@rjharishabh I have questions regarding this PR.

  1. Will it still be possible to resize the image without keeping the ratio, e.g. by directly entering the width or height value? If not, the PR breaks the possibility to do that.
  2. If the answer to 1 is "No" so it breaks that, would it be good to have a check box to decide if ratio should be kep or not when resizing?
  3. Has there been an issue for it? If so and you know it, please refer to it with the "Pull Request for Issue #x ." line, with x being the issue number.

Maybe question 1 and 2 were already discussed somewhere else, and I'm not aware of it. If you have any info about that, let us know.

avatar richard67
richard67 - comment - 8 May 2021

P.S.: If always both sliders mode, keeping the image ratio, and we can't enter one of the values directly without the other one being changed, too, so there is no way to resize without keeping the ratio, then it doesn't make sense to have 2 sliders. One slider for scaling 0 to 100 % would be enough in that case.

avatar richard67
richard67 - comment - 8 May 2021

@drmenzelit Can you check my above comments and the PR with respect to if it's desired functionality or not?

avatar brianteeman
brianteeman - comment - 8 May 2021

@chmst can you test this for accessibility as a value is being changed without it being announced (i think)

avatar rjharishabh
rjharishabh - comment - 8 May 2021

When we change the width (or height), then the other value will change but the range slider was not updated
So I created this PR to change the range slider accordingly

avatar richard67
richard67 - comment - 8 May 2021

When we change the width (or height), then the other value will change but the range slider was not updated

Ah, silly me, have not watched the value, only the slider, when watching the "before PR" video.

avatar rjharishabh
rjharishabh - comment - 8 May 2021

By directly entering the width or height value

It works

This PR just updates the range slider according to the corresponding width and height.

avatar rjharishabh rjharishabh - change - 8 May 2021
The description was changed
avatar rjharishabh rjharishabh - edited - 8 May 2021
avatar richard67
richard67 - comment - 8 May 2021

By directly entering the width or height value

It works

This PR just updates the range slider according to the corresponding width and height.

@rjharishabh Yes, I've noticed meanwhile. Only remaining question is if it is accessible or not, see Brian's question above, but if not, then it is also not accessible without this PR, I think, so that might be an issue to be fixed separately.

avatar rjharishabh rjharishabh - change - 8 May 2021
The description was changed
avatar rjharishabh rjharishabh - edited - 8 May 2021
avatar richard67
richard67 - comment - 8 May 2021

@brianteeman In case if the change of the number field of the other dimension already is announced by a screenreader without this PR, would it need to announce the same change on the other slider in addition? Or would that be redundant?

If redundant and the announcement of the number field is missing without this PR, then the PR is good as it is and we should open a new, separate issue for the missing announcement.

If not redundant, it should be added with this PR for the slider, if missing.

avatar chmst
chmst - comment - 8 May 2021

@brianteeman As much as I see, the PR is an improvment for sighted users - very nice - but changes nothing concerning accessibility.

avatar richard67 richard67 - change - 8 May 2021
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 8 May 2021

RTC


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

avatar brianteeman
brianteeman - comment - 8 May 2021

I'm in the studio filming today so couldn't check myself

avatar brianteeman brianteeman - test_item - 8 May 2021 - Tested unsuccessfully
avatar brianteeman
brianteeman - comment - 8 May 2021

I have tested this item ? unsuccessfully on ae90ba6


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

avatar brianteeman
brianteeman - comment - 8 May 2021

Now that I am in front of a computer and able to test - this is not correct.

The slider is an input tool not an indicator. It is correct that the slider for height doesnt change when the slider for width is used.

It is the output of the field that changes not the slider. So everything is working correctly without this pr

avatar richard67 richard67 - change - 8 May 2021
Status Ready to Commit Pending
avatar richard67 richard67 - change - 8 May 2021
Status Pending Needs Review
avatar rjharishabh
rjharishabh - comment - 8 May 2021

The slider is an input tool not an indicator.

Yes,
But here both the width field and the corresponding slider works as input and both should correspond to the same value because both for width( or height) but using different input types

avatar rjharishabh
rjharishabh - comment - 8 May 2021

Both slider and field have value and it should be same because the fields are same

avatar chmst
chmst - comment - 8 May 2021

@brianteeman I see this as n improvement. Values are the same with or without PR. Only the slider position is visible corresponding to the other value.

avatar brianteeman
brianteeman - comment - 8 May 2021

agree to disagree then. the slider is not an indicator of anything and shouldnt be used as one.

avatar richard67
richard67 - comment - 8 May 2021

agree to disagree then. the slider is not an indicator of anything and shouldnt be used as one.

@brianteeman Does that justify an unsuccessful test result?

avatar brianteeman
brianteeman - comment - 8 May 2021

imho yes. this is wrong. can you find an example somewhere of anything else doing this

avatar angieradtke
angieradtke - comment - 9 May 2021

Hi,
General a11y issue:
While two values will be changed, the big question is where the focus is at which moment. I think this issue is not easy to solve, because assestive technologie can't really deal with this kind of technique. A potential possibility could be a script that give the value of both fields to a live region, which gets the focus when one value is changed. After that assestive technologie needs a skiplink to jump back. But what is the right field? Width or height? I thing to make this depenencies accessible is not so easy.

This pull request does't effect that issue, but is helpful for the Usability, because it leads to a consistent behavior, I think.

avatar brianteeman
brianteeman - comment - 9 May 2021

Consistent with what?

I havent been able to find another example of this type of behaviour anywhere (except where the position on the range slide indicates a value which it doesn't here)

avatar chmst
chmst - comment - 9 May 2021

@angieradtke you are right, this is an issue and a challenge, and I have no solution. My opinion is, as I said - the PR is an improvment which changes nothing for accessibility.

@brianteeman - for me it is consistent to see the slider on the place where it matches the number in the field.

avatar richard67
richard67 - comment - 9 May 2021

@brianteeman I can tell you an example where the slider is not used for doing the input but is adjusted to the input in another field: It's the hue setting in Atum's template style settings. Ok, there it is only one number value and one slider. But it is what you complain about. The slider is a pure input, but when you enter a valid number in the number field and use the enter key, the slider moves to that value. I think you have seen that many times recently when testing the admin UI repaint. Here with 2 sliders it is nothing else: The number field of the other dimension is updated, and so should be the corresponding slider.

avatar brianteeman
brianteeman - comment - 9 May 2021

If the position of the slider matched the value (like it does with the hue in atum) then I would agree 10000%
In this case it does not. It is just used as a means of changing the number and nothing else.
again I say - show me an example of where the behaviour this PR creates is used. Not just joomla but anywhere

avatar rdeutz rdeutz - close - 9 May 2021
avatar rdeutz rdeutz - merge - 9 May 2021
avatar rdeutz rdeutz - change - 9 May 2021
Status Needs Review Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-05-09 11:44:03
Closed_By rdeutz
avatar brianteeman
brianteeman - comment - 9 May 2021

so sad that this has been merged - its completely wrong and no one has shown any example anywhere in anything where this incorrect behaviour is used.

avatar rjharishabh
rjharishabh - comment - 9 May 2021

Don't know why @rdeutz merged this

avatar richard67
richard67 - comment - 9 May 2021

Maybe because it's an improvement which makes it work like people expect it to work, and feedback here from all others except of Brian was positive?

Just my guess.

avatar angieradtke
angieradtke - comment - 10 May 2021

Dear Brian , @brianteeman
I haven't quite understood your point of criticism yet.
These sliders are not suitable as a precise value transfer, because fine tuning with them is difficult. Nevertheless they are practical because you can quickly switch between value extremes ( very high/ very low) and in this case you can see the changes visually right away. Switch and input field in combination are very useful for sighted users in this case and complement each other. Therefore, the mutual value transfer makes sense here.

In terms of accessibility, only one input field would be better. However, this would affect the usability for sighted people.

avatar brianteeman
brianteeman - comment - 10 May 2021

OK let me try again.

In Joomla 4 we are using sliders as an input method in two places.

The first is for setting the hue of the admin template
image

Here the slider does TWO things. It's an indicator of value and an input tool

  1. it is the handle to select and change the value displayed as text
  2. it is a visual indicator of the value as indicated by the position of the handle in the slider. After saving a change you will see that the position of the handle still shows the position of the value in the range

The second use is for editing images
image

Here the slider does ONE thing. Its an inaccessible input tool

  1. it is the handle to select and change the value displayed as text. After saving a change you will see that the position of the handle is back at the same starting position.

If changing the position of the height slider had a purpose other than being a handle then I can see why you would want to move the width slider if it had a purpose. such as displaying the value

before

But in this case moving the width slider when you move the height slider serves no purpose as its not an indicator of anything. As I have repeatedly asked and no one has responded. Please show me an example anywhere of this behaviour.

All of the above is completely avoiding saying

  1. that this slider is coded in a way that is completely inaccessible when used together with an input box. Just test it with a screen reader
  2. the slider can only be used to reduce the size of the image

Final demo showing that the position of the slider handle has no visual purpose
final

avatar richard67
richard67 - comment - 10 May 2021

@brianteeman With your more detailed explanation I understand now. Sorry if I'm a bit slow with that. We're discussing on Glip what we should do, just revert this PR, or remove the sliders completely because they are not accessible. What would you suggest to do?

avatar brianteeman
brianteeman - comment - 10 May 2021

Step 1
Revert this PR

Step 2
Change the slider inputs so that they work (can be used to increase as well as reduce the size) and make it accessible.

Personally I would kill the slider

Note 1. there is the reverse issue with the rotate plugin. If you press an angle button then the text in in the input updates. But if you change the text in the field the button doesn't update

Note 2. the crop plugin doesnt use sliders

avatar brianteeman
brianteeman - comment - 10 May 2021

To be absolutely clear. the range slider is being used completely wrong here. It is called a range slider because it has a minimum and a maximum value. But you dont want it to be limited with a maximum value or you cant enlarge the image. The only type of slider you could have is one that had a linear scale. But they suck. This is why you hardly ever see sliders for image resize. Thats why I kept asking to be shown an example

avatar chmst
chmst - comment - 10 May 2021

After sleeping over it - I agree. Let' remove the slider.

avatar richard67
richard67 - comment - 10 May 2021

To be honest: I haven't found the example Brian asked me to look for.

And I can live without the sliders.

So gor for it.

avatar brianteeman
brianteeman - comment - 10 May 2021

you cant find what doesnt exist ;)

creating a pr

avatar richard67
richard67 - comment - 10 May 2021

@brianteeman I remembered having seen sliders for image resizing a long time ago, maybe end of the 1990's on Windows 3.11 or so, in some program, but when I check all kinds of programs now where you can resize or scale images I don't find any sliders at all. The all have number fields for entering height and width, and a check box if aspect ratio to be kept or not. So maybe I should forget about the old times ;-)

avatar brianteeman
brianteeman - comment - 10 May 2021

or perhaps your memory is thinking of something else ;)

avatar brianteeman
brianteeman - comment - 10 May 2021

See #33732

Add a Comment

Login with GitHub to post a comment