Skip to content

Conversation

@brianteeman
Copy link
Contributor

when there is no active tab set we need to remove the attribute aria-hidden. Other wise the tab is invisible to a screen reader

PR for joomla/joomla-cms#28122

obviously this will need a new release after merging

when there is no active tab set we need to remove the attribute aria-hidden. Other wise the tab is invisible to a screen reader

PR for joomla/joomla-cms#28122

// Fallback if no active tab
if (!this.hasActive) {
tabsEl[0].setAttribute('active', '');
Copy link
Contributor

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

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

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

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

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

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

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

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

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

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

@wilsonge wilsonge merged commit 1f31ec2 into joomla-projects:master Mar 1, 2020
@brianteeman
Copy link
Contributor Author

great thanks - if you could merge #136 and do you a new release that would be awesome

@brianteeman brianteeman deleted the fallback branch March 1, 2020 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants