Skip to content

aria hidden #137

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 1, 2020
Merged

aria hidden #137

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/js/tab/tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@
// Fallback if no active tab
if (!this.hasActive) {
tabsEl[0].setAttribute('active', '');
Copy link
Contributor

@wilsonge wilsonge Feb 29, 2020

Choose a reason for hiding this comment

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

Assume you've tested this - because right now I'm extra confused on code review about the difference between this line and the line below on 145. As a result not sure if this belongs here or in that querySelector section :/

Copy link
Contributor Author

@brianteeman brianteeman Mar 1, 2020

Choose a reason for hiding this comment

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

I couldnt work it out either. did it all by trial and error

from what I could tell when we dont have an active tab defined on load the code here just added "active"
we have code elsewhere that puts ariahidden on everything
It is only removed when you have a real active tab hence this code

Copy link
Collaborator

@dgrammatiko dgrammatiko Mar 1, 2020

Choose a reason for hiding this comment

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

FWIW these lines till this.hasActive = true; are a fallback in case the backend throws some markup without any tab active. The lines starting from 142 also are referring to the links and the lines before to the tab content, so it's a sync dance

Copy link
Contributor Author

@brianteeman brianteeman Mar 1, 2020

Choose a reason for hiding this comment

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

it always opens without an active tab unless one has been saved in the session and a tab with that name exists or the url included the #tab fragment

Copy link
Collaborator

@dgrammatiko dgrammatiko Mar 1, 2020

Choose a reason for hiding this comment

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

it always opens without an active tab

IIRC there is some code that sets the active attribute in one of the 2 relative layouts. Maybe there's a bug there. Normally the active tab should be set in the backend, the code for the hash or the localsorage should be applied after this part as it's a client related code

Copy link
Contributor Author

@brianteeman brianteeman Mar 1, 2020

Choose a reason for hiding this comment

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

I dont think there can be or this section of the js would never have been needed

Copy link
Collaborator

@dgrammatiko dgrammatiko Mar 1, 2020

Choose a reason for hiding this comment

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

There is: https://github.com/joomla/joomla-cms/blob/6bffa56d6eab72555d941ff59a908ce9e377b7e2/libraries/cms/html/uitab.php#L87

The fallback is there because there should always be a fallback. Also I think that this parts reverts the active tab form the hash, if I read the code correctly (eg no active code from the backend but hash in the url => the hash will not be respected, order of execution)

Copy link
Collaborator

@dgrammatiko dgrammatiko Mar 1, 2020

Choose a reason for hiding this comment

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

Ah I already see the bug: JHtmlUiTab that will not compute it needs to be HTMLHepler or better __METHOD__ as few lines above

Copy link
Contributor

@wilsonge wilsonge Mar 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

__METHOD__ won't work as we need it to check for the startTabSet method - not the addTab method. We'd need to use __CLASS__ i guess

Copy link
Contributor

@wilsonge wilsonge Mar 1, 2020

Choose a reason for hiding this comment

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

joomla/joomla-cms#28179 PR - but I don't think this will fix it - __METHOD__ will still use the JHtmlUiTab class I think as it's not namespaced yet. But it's good practice to use __CLASS__ anyhow as we will need to namespace at some point. If it fixes it all the better

tabsEl[0].removeAttribute('aria-hidden');
this.hasActive = true;
this.currentActive = tabsEl[0].id;
this.querySelector(`#tab-${tabsEl[0].id}`).setAttribute('aria-selected', 'true');
Expand Down