? NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar khu5h1
khu5h1
26 Oct 2021

Pull Request for Issue #35229 .

Summary of Changes

I have enabled focus on media files by changing their tabindex from -1 to 0.

Testing Instructions

Check out issue #35229 for testing and reproducing the PR.

Actual result BEFORE applying this Pull Request

Unable to focus on media files while using the tab button.

Expected result AFTER applying this Pull Request

Enabled to focus in media files using the tab button.

avatar khu5h1 khu5h1 - open - 26 Oct 2021
avatar khu5h1 khu5h1 - change - 26 Oct 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 26 Oct 2021
Category Administration com_media NPM Change
avatar manav014 manav014 - test_item - 28 Oct 2021 - Tested successfully
avatar manav014
manav014 - comment - 28 Oct 2021

I have tested this item successfully on 4bd85f5


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

avatar Fedik
Fedik - comment - 29 Oct 2021

@khu5h1 as I see from code you change tabindex to -1 when isActive that meant tab index is disabled.
the fix does not looks right, there probably error in another place

avatar brianteeman
brianteeman - comment - 29 Oct 2021

Does it even need a tabindex - ever?

avatar Fedik
Fedik - comment - 29 Oct 2021

Does it even need a tabindex - ever?

If it non interactive element (like a, button, input), and need "tab navigation" then yes.

avatar Fedik
Fedik - comment - 29 Oct 2021

@khu5h1 change your code to:

getTabindex() {
  return  0;
}

Then all be good

avatar brianteeman
brianteeman - comment - 29 Oct 2021

If it non interactive element (like a, button, input), and need "tab navigation" then yes

buttons and inputs are interactive

avatar Fedik
Fedik - comment - 29 Oct 2021

buttons and inputs are interactive

yes, I meant: .. if not like a button or input ;)

avatar richard67
richard67 - comment - 29 Oct 2021

@khu5h1 change your code to:

getTabindex() {
  return  0;
}

Then all be good

@Fedik Should that also be done at the other place which I‘ve mentioned in my previous comment?

avatar Fedik
Fedik - comment - 29 Oct 2021

there can stay as is, if it works

avatar khu5h1 khu5h1 - change - 29 Oct 2021
Title
Enabled to focus in media files
[4.1] Enabled to focus in media files
avatar khu5h1 khu5h1 - edited - 29 Oct 2021
avatar khu5h1 khu5h1 - change - 29 Oct 2021
Labels Added: NPM Resource Changed ?
avatar richard67
richard67 - comment - 29 Oct 2021

Silly question: Why is this PR targeted for 4.1? Is it a new feature? If it's a bug fix it should go into 4.0.

avatar khu5h1
khu5h1 - comment - 29 Oct 2021

Thank you @Fedik for your suggestion and I have committed the desired change.
And as per @richard67 's comment, I am not sure that whether we shall commit the change to drive.vue or not. As IMO that won't have any impact on the issue.
Regards

avatar khu5h1
khu5h1 - comment - 29 Oct 2021

Silly question: Why is this PR targeted for 4.1? Is it a new feature? If it's a bug fix it should go into 4.0.

I am not sure about that, If that's the case, I will have to rebase my PR to 4.0-dev. I may do that if that sounds good.

avatar khu5h1 khu5h1 - change - 29 Oct 2021
The description was changed
avatar khu5h1 khu5h1 - edited - 29 Oct 2021
avatar encrypt3r12 encrypt3r12 - test_item - 7 Nov 2021 - Tested successfully
avatar encrypt3r12
encrypt3r12 - comment - 7 Nov 2021

I have tested this item successfully on 7a8a0d3


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

avatar manav014 manav014 - test_item - 8 Nov 2021 - Tested successfully
avatar manav014
manav014 - comment - 8 Nov 2021

I have tested this item successfully on 7a8a0d3


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

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

RTC


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

avatar khu5h1 khu5h1 - change - 10 Nov 2021
Labels Added: ?
avatar richard67
richard67 - comment - 10 Nov 2021

@khu5h1 Why do you update your branch without any need? Any new commit resets the human test counter and we have to set the test result again then for the tests being properly counted. To update the branch is only needed when GitHub shows conflicting files.

avatar khu5h1
khu5h1 - comment - 10 Nov 2021

@richard67 I was not aware of the same. Thanks for your comment. I will keep this in mind for future PRs.

Also, I would request @encrypt3r12 and @manav014 to please test this PR again.

avatar encrypt3r12 encrypt3r12 - test_item - 10 Nov 2021 - Tested successfully
avatar encrypt3r12
encrypt3r12 - comment - 10 Nov 2021

I have tested this item successfully on cf80dbc


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

avatar khu5h1 khu5h1 - test_item - 10 Nov 2021 - Tested successfully
avatar manav014 manav014 - test_item - 10 Nov 2021 - Tested successfully
avatar manav014
manav014 - comment - 10 Nov 2021

I have tested this item successfully on cf80dbc


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

avatar khu5h1
khu5h1 - comment - 10 Nov 2021

Thanks, everyone :)

avatar brianteeman brianteeman - test_item - 10 Nov 2021 - Tested unsuccessfully
avatar brianteeman
brianteeman - comment - 10 Nov 2021

I have tested this item ? unsuccessfully on cf80dbc

Sorry this is completely wrong.

Firstly the expected behaviour for a tree view is to navigate between the tree items with the arrow keys and not the tab key

Secondly while this pr does appear to navigate between tree items with a tab it does not allow you to select a tree item using the keyboard

Thirdly If you have a nested tree then the tab key will select the entire nested tree as a single item


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

avatar richard67 richard67 - change - 10 Nov 2021
Status Ready to Commit Pending
avatar richard67
richard67 - comment - 10 Nov 2021

Back to pending after unsuccessful test.


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

avatar richard67
richard67 - comment - 10 Nov 2021

@richard67 I was not aware of the same. Thanks for your comment. I will keep this in mind for future PRs.

@khu5h1 Thank you. It was not a big problem, I only wanted to let you know that it was not necessary. And thanks for your contributions and testing other PR's. It's really appreciated, regardless of the unsuccessful test above (that can happen to anybody of us).

avatar khu5h1
khu5h1 - comment - 11 Nov 2021

Firstly the expected behaviour for a tree view is to navigate between the tree items with the arrow keys and not the tab key
Secondly while this pr does appear to navigate between tree items with a tab it does not allow you to select a tree item using the keyboard
Thirdly If you have a nested tree then the tab key will select the entire nested tree as a single item

@brianteeman Actually, I thought that the expected behavior is to navigate with tab key. I will modify this PR to make it compatible to move across items with arrow key. Also, I just realized that it is not allowing me to select nested tree items so that's also is to be done.

May anyone please suggest me some code pointers or references to solve the problem.

avatar khu5h1
khu5h1 - comment - 11 Nov 2021

@richard67 Thanks for the appreciation. I totally understand that it is common for tests to be failed and I will modify my PR accordingly to pass the tests.

avatar brianteeman
brianteeman - comment - 11 Nov 2021
avatar richard67 richard67 - change - 13 Nov 2021
Status Pending Needs Review
avatar richard67
richard67 - comment - 13 Nov 2021

Needs some more work.


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

avatar Quy
Quy - comment - 27 Feb 2022

Closing in favor of #37152. Thanks.

avatar Quy Quy - change - 27 Feb 2022
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2022-02-27 14:40:43
Closed_By Quy
Labels Added: ?
Removed: ?
avatar Quy Quy - close - 27 Feb 2022

Add a Comment

Login with GitHub to post a comment