? ? Success

User tests: Successful: Unsuccessful:

avatar Scrabble96
Scrabble96
21 Sep 2020

Pull Request for Issue #30678 (second attempt) .

Summary of Changes

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.

Testing Instructions

  1. Create a new module if none already exists and in the Advanced tab select Cassiopeia's 'card' chrome from the list in 'Module Style' if not already applied.
  2. On the front end, use the browser's inspector to inspect this module with title shown and then again where title is not shown
    Result: See section "Actual result BEFORE applying this Pull Request" below.
  3. Apply the patch of this PR
  4. Repeat steps 1. and 2.
    See section "Actual result AFTER applying this Pull Request" below.

Actual result BEFORE applying this Pull Request

module #id only appears if title is shown

Expected result AFTER applying this Pull Request

  1. module #id always applied to Cassiopeia modules using Cassiopeia 'card' chrome
  2. id can now be used for anchors, etc if 1. above applies.

No visible difference on front end for website visitors

Documentation Changes Required

Note to highlight change

avatar Scrabble96 Scrabble96 - open - 21 Sep 2020
avatar Scrabble96 Scrabble96 - change - 21 Sep 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 21 Sep 2020
Category Front End Templates (site)
avatar richard67
richard67 - comment - 21 Sep 2020

@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.

avatar Scrabble96 Scrabble96 - change - 21 Sep 2020
Labels Added: ?
avatar Bakual
Bakual - comment - 21 Sep 2020

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.

avatar Scrabble96
Scrabble96 - comment - 21 Sep 2020

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.

avatar Bakual
Bakual - comment - 21 Sep 2020

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.

avatar Scrabble96
Scrabble96 - comment - 21 Sep 2020

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'.

avatar Bakual
Bakual - comment - 21 Sep 2020

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.

avatar Scrabble96
Scrabble96 - comment - 22 Sep 2020

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.

avatar Scrabble96
Scrabble96 - comment - 22 Sep 2020

Ok. What's wrong with my code this time?
Edit: I've lost the aria-label and aria-labelledby
I'll put them back.

avatar richard67
richard67 - comment - 22 Sep 2020

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.

avatar Scrabble96
Scrabble96 - comment - 22 Sep 2020

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.

avatar richard67
richard67 - comment - 22 Sep 2020

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.

avatar richard67
richard67 - comment - 22 Sep 2020

PHPCS is fine now. Thanks @Scrabble96 .

avatar Scrabble96
Scrabble96 - comment - 22 Sep 2020

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

avatar richard67
richard67 - comment - 22 Sep 2020

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.

avatar Scrabble96
Scrabble96 - comment - 22 Sep 2020

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.

avatar richard67
richard67 - comment - 22 Sep 2020

@Scrabble96 The more often you do it, the better it will be ?

avatar Scrabble96
Scrabble96 - comment - 22 Sep 2020

What happens now?

avatar richard67
richard67 - comment - 22 Sep 2020

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.

avatar Scrabble96
Scrabble96 - comment - 22 Sep 2020

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

avatar richard67
richard67 - comment - 22 Sep 2020

@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?

avatar Scrabble96 Scrabble96 - change - 23 Sep 2020
Title
[4.0] Cassiopeia - update mod chromes and add new chrome
[4.0] Cassiopeia - update mod chromes
avatar Scrabble96 Scrabble96 - edited - 23 Sep 2020
avatar Scrabble96 Scrabble96 - change - 23 Sep 2020
The description was changed
avatar Scrabble96 Scrabble96 - edited - 23 Sep 2020
avatar Scrabble96
Scrabble96 - comment - 23 Sep 2020

@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.

avatar richard67
richard67 - comment - 23 Sep 2020

@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.

avatar Scrabble96
Scrabble96 - comment - 23 Sep 2020

@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.

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.

avatar richard67
richard67 - comment - 23 Sep 2020

Maybe you had forgotten to mark the conflict as resolved? I will check if I can solve it here now after the rename.

avatar richard67
richard67 - comment - 23 Sep 2020

Done. After the rename it sill was necessary to resolve the conflict.

avatar richard67
richard67 - comment - 23 Sep 2020

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.

avatar Scrabble96 Scrabble96 - change - 23 Sep 2020
The description was changed
avatar Scrabble96 Scrabble96 - edited - 23 Sep 2020
avatar Scrabble96
Scrabble96 - comment - 23 Sep 2020

