User tests: Successful: Unsuccessful:
Pull Request for Issue #33528
This also fixes an issue where Passwords fields without rules="true"
had to include a misleading attribute: hiddenDescription="true"
so that they could prevent the description being shown twice
Edit: Removed the dependency of rules on description as discussed in the conversation below. I've modified some of the testing conditions accordingly
Now, the description can be independent of the rules text and both can be used together with different text in a single field
Admin Panel -> System -> Global Config -> Server-> Check the Password Field under Database Fieldset
Field 1 contains: | Field 2 contains: |
---|---|
|
|
<field
name="password1"
type="password"
label="COM_CONFIG_FIELD_DATABASE_PASSWORD_LABEL"
description="COM_CONFIG_FIELD_DATABASE_PASSWORD_DESC"
filter="raw"
autocomplete="off"
size="30"
lock="true"
/>
<field
name="password"
type="password"
label="JGLOBAL_PASSWORD"
rules="true"
description="COM_CONFIG_FIELD_DATABASE_PASSWORD_DESC"
autocomplete="new-password"
class="validate-password-strength"
filter="raw"
validate="password"
strengthmeter="true"
force="on"
size="30"
/>
You can also test out different combinations of the following attributes:
None
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_config Layout |
Labels |
Added:
?
|
Category | Administration com_config Layout | ⇒ | Administration com_config Layout Front End Plugins |
The PR might do what's desired as a visual result, but I'm not sure if it's good design to make the handling of the description dependent on the presence or absence of the rules. Isn't there another way, cleaner way to achieve the same visual result?
It's just a question. I am not a password fields expert or so, so I might be wrong with my concerns.
@richard67 I hope I understood your point correctly, this PR doesn't change how description is normally shown as that is done via renderField.php which is a totally separate file:
What this achieves is that it fixes a bug where the description was displayed on the top of the field a second time because of the else block that came in this piece of code:
<?php if (!empty($description)) : ?>
<div id="<?php echo $name . '-desc'; ?>" class="small text-muted">
<?php if ($rules) : ?>
<?php echo Text::sprintf($description, implode(', ', $requirements)); ?>
<?php else : ?>
<?php echo Text::_($description); ?>
<?php endif; ?>
</div>
<?php endif; ?>
This code is taken from password.php and it is present right above the markup for rendering the input tag.
Before the PR, a second description was rendered if the condition $rules
was "false" and $description
was not empty resulting in something like: (refer to the first field), All the existing password fields used a hack where they added the attribute: hiddenDescription="true"
to hide the bottom description and find the easy way.
@YatharthVyas No need to explain the issue again. I did fully understand it. I just was asking if there is another way to fix it.
Let's not make rules attribute be dependent on description attribute.
Do this instead and remove the check for $description
.
<?php echo Text::sprintf('JFIELD_PASSWORD_RULES_MINIMUM_REQUIREMENTS', implode(', ', $requirements)); ?>
@richard67 Oh, I apologize. In the time I spent on this, I found that the only possible solution to fix this was by modifying the code that was involved in rendering the rules and omitting the seemingly unnecessary part from there. In terms of a better way to fix this, I am unable to come up with anything. I'll still try to brainstorm a little more around this but I doubt I'll be having any better luck
@YatharthVyas No need to apologize. If there is no better way then it's ok. But maybe @Quy 's suggestion above could make us find a way?
Thanks, @Quy and @richard67
Got to learn something new working on this PR!
I was hesitant about mapping to this language string of 'Minimum Requirements:' because I was concerned about Rules involving Maximum Character limit or something similar but turns out that the requirements array that holds all the rules is only based on:
So, 'Minimum' stays common among all
Perfect! Please remove where applicable.
description="JFIELD_PASSWORD_RULES_MINIMUM_REQUIREMENTS"
hiddenDescription="true"
There is an issue to address when rules/description are specified where id
is duplicate.
<div id="jform[password]-desc" class="small text-muted">
<strong>Minimum Requirements</strong> — Characters: 12 </div>
<div id="jform[password]-desc">
<small class="form-text">
<strong>Minimum Requirements</strong> — %s </small>
</div>
Category | Administration com_config Layout Front End Plugins | ⇒ | Administration com_config com_users Front End Layout Plugins |
Almost there. If there is a rules attribute but no description, then aria-describedby
should be the id to rules. But I don't how to handle it when both rules/description are specified.
If both rules and description are valid, I could try writing both of them and separate them by a space. As an extra measure, I'll also ensure that a punctuation mark by adding one after %s in the language constant used for rules
That is cool! It may not be necessary to add the period for rules so you can decide which is best.
Nice team work guys just by the way. That's why I love Joomla, because it's community.
@YatharthVyas How does the generated HTML source look like?
I ask because the $ariaDescribedBy .= !empty($description) ? $name . '-desc' : '';
concatenates without a space in-between, so I would expect it to result in jform[password]-rulesjform[password]-descr
. Is that the case, and the clever browser fixed that? If so, code should be $ariaDescribedBy .= !empty($description) ? ' ' . $name . '-desc' : '';
Update: My suggested change would cause it end in a space if both $rules and $description are empty, so that would not be the perfect fix. It should be refined further.
But maybe I'm wrong and am just missing something, that always can be the case.
@richard67
There is a space after -rules
in Line 88 of password.php (files changed) $ariaDescribedBy = $rules ? $name . '-rules ' : '';
Here's the markup generated in case both are present:
<input
type="password"
name="jform[password]"
id="jform_password"
value="" autocomplete="new-password"
class="form-control js-password-strength validate-password-strength"
aria-describedby="jform[password]-rules jform[password]-desc"
size="30"
maxlength="99"
data-min-length="12"
>
However, I'll add a trim() function to ensure that the extra trailing space isn't there incase of a tag with only rules without description
@richard67
There is a space after-rules
in Line 68 of password.php (files changed)$ariaDescribedBy = $rules ? $name . '-rules ' : '';
Seems I need glasses ;-) Haven't seen that space.
I have tested this item
Awesome work! Thank you!!!
Maybe it needs to adjust the testing instructions to the changes for other tester(s).
Testing Instructions have been updated.
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-05-06 21:34:32 |
Closed_By | ⇒ | chmst | |
Labels |
Added:
?
|
Thank you all, great team work
My latest commit also fixes one more and the only other occurrence of this issue:
Admin Panel -> Plugins -> Authentication - LDAP -> Check the password field
Before:
After: