User tests: Successful: Unsuccessful:
Pull Request for Issue #30678 (second attempt) .
Updated html > layouts > chromes > card.php (renamed from 'default', see #30729) to display mod-id as the containing element's (address, div, section, etc) #id
instead of just in the title so that the id displays whether the title is shown or not.
module #id
only appears if title is shown
#id
always applied to Cassiopeia modules using Cassiopeia 'card' chromeNo visible difference on front end for website visitors
Note to highlight change
Status | New | ⇒ | Pending |
Category | ⇒ | Front End Templates (site) |
Labels |
Added:
?
|
Honestly, I don't think we need a new chrome "noTitle". Hiding a title is possible with each module chrome already. There is an option for just that.
Honestly, I don't think we need a new chrome "noTitle". Hiding a title is possible with each module chrome already. There is an option for just that.
In that case, I don't see the reason for having any of the modules as "style=none"
because:
a. those modules won't have an id added.
b. you can't add a module class suffix (or if you do, it's ignored). This could be confusing (as it was for me, initially) for someone who does not have the user access level to edit the source code but wants to add a known css class to a module.
Perhaps all the modules currently given "style=none"
in the index.php should be "style=default"
which would avoid the need for the new 'noTitle' chrome.
In that case, I don't see the reason for having any of the modules as "style=none" because:
a. those modules won't have an id added.
b. you can't add a module class suffix (or if you do, it's ignored). This could be confusing (as it was for me, initially) for someone who does not have the user access level to edit the source code but wants to add a known css class to a module.
That's exactly the purpose of "none". To have only the module output without any chrome around it.
If you need the id or suffix, you can easily change the module style (= chrome) to another one of your choice in the module options.
The chrome specified in the index.php is just the default chrome to be used. It can be overriden in the module options.
And yes, "default" is a bad and misleading choice as a chrome name.
That's exactly the purpose of "none". To have only the module output without any chrome around it.
If you need the id or suffix, you can easily change the module style (= chrome) to another one of your choice in the module options.
Yes, of course. I haven't used that way as I've always other methods, so I forgot. Sorry.
The chrome specified in the index.php is just the default chrome to be used. It can be overriden in the module options.
And yes, "default" is a bad and misleading choice as a chrome name.
Perhaps 'default' should be renamed as 'unstyled'.
Perhaps 'default' should be renamed as 'unstyled'.
Nah, unstyled is wrong as well, as there certainly are styles applied. Actually "default" applies a "card" styling. And I don't even see the difference to "cardGrey".
But I leave that to others who have more experience with design.
Nah, unstyled is wrong as well, as there certainly are styles applied. Actually "default" applies a "card" styling. And I don't even see the difference to "cardGrey".
"cardGrey" has a light grey background in the class "card-header". "card-header" is assigned to the default.php as well, but doesn't seem to be applied. I can't work out why. I can't see any other differences.
But I leave that to others who have more experience with design.
Ok. What's wrong with my code this time?
Edit: I've lost the aria-label and aria-labelledby
I'll put them back.
Ok. What's wrong with my code this time?
@Scrabble96 You can check that yourself. At the bottom of the PR where the tests are shown, use the "Details" link at the right side or a failed test. In this case drone failed. The "Details" link brings you to a page where on the left hand side is a list of test steps, failed ones in red. In your case now it is "PHPCS", which means PHP code style. Click this and you get on the right hand side the details on this page: https://ci.joomla.org/joomla/joomla-cms/35590/1/8. You will see that the problem is that you use spaces instead of tabs to indent certain lines.
So please follow my advise from above, set up your editor so you can see tabs and spaces and so can distinguish them. And in addition switch off any automatic indent or formatting in your editor (or IDE) settings, or adjust them in a way so that they meet the code style standards documented e.g. here for PHP: https://developer.joomla.org/coding-standards/php-code.html.
So please follow my advise from above, set up your editor so you can see tabs and spaces and so can distinguish them. And in addition switch off any automatic indent or formatting in your editor (or IDE) settings, or adjust them in a way so that they meet the code style standards documented e.g. here for PHP: https://developer.joomla.org/coding-standards/php-code.html.
Edit: if I use tabs and specify the number of spaces that the tab uses, in which case, is it 4? On looking at the original code, it looks like it. Thank you.
Can you please clarify whether I have to use tabs instead of spaces?
If I use four spaces instead of a tab, for instance, is that ok or should I use tabs and specify the number of spaces that the tab uses, in which case, is it 4? On looking at the original code, it looks like it. Thank you.
@Scrabble96 If the code style says you have to use tabs then you have to use tabs. 1 tab means one level of indent. 4 spaces are not the same as 1 tab, it only can be made that it looks the same by editor settings. The drone PHPCS log for which I have pasted the link https://ci.joomla.org/joomla/joomla-cms/35590/1/8 and of which I have explained you how to find it clearly tells the line numbers where tabs have to be used.
PHPCS is fine now. Thanks @Scrabble96 .
PHPCS is fine now. Thanks @Scrabble96 .
Thank you. But I'm confused as to why cardGrey has passed checks but default has not. The ONLY difference in the code is line 25 - the class 'card-grey' does not appear in default.php
Thank you. But I'm confused as to why cardGrey has passed checks but default has not. The ONLY difference in the code is line 25 - the class 'card-grey' does not appear in default.php
@Scrabble96 The check is not only done for that one file which is in your commit. It is always done for the complete installation. I.e. after you had checked in the corrected default.php, the test has still failed because cardGrey.php still was not correct. After you had checked that in with the next commit, the test succeeded because now all files were correct. If you had checked in both changed files with one single commit, the test would have been immediately ok with that one commit.
I hope this explains it now.
I hope this explains it now.
More or less. Hopefully, if I do this again (if my nerves will stand it) I'll do a better job.
@Scrabble96 The more often you do it, the better it will be
What happens now?
Now volunteers need to test your PR. If it has 2 good tests, it will get status "RTC", which means "ready to commit". Then finally a maintainer will have a look on it and if ok, merge it, and then it will be in the next 4.0 Beta or RC release after that merge.
The 2 tests can't include the author of a PR because it is assumed that a PR's author has already tested his or her code.
Now volunteers need to test your PR. If it has 2 good tests, it will get status "RTC", which means "ready to commit". Then finally a maintainer will have a look on it and if ok, merge it, and then it will be in the next 4.0 Beta or RC release after that merge.
The 2 tests can't include the author of a PR because it is assumed that a PR's author has already tested his or her code.
Thanks again, @richard67
@Scrabble96 Could you update the description of this PR, section "Summary of Changes"? It still mentions the "noTitle.php" chrome, which was meanwhile deleted, and maybe other things have to be adapted to recent changes, too, in that section.
Beside this, I don't fully understand your testing instructions:
Copy the code and create new chromes (e.g. cardGreyCopy) in J4 Cassiopeia template.
Why does it need to create a copy? Isn't it sufficient just to test the existing chromes modified by this PR?
Title |
|
@Scrabble96 Could you update the description of this PR, section "Summary of Changes"? It still mentions the "noTitle.php" chrome, which was meanwhile deleted, and maybe other things have to be adapted to recent changes, too, in that section.
Done
Beside this, I don't fully understand your testing instructions:
Copy the code and create new chromes (e.g. cardGreyCopy) in J4 Cassiopeia template.
Why does it need to create a copy? Isn't it sufficient just to test the existing chromes modified by this PR?
I always use a copy for testing so that I can view that and the original side by side in separate tabs.
@Scrabble96 Now after #30729 has been merged, your PR shows conflicts for file templates/cassiopeia/html/layouts/chromes/default.php
. I think you can resolve them by renaming the file in your branch from default.php
to card.php
and then marking the conflict as recolved in your Git client or IDE. Could you do that? Please let me know if I shall help.
@Scrabble96 Now after #30729 has been merged, your PR shows conflicts for file
templates/cassiopeia/html/layouts/chromes/default.php
. I think you can resolve them by renaming the file in your branch fromdefault.php
tocard.php
and then marking the conflict as recolved in your Git client or IDE. Could you do that? Please let me know if I shall help.
Hm. Did that (renamed file), but something went wrong. Looked at file in my repository (online) but none the wiser except that my code is different - obviously - from the original. Sorry, but I'm away for about an hour now.
Maybe you had forgotten to mark the conflict as resolved? I will check if I can solve it here now after the rename.
Done. After the rename it sill was necessary to resolve the conflict.
I always use a copy for testing so that I can view that and the original side by side in separate tabs.
@Scrabble96 That makes maybe sense when you develop locally before making a PR. But it doesn't make any sense for this PR, because when the PR is applied, the file to be copied is already the changed file. So please correct your testing instructions so that they make sense. Thanks in advance.
So please correct your testing instructions so that they make sense. Thanks in advance.
Is this ok now?
Is this ok now?
Well, step 1 is simply "Apply the patch of this PR", it is on the tester if he or she does this by copying files, like you wrote, or if he or she is using the patchtester for that, or applying a diff with git, there are several ways.
But when applying the patch as first, you can't test the "Actual result BEFORE applying this Pull Request" part anymore.
Many PR's leave it like this or even leave away step 1 because it is more or less clear that a patch has to be applied to see the "Expected result AFTER applying this Pull Request" result. Sometimes they only mention it if it requires additional steps like e.g. running npm to compile some scss.
But if you want to make it perfect, you move step 1 "Apply the patch of this PR" to after the first test, and after that repeat the test. This is how I do it most times. So it would be:
In the new step 1 (old step 2) it could be helpful to specify which chromes to be tested. The "the changed chrome to be tested" is not clear to everybody when just reading the instructions.
@Scrabble96 Meanwhile PR #30734 which removed CarGrey has been merged. I've allowed myself to update your PR by the latest changes in the 4.0-dev branch, including that one, and solved the "conflict" by using the deleted file. Sounds strange, but that's how it works in git.
Could you update your testing instructions so they don't mention CardGrey anymore?
Thanks in advance.
Updated to remove ref to cardGrey
Thanks for updating my branch.
(Can’t seem to quote your post in my reply when using iPad)
I have tested this item
Visible differences on frontend:
iMac, Firefox 81.
I have tested this item
Using chrome on windows I see exactly the same error with < class="card-header "> appearing on all the modules before and after the module being tested.
This is the code I see in inspect
I have not tested this item.
Tested on all 'Module Tags' in the advanced tab. Module #id appears in every tag with and without title. Previous error reported no longer exists.
If Module Style -> none then no #id appears.
Side note: Module Tag also doesn't appear if Module Style -> none option is selected .. is this a desired result?
I have tested this item
Tested on all 'Module Tags' in the advanced tab. Module #id appears in every tag with and without title. Previous error reported no longer exists.
If Module Style -> none then no #id appears.
Side note: Module Tag also doesn't appear if Module Style -> none option is selected .. is this a desired result?
Tested on all 'Module Tags' in the advanced tab. Module #id appears in every tag with and without title. Previous error reported no longer exists.
If Module Style -> none then no #id appears.
Side note: Module Tag also doesn't appear if Module Style -> none option is selected .. is this a desired result?
Tested on all 'Module Tags' in the advanced tab. Module #id appears in every tag with and without title. Previous error reported no longer exists.
If Module Style -> none then no #id appears.
Side note: Module Tag also doesn't appear if Module Style -> none option is selected .. is this a desired result?
It happens because 'none' means that no chrome is used. On my own custom template (i.e. not Cassiopeia) I don't use "style=none"
for any modules for exactly that reason.
I have tested this item
id isn't shown using module style:
@Formatio-hippocampi Thank you for testing. Could you report back what did not work for you so your test was not successful? You should always provide some information when a test failed so we can knwow hat has failed and try to reproduce it. Thanks in advance.
was working on update the comment.
Thanks. I will be patient
If I understand correctly, this PR only deals with the card chrome, not the other possible chromes layouts.
If I understand correctly, this PR only deals with the card chrome, not the other possible chromes layouts.
That's my understanding, too.
@Formatio-hippocampi Could you check and maybe correct your testing result? It seems your test was successful because for "card" it worked, and the other chromes are not touched by this PR. The "card" was before a recently merged other PR the "default" chrome, testing instructions still refer to that.
If I understand correctly, this PR only deals with the card chrome, not the other possible chromes layouts.
The "card" was before a recently merged other PR the "default" chrome, testing instructions still refer to that.
I have just updated the testing instructions changing 'default' to 'card'
If I understand correctly, this PR only deals with the card chrome, not the other possible chromes layouts.
Yes. There is currently only one mod chrome in the Cassiopeia template which is called 'card'.
The PR title and the instructions specify 'Cassiopeia'. This change has nothing to do with the system chromes.
I have added 'Cassiopeia' in three more places in the testing instructions so hopefully this is absolutely clear now.
Perhaps a new PR to apply mod #ids to system chromes would be useful?
@Formatio-hippocampi Could you check the pevious comment and if ok for you change your testing result?
I agree it was not 100% clear from reading the testing instructions, but together with the title it makes sense.
It made sense to test "from Template", "System" and "Cassiopeia" as the mentioned "none" in test instuctions are below "System".
Its frustrating to waste time cause instructions are written for developers.
It made sense to test "from Template", "System" and "Cassiopeia" as the mentioned "none" in test instuctions are below "System".
Its frustrating to waste time cause instructions are written for developers.
Sorry. You are right and 'none' is a system chrome. In the Cassiopeia template index.php it uses "style=card"
and "style=none"
and I didn't see that 'none' is listed under system. Apologies. I am definitely not a developer; I just wanted to add the mod #id to as many modules in Cassiopeia as possible instead of just the title when displayed.
Cassiopea also uses html5 as a possible chrome in its index.php.
<?php if ($this->countModules('banner')) : ?>
<div class="grid-child container-banner">
<jdoc:include type="modules" name="banner" style="html5" />
</div>
Cassiopea also uses html5 as a possible chrome in its index.php.
<?php if ($this->countModules('banner')) : ?> <div class="grid-child container-banner"> <jdoc:include type="modules" name="banner" style="html5" /> </div>
I have now updated the testing instructions to remove reference to 'none'.
To clarify - this PR applies ONLY to the Cassiopeia 'card' chrome
Expected result AFTER applying this Pull Request
module #id always applied to Cassiopeia modules using Cassiopeia 'card' chrome
Title |
|
Hello. This has now had one successful 'human' test. Can two others please test? Many thanks.
Hello. This has now had one successful 'human' test. Can two others please test? Many thanks.
One other is sufficient. It needs 2 tests in total.
I have tested this item
This looks good. The testing notes were really clear.
Before the patch, the HTML of my module looked like...
<div class="sidebar-right card " aria-label="30716 test module">
<div class="card-body">
<div class="mod-custom custom" >
<p>This is a test module for [4.0] Cassiopeia - update card mod chrome #30716</p>
</div>
</div>
</div>
After the patch, the HTML of my module looked like...
<div id="mod-122" class="sidebar-right card " aria-label="30716 test module">
<div class="card-body">
<div class="mod-custom custom" >
<p>This is a test module for [4.0] Cassiopeia - update card mod chrome #30716</p>
</div>
</div>
</div>
It's great to have the ID on every module, really useful for template authoring and content styling.
Thanks @Scrabble96 awesome work :)
Status | Pending | ⇒ | Ready to Commit |
RTC
@Scrabble96 Just as explanation: As the PR has 2 good tests now, it got the status "RTC", which means "ready to commit". This means sooner or later finally a maintainer will have a look on it, and it's ok it will be merged and the come into the next 4.0 Beta release after that merge.
I have tested this item
✅ successfully on 64bde14This looks good. The testing notes were really clear.
Thanks. They took a few revisions and I’ve learnt a lot so it was worth it.
It's great to have the ID on every module, really useful for template authoring and content styling.
Thanks @Scrabble96 awesome work :)
You’re very welcome @AndyGaskell :-))
@Scrabble96 Just as explanation: As the PR has 2 good tests now, it got the status "RTC", which means "ready to commit". This means sooner or later finally a maintainer will have a look on it, and it's ok it will be merged and the come into the next 4.0 Beta release after that merge.
Many thanks, @richard67, for all your help and your patience.
Thanks for your pr but I have some problems with it.
Annotations
$module->module . '-' . $module->id . '-aria-header'
But since 1. can't be resolved I'm not sure if it's a good idea to break the html standard
Thanks for your pr but I have some problems with it.
1. A module maybe display more then once in this case you have duplicated ids
How often do you use the same module on the same page? If the same module is used on different pages, then the duplicate ID doesn't matter. But if a module is used twice on the same page then there could be a duplicate ID but this theory then also applies to an article with a manually added ID and used more than once on the same page.
2. You use the id one time for the aria label and on the other time (when no title exists) as container id, that's confusing
I did originally have the id on the outer container in both instances but this was changed to the current option based on input from others and relates to the use of aria-label and aria-labelledby.
Annotations
1. I think that can't be solved that might be the reason why we don't have a module id (ymmv) 2. Use an explicit id for the aria label something like `$module->module . '-' . $module->id . '-aria-header'`
The aria-label can't - as far as I know - be used for URL anchors, which was one of the main reasons I raised this PR.
But since 1. can't be resolved I'm not sure if it's a good idea to break the html standard
I wonder if there's a way of identifying a second/third/... use of a module on a page and adding an additional flag to the id?
1. A module maybe display more then once in this case you have duplicated ids
@HLeithner Shame on me that I haven't noticed that. Shall I remove RTC?
It was the same way before patch.
It was the same way before patch.
true then it was wrong before too...
How often do you use the same module on the same page? If the same module is used on different pages, then the duplicate ID doesn't matter. But if a module is used twice on the same page then there could be a duplicate ID but this theory then also applies to an article with a manually added ID and used more than once on the same page.
Doesn't matter how often we should not ship known broken code... but I wouldn't stop it if a11y team says we have to
I did originally have the id on the outer container in both instances but this was changed to the current option based on input from others and relates to the use of aria-label and aria-labelledby.
I didn't read all comments, is there a reason why? I don't think we should move the id randomly. But anyway if we keep it for a11y it should only be on the header marked as a11y id
Annotations
1. I think that can't be solved that might be the reason why we don't have a module id (ymmv) 2. Use an explicit id for the aria label something like `$module->module . '-' . $module->id . '-aria-header'`
The aria-label can't - as far as I know - be used for URL anchors, which was one of the main reasons I raised this PR.
? that was not what I mean. just rename the id to mod_xyz-123-aria-header (or label if it fits better) and only add it to the header tag.
But since 1. can't be resolved I'm not sure if it's a good idea to break the html standard
I wonder if there's a way of identifying a second/third/... use of a module on a page and adding an additional flag to the id?
sadly not because a module can (or should be cached) and the id is part of the complete html cache.
Status | Ready to Commit | ⇒ | Pending |
Back to pending
So, we seem to be back to square one, where the current module chrome only adds an id if there is a module title displayed.
(My PR was designed to add the id whether the title was displayed or not.)
But it would seem that even having an id only shown when the title is displayed shouldn't be allowed for the reasons @HLeithner pointed out.
just rename the id to mod_xyz-123-aria-header
will have the same problem with duplicate ids if the module is displayed twice on the page.
just rename the id to mod_xyz-123-aria-header
will have the same problem with duplicate ids if the module is displayed twice on the page.
Yes but at least it's explained why we have a duplicated id... we can add it with javascript (just a joke)
I'm not sure if the "chrome" is cached too maybe someone can look at it, if not then it would be at least possible to use a "random" id per module
I was thinking about this, and there seems to be a couple of options...
Move this from the id property to the class property. This way having multiples of a module would not be a problem, but you'd still be able to style your module.
Merge this as is, and create a new issue to deal with the corner case of multiple instances of the same module existing on a page.
- Move this from the id property to the class property. This way having multiples of a module would not be a problem, but you'd still be able to style your module.
That doesn't work because the aria label needs an id, but for a normal css property a class would be good anyway.
- Merge this as is, and create a new issue to deal with the corner case of multiple instances of the same module existing on a page.
and nobody will every look at it....
@HLeithner fair points there.
In that case, perhaps it is a question of which is worse...
As @HLeithner stated a unique ID is required. Its not an option
Which function is responsible for injecting modules Output into the page?
Hooking into that point to create a internal list of already used module-id's on that page,
when a module-id already exist, adding a "-xx" number to that to-beadded module-id?
@brianteeman Aye, that was understood.
@jiweigert Yea, that sounds promising.
I think there is value in spinning off the whole modules having duplicate IDs is used displayed twice on the page, issue.
This PR only effects modules with titles, so best to discus under that premise.
Current code
Issues: Modules not having IDs on their div, only on titles, if displayed more than once on the page, titles having the same ID.
This looks like...
<div class="sidebar-right card " aria-labelledby="mod-122">
<h3 class="card-header " id="mod-122">30716 test module</h3>
<div class="card-body">
<div class="mod-custom custom" >
<p>This is a test module for [4.0] Cassiopeia - update card mod chrome #30716</p>
</div>
</div>
</div>
With this PR
Issues: Modules with titles, if displayed more than once on the page, having the same ID.
This looks like...
<div id="mod-122" class="sidebar-right card" aria-labelledby="mod-122-title">
<h3 class="card-header" id="mod-122-title">30716 test module</h3>
<div class="card-body">
<div class="mod-custom custom" >
<p>This is a test module for [4.0] Cassiopeia - update card mod chrome #30716</p>
</div>
</div>
</div>
...though I did add the title suffix in a couple of places, compared to the PR's code.
Just wondering: How can you have the same module instance twice on the page? Afaik you can't. You can have multiple instances of the same module, even with same parameters. But they are still own instances with own IDs.
Just wondering: How can you have the same module instance twice on the page? Afaik you can't. You can have multiple instances of the same module, even with same parameters. But they are still own instances with own IDs.
just load the same module position multiple times on the page, for example a menu once used for mobile and one for desktop. (only an example)
Ah true, didn't think that anyone would have a duplicate module position. Makes sense now.
... if not then it would be at least possible to use a "random" id per module
A random unpredictable ID defeats the purpose of an id really in terms of targeting it with anything front-end, having a predictable ID seems important. So can't the position and the module ID be used together as the ID? or what am I missing? Certainly you can't have the same module in the same position multiple times.
A random unpredictable ID defeats the purpose of an id really in terms of targeting it with anything front-end, having a predictable ID seems important. So can't the position and the module ID be used together as the ID? or what am I missing? Certainly you can't have the same module in the same position multiple times.
@Llewellynvdm That seems an excellent idea to me and a very simple tweak to the code.
Certainly you can't have the same module in the same position multiple times.
@Llewellynvdm That's not true. Theoretically one can show the same menu module for the same menu multiple times on the same position. That could make sense e.g. if the menu has several levels of submenus and in the first menu you show e.g. the top level, the second one the next deeper level and so on. I have no idea if this is practically relevant, but it is possible. Sorry, forget it, my mistake. You said to use the module ID, not the menu ID, and so they are different modules and so have different IDs. Silly me.
Shall I change and submit the new code? If so, perhaps the #id should be on the module container, regardless of whether the title is displayed or not. In which case, how should I best handle the ‘aria-label’ and ‘aria-labelledby’ aspects? Do we need both?
A random unpredictable ID defeats the purpose of an id really in terms of targeting it with anything front-end, having a predictable ID seems important. So can't the position and the module ID be used together as the ID? or what am I missing? Certainly you can't have the same module in the same position multiple times.
I see no way how to do this. ymmv
If you have the same template position multiple times, or manually load a module multiple times, then you get duplicate IDs.
So you just can't set the ID in the chrome. You need to use class.
If you have the same template position multiple times.
Why would you do that?
You need to use class.
How? By using a pseudo 'before' class?
I have modified the chrome (locally, only) to include the module position in the id and it works. But this is based on the assumption that module positions are only used once. I'll post it here if anyone is interested.
[I do, however, get this message on Firefox: "Layout was forced before the page was fully loaded. If stylesheets are not yet loaded this may cause a flash of unstyled content." but not on Chrome or Edge. ] Edit: I refreshed the .htaccess file and this seems to have cleared the problem.
See comment from Harald why a position may be present more than once.
So using the ID attribute is not an option. Even when coupled with the position you still get duplicates.
You can only use the class attribute. There you can insert the module ID as a class which can then be used for CSS styling.
Funny, I got a link to this document today by a customer: https://www.w3.org/TR/WCAG20-TECHS/H93.html IDs have to be unique.
There you can insert the module ID as a class which can then be used for CSS styling.
But you can't use it for URL anchors which was one of the two reasons I wanted to add an ID to all modules.
Technically it should be possible to inject the file and line position of the into the module attributes and generate a unique id per module and position. That would invalidate our module caching if it's loaded twice on the same page. That can be solved in 2 ways.
The relevant starting point for a PR is in libraries/src/Document/HtmlDocument.php:849
Probably the simplest approach is to just create your own chrome for your own installation where you need the anchor. There you can make sure that module with that chrome is only showing once per page.
For a module chrome shipped with the CMS we just can't be sure that the ID will be unique.
By the way, a PR which tried to achieve a similar thing with a different approach was #12473
Probably the simplest approach is to just create your own chrome for your own installation where you need the anchor. There you can make sure that module with that chrome is only showing once per page.
For those who would like it, I am happy to share my chrome code which shows both the module position and the module id, but, as @Bakual says, obviously taking into account that any id should be shown only once per page:
<?php
/**
* @package Joomla.Site
* @subpackage Templates.cassiopeia
*
* @copyright Copyright (C) 2005 - 2020 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE.txt
*/
defined('_JEXEC') or die;
use Joomla\Utilities\ArrayHelper;
$module = $displayData['module'];
$params = $displayData['params'];
$attribs = $displayData['attribs'];
if ($module->content === null || $module->content === '')
{
return;
}
$moduleTag = $params->get('module_tag', 'div');
$modId = $module->id;
$modPos = $module->position;
$moduleAttribs = [];
$moduleAttribs['id'] = $modPos . '-' . $modId;
$moduleAttribs['class'] = $module->position . ' card ' . htmlspecialchars($params->get('moduleclass_sfx'), ENT_QUOTES, 'UTF-8');
$headerTag = htmlspecialchars($params->get('header_tag', 'h4'), ENT_QUOTES, 'UTF-8');
$headerClass = 'card-header ' . htmlspecialchars($params->get('header_class', ''), ENT_QUOTES, 'UTF-8');
$headerAttribs = [];
$headerAttribs['class'] = $headerClass;
if ($module->showtitle) :
else:
$moduleAttribs['aria-label'] = $module->title;
endif;
$header = '<' . $headerTag . ' ' . ArrayHelper::toString($headerAttribs) . '>' . $module->title . '</' . $headerTag . '>';
?>
<<?php echo $moduleTag; ?> <?php echo ArrayHelper::toString($moduleAttribs); ?>>
<?php if ($module->showtitle && $headerClass !== 'card-title') : ?>
<?php echo $header; ?>
<?php endif; ?>
<div class="card-body">
<?php echo $module->content; ?>
</div>
</<?php echo $moduleTag; ?>>
The html looks like this with title:
And like this without a title:
For a module chrome shipped with the CMS we just can't be sure that the ID will be unique.
As pointed out earlier, the current 'card' chrome includes a module ID, but only if the title is shown.
By the way, a PR which tried to achieve a similar thing with a different approach was #12473
I hadn't spotted this as it isn't J4.
EDIT by hleithner: updated the code highlighting
Tested successfully on localhost
@HLeithner @Bakual @infograf768 What shall we do with this one? It has 2 good tests, but these seems to be some unresolved discussion? Shall I set discussion status in the issue tracker?
I have tested this item
#30716 (comment) Option 1 is the way to go.
@richard67 you got the answer a week ago
We don't need trolls here
true
I have tested this item
✅ successfully on 64bde14
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30716.
@gostn this PR was effectively put on hold due to the use of IDs in the chrome which would create duplicate IDs if the same module was used twice on the page, which is not allowed in good website page setup. There was a suggestion to add the 'discussion' tag to this PR, but that hasn't happened. I don't have the authority to do that. My original reason for opening this PR was two-fold: that the ID could be used for in-page anchors and that the ID would be shown whether the title was shown or not. Due to the problem with duplicate IDs this PR doesn't seem viable as it stands.
Frustratingly, before HTML5 the name attribute could be used in for anchors in links but it has now been deprecated in favour of the use of IDs, which brings us back to square one. I wonder when they (whoever 'they' are) deprecated its use whether they considered the possibility of duplicate IDs on a page?
@Scrabble96 thanks for your friendly information.
@Scrabble96 This PR of your seems to have a merge conflict and cannot be merged into Joomla.
Can you rewrite your PR?
I have a suggestion.
joomla-cms/templates/cassiopeia/html/layouts/chromes/card.php
Lines 24 to 25 in 3b82bf1
To make sure a module always has an id
, rewrite the two lines above into the three lines below:
$moduleAttribs = [];
$moduleAttribs['id'] = 'mod-' . $module->id;
$moduleAttribs['class'] = $module->position . ' card ' . htmlspecialchars($params->get('moduleclass_sfx'), ENT_QUOTES, 'UTF-8');
Two prevent duplicate ID's we have to adjust a second part of this file too:
joomla-cms/templates/cassiopeia/html/layouts/chromes/card.php
Lines 36 to 45 in 3b82bf1
To prevent duplicate ID's we have to adjust the name of the id for the header into:
// Only add aria if the moduleTag is not a div
if ($moduleTag !== 'div')
{
if ($module->showtitle) :
$moduleAttribs['aria-labelledby'] = 'mod-title-' . $module->id;
$headerAttribs['id'] = 'mod-title-' . $module->id;
else:
$moduleAttribs['aria-label'] = $module->title;
endif;
}
With these two changes based on the current implementation of template/cassiopeia/html/layouts/chromes/card.php the PR is good to go I think.
What do you think?
As already mentioned if the module is cached (and if I'm not wrong) the id is still duplicated.
@Scrabble96 This PR of your seems to have a merge conflict and cannot be merged into Joomla.
Can you rewrite your PR?
@hans2103 I'll have a look in the next few days but, as @HLeithner says, there is a problem with cached modules so I don't think this is going to work.
I'm going to close this PR due to the cache conflict.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-04-26 11:21:58 |
Closed_By | ⇒ | Scrabble96 | |
Labels |
Added:
?
|
@Scrabble96 You should change your editor's settings so it shows spaces and tabs, so you can see what kind of white space is used.