So please correct your testing instructions so that they make sense. Thanks in advance.

Is this ok now?

avatar richard67
richard67 - comment - 23 Sep 2020

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:

  1. Create a new module if none already exists and in the Advanced tab select the changed chrome to be tested from the list in 'Module Style' if not already applied.
  2. On the front end, use the browser's inspector to inspect this module with title shown and then again where title is not shown.
    Result: See section "Actual result BEFORE applying this Pull Request" below.
  3. Apply the patch of this PR.
  4. Repeat steps 1 and 2.
    Result: See section "Expected result AFTER applying this Pull Request" below.

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.

avatar Scrabble96 Scrabble96 - change - 23 Sep 2020
The description was changed
avatar Scrabble96 Scrabble96 - edited - 23 Sep 2020
avatar Scrabble96 Scrabble96 - change - 23 Sep 2020
The description was changed
avatar Scrabble96 Scrabble96 - edited - 23 Sep 2020
avatar richard67
richard67 - comment - 24 Sep 2020

@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.

avatar Scrabble96 Scrabble96 - change - 24 Sep 2020
The description was changed
avatar Scrabble96 Scrabble96 - edited - 24 Sep 2020
avatar Scrabble96
Scrabble96 - comment - 24 Sep 2020

Updated to remove ref to cardGrey
Thanks for updating my branch.
(Can’t seem to quote your post in my reply when using iPad)

avatar Formatio-hippocampi Formatio-hippocampi - test_item - 25 Sep 2020 - Tested unsuccessfully
avatar ghost
ghost - comment - 25 Sep 2020

I have tested this item ? unsuccessfully on c614e23

Visible differences on frontend:
Screenshot_2020-09-25 Home


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30716.
avatar Scrabble96
Scrabble96 - comment - 25 Sep 2020

I have tested this item ? unsuccessfully on c614e23

Which device and browser are you using?

avatar ghost
ghost - comment - 25 Sep 2020

iMac, Firefox 81.

avatar 1252spike 1252spike - test_item - 25 Sep 2020 - Tested unsuccessfully
avatar 1252spike
1252spike - comment - 25 Sep 2020

I have tested this item ? unsuccessfully on c614e23

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 comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30716.
avatar 1252spike
1252spike - comment - 25 Sep 2020

screen shot 2020-09-25 at 13 08 24

This is the code I see in inspect


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

avatar 1252spike 1252spike - test_item - 25 Sep 2020 - Not tested
avatar 1252spike
1252spike - comment - 25 Sep 2020

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?


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

avatar 1252spike 1252spike - test_item - 25 Sep 2020 - Tested successfully
avatar 1252spike
1252spike - comment - 25 Sep 2020

I have tested this item successfully on 2bb9e0c

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?


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

avatar 1252spike
1252spike - comment - 25 Sep 2020

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?


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

avatar Scrabble96
Scrabble96 - comment - 25 Sep 2020

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.

avatar Formatio-hippocampi Formatio-hippocampi - test_item - 26 Sep 2020 - Tested unsuccessfully
avatar ghost
ghost - comment - 26 Sep 2020

I have tested this item ? unsuccessfully on 6bdab92

id isn't shown using module style:

html5

html5

outline

outline

table

table


id is shown using module style "card", "inherited"

card-inherited


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30716.
avatar richard67
richard67 - comment - 26 Sep 2020

@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.

avatar ghost
ghost - comment - 26 Sep 2020

was working on update the comment.

avatar richard67
richard67 - comment - 26 Sep 2020

Thanks. I will be patient ?

avatar infograf768
infograf768 - comment - 26 Sep 2020

If I understand correctly, this PR only deals with the card chrome, not the other possible chromes layouts.

avatar richard67
richard67 - comment - 26 Sep 2020

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.

avatar Scrabble96 Scrabble96 - change - 26 Sep 2020
The description was changed
avatar Scrabble96 Scrabble96 - edited - 26 Sep 2020
avatar Scrabble96
Scrabble96 - comment - 26 Sep 2020

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'

avatar Scrabble96
Scrabble96 - comment - 26 Sep 2020

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'.

avatar ghost
ghost - comment - 26 Sep 2020

so:
94336411-6c6d8f80-ffe3-11ea-9465-580230d85a47
didn't mean
Screen Shot 2020-09-26 at 10 50 45
only styles below "cassiopeia"?

avatar Scrabble96
Scrabble96 - comment - 26 Sep 2020

so:
94336411-6c6d8f80-ffe3-11ea-9465-580230d85a47
didn't mean
Screen Shot 2020-09-26 at 10 50 45
only styles below "cassiopeia"?

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?

avatar Scrabble96 Scrabble96 - change - 26 Sep 2020
The description was changed
avatar Scrabble96 Scrabble96 - edited - 26 Sep 2020
avatar richard67
richard67 - comment - 26 Sep 2020

@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.

avatar ghost
ghost - comment - 26 Sep 2020

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.

avatar Scrabble96
Scrabble96 - comment - 26 Sep 2020

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.

avatar infograf768
infograf768 - comment - 26 Sep 2020

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>
avatar Scrabble96 Scrabble96 - change - 26 Sep 2020
The description was changed
avatar Scrabble96 Scrabble96 - edited - 26 Sep 2020
avatar Scrabble96
Scrabble96 - comment - 26 Sep 2020

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
avatar Scrabble96 Scrabble96 - change - 26 Sep 2020
Title
[4.0] Cassiopeia - update mod chromes
[4.0] Cassiopeia - update card mod chrome
avatar Scrabble96 Scrabble96 - edited - 26 Sep 2020
avatar gostn gostn - test_item - 29 Sep 2020 - Tested successfully
avatar Scrabble96
Scrabble96 - comment - 4 Oct 2020

Hello. This has now had one successful 'human' test. Can two others please test? Many thanks.

avatar richard67
richard67 - comment - 4 Oct 2020

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.

avatar AndyGaskell AndyGaskell - test_item - 5 Oct 2020 - Not tested
avatar AndyGaskell AndyGaskell - test_item - 5 Oct 2020 - Tested successfully
avatar AndyGaskell
AndyGaskell - comment - 5 Oct 2020

I have tested this item successfully on 64bde14

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 :)


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

avatar richard67 richard67 - change - 5 Oct 2020
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 5 Oct 2020

RTC


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

avatar richard67
richard67 - comment - 5 Oct 2020

@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.

avatar Scrabble96
Scrabble96 - comment - 6 Oct 2020

I have tested this item successfully on 64bde14

This 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 :-))

avatar Scrabble96
Scrabble96 - comment - 6 Oct 2020

@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.

avatar HLeithner
HLeithner - comment - 6 Oct 2020

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
  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

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'

But since 1. can't be resolved I'm not sure if it's a good idea to break the html standard

avatar Scrabble96
Scrabble96 - comment - 6 Oct 2020

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?

avatar richard67
richard67 - comment - 6 Oct 2020
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?

avatar SharkyKZ
SharkyKZ - comment - 6 Oct 2020

It was the same way before patch.

avatar HLeithner
HLeithner - comment - 6 Oct 2020

It was the same way before patch.

true then it was wrong before too...

avatar HLeithner
HLeithner - comment - 6 Oct 2020

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.

avatar infograf768 infograf768 - change - 6 Oct 2020
Status Ready to Commit Pending
avatar infograf768
infograf768 - comment - 6 Oct 2020

Back to pending


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

avatar Scrabble96
Scrabble96 - comment - 6 Oct 2020

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.

avatar HLeithner
HLeithner - comment - 6 Oct 2020

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

avatar AndyGaskell
AndyGaskell - comment - 6 Oct 2020

I was thinking about this, and there seems to be a couple of options...

  1. 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.

  2. 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.

avatar HLeithner
HLeithner - comment - 6 Oct 2020
  1. 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.

  1. 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....

avatar AndyGaskell
AndyGaskell - comment - 6 Oct 2020

@HLeithner fair points there.

In that case, perhaps it is a question of which is worse...

  1. Modules not having IDs on their div, and modules with titles, if displayed more than once on the page, having the same ID.
  2. Modules with titles, if displayed more than once on the page, having the same ID.
avatar brianteeman
brianteeman - comment - 6 Oct 2020

As @HLeithner stated a unique ID is required. Its not an option

avatar jiweigert
jiweigert - comment - 6 Oct 2020

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?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30716.
avatar AndyGaskell
AndyGaskell - comment - 6 Oct 2020

@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.

avatar Bakual
Bakual - comment - 7 Oct 2020

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.

avatar HLeithner
HLeithner - comment - 7 Oct 2020

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)

avatar Bakual
Bakual - comment - 8 Oct 2020

Ah true, didn't think that anyone would have a duplicate module position. Makes sense now.

avatar Llewellynvdm
Llewellynvdm - comment - 9 Oct 2020

@HLeithner

... 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.

avatar Scrabble96
Scrabble96 - comment - 9 Oct 2020

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.

avatar richard67
richard67 - comment - 9 Oct 2020

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.

avatar Scrabble96
Scrabble96 - comment - 9 Oct 2020

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?

avatar HLeithner
HLeithner - comment - 9 Oct 2020

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

avatar Bakual
Bakual - comment - 9 Oct 2020

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.

avatar Scrabble96
Scrabble96 - comment - 9 Oct 2020

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?

avatar Scrabble96
Scrabble96 - comment - 9 Oct 2020

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.

avatar Bakual
Bakual - comment - 9 Oct 2020

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.

avatar HLeithner
HLeithner - comment - 9 Oct 2020

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.

avatar Scrabble96
Scrabble96 - comment - 9 Oct 2020

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.

avatar HLeithner
HLeithner - comment - 9 Oct 2020

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.

  1. make a placeholder in the chrome/module layout and do a str_replace on render (we do this on full page cache for example)
  2. ignore it and accept that the same module maybe rendered twice (I think that's a less complicated variant and shouldn't hurt to much)

The relevant starting point for a PR is in libraries/src/Document/HtmlDocument.php:849

avatar Bakual
Bakual - comment - 11 Oct 2020

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

avatar Scrabble96
Scrabble96 - comment - 12 Oct 2020

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:

pr-30716-with-id-and-module-position-with-title

And like this without a title:

pr-30716-with-id-and-module-position-no-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

avatar ChrisHoefliger
ChrisHoefliger - comment - 17 Oct 2020

Tested successfully on localhost

avatar richard67
richard67 - comment - 8 Nov 2020

@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?

avatar gostn gostn - test_item - 10 Nov 2020 - Not tested
avatar gostn gostn - test_item - 24 Nov 2020 - Tested successfully
avatar gostn
gostn - comment - 24 Nov 2020

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.

avatar HLeithner
HLeithner - comment - 24 Nov 2020

#30716 (comment) Option 1 is the way to go.

avatar gostn
gostn - comment - 1 Dec 2020

@richard67 you got the answer a week ago

avatar richard67
richard67 - comment - 1 Dec 2020

@gostn We don't need trolls here.

avatar gostn
gostn - comment - 1 Dec 2020

We don't need trolls here

true

avatar Scrabble96
Scrabble96 - comment - 1 Dec 2020

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?

avatar gostn
gostn - comment - 1 Dec 2020

@Scrabble96 thanks for your friendly information.

avatar hans2103
hans2103 - comment - 16 Mar 2021

@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.

$moduleAttribs = [];
$moduleAttribs['class'] = $module->position . ' card ' . htmlspecialchars($params->get('moduleclass_sfx'), ENT_QUOTES, 'UTF-8');

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:

// Only add aria if the moduleTag is not a div
if ($moduleTag !== 'div')
{
if ($module->showtitle) :
$moduleAttribs['aria-labelledby'] = 'mod-' . $module->id;
$headerAttribs['id'] = 'mod-' . $module->id;
else:
$moduleAttribs['aria-label'] = $module->title;
endif;
}

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?

avatar HLeithner
HLeithner - comment - 16 Mar 2021

As already mentioned if the module is cached (and if I'm not wrong) the id is still duplicated.

avatar Scrabble96
Scrabble96 - comment - 18 Mar 2021

@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.

avatar Scrabble96
Scrabble96 - comment - 26 Apr 2021

I'm going to close this PR due to the cache conflict.

avatar Scrabble96 Scrabble96 - change - 26 Apr 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-04-26 11:21:58
Closed_By Scrabble96
Labels Added: ?
avatar Scrabble96 Scrabble96 - close - 26 Apr 2021

Add a Comment

Login with GitHub to post a comment