? ? Pending

User tests: Successful: Unsuccessful:

avatar YatharthVyas
YatharthVyas
4 May 2021

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

Summary of Changes

  • Password fields use the rules attribute to check for the rules specified in the Description (These include max length, allowed chars, etc)
  • If the rules attribute is set to true, then the description and rules are displayed above the password field like:

image

  • Along with this, the description is displayed again below the field because of the echo in renderfield.php layout which is common to all the types of fields
    <?php echo $description; ?>
  • So unless the field has an attribute of hiddenDescription="true", the output turns out to be something like:

image

  • The bug occurs when you add a description without a rule. In this case, the description is repeated twice unless you add the hiddenDescription="true" attribute. But the catch here is that when you apply hiddenDescription, then the description that is present below the field is hidden while the description above the password field stays visible (resulting in issue #33528)
  • Until now, fields with description and without rules had to use hiddenDescription="true" as a make-do way to show description only once. After this PR is merged, developers will no longer have to worry about why their description is being shown twice and they won't have to add a misleading hiddenDescription attribute to tix this.

Testing Instructions

Test for the fixed Issue:

Admin Panel -> System -> Global Config -> Server-> Check the Password Field under Database Fieldset

Test for checking this works globally

  1. Make a new form / Edit an Existing form
  2. Add two fields of type password such that:
    Field 1 contains: Field 2 contains:
    • ✔️ description
    • no rules attribute
    • no hidden description attribute
    • ✔️ description
    • ✔️ rules="true"
    • no hidden description attribute

For Example:
<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"
/>
Before After
image image
Field 1 shows duplicate descriptions and Field 2 shows description instead of rules because it uses the description text to display rules and the language constants for it is missing a %s Field 1 is fixed and Field 2 now shows unique text for rules message and description message

You can also test out different combinations of the following attributes:

  • description (valid or empty)
  • rules (true or false)
  • hiddenDescription (true or false)

Actual result BEFORE applying this Pull Request (For #33528)

image

Expected result AFTER applying this Pull Request (For #33528)

image

Documentation Changes Required

None

avatar YatharthVyas YatharthVyas - open - 4 May 2021
avatar YatharthVyas YatharthVyas - change - 4 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 May 2021
Category Administration com_config Layout
avatar YatharthVyas YatharthVyas - change - 4 May 2021
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 4 May 2021
Category Administration com_config Layout Administration com_config Layout Front End Plugins
avatar YatharthVyas
YatharthVyas - comment - 4 May 2021

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:

image

After:

image

avatar richard67
richard67 - comment - 4 May 2021

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.

avatar YatharthVyas
YatharthVyas - comment - 4 May 2021

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

<?php echo $description; ?>

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.

image

avatar richard67
richard67 - comment - 4 May 2021

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

avatar Quy
Quy - comment - 4 May 2021

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)); ?>

avatar YatharthVyas
YatharthVyas - comment - 4 May 2021

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

avatar richard67
richard67 - comment - 4 May 2021

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

avatar YatharthVyas
YatharthVyas - comment - 4 May 2021

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:

  • Min Length
  • Min Symbols
  • Min Integers
  • Min Lowercase
  • Min Uppercase

So, 'Minimum' stays common among all

avatar Quy
Quy - comment - 4 May 2021

Perfect! Please remove where applicable.

description="JFIELD_PASSWORD_RULES_MINIMUM_REQUIREMENTS"
hiddenDescription="true"
avatar Quy
Quy - comment - 4 May 2021

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>
avatar joomla-cms-bot joomla-cms-bot - change - 4 May 2021
Category Administration com_config Layout Front End Plugins Administration com_config com_users Front End Layout Plugins
avatar YatharthVyas YatharthVyas - change - 4 May 2021
The description was changed
avatar YatharthVyas YatharthVyas - edited - 4 May 2021
avatar Quy
Quy - comment - 4 May 2021

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.

avatar YatharthVyas
YatharthVyas - comment - 4 May 2021

https://stackoverflow.com/questions/53398842/can-i-reference-multiple-elements-using-aria-describedby#:~:text=Yes.,into%20a%20single%20description%20string.

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

avatar Quy
Quy - comment - 4 May 2021

That is cool! It may not be necessary to add the period for rules so you can decide which is best.

avatar richard67
richard67 - comment - 4 May 2021

Nice team work guys just by the way. That's why I love Joomla, because it's community.

avatar YatharthVyas
YatharthVyas - comment - 4 May 2021

I tested it for all cases and it seems to work fine.

Here's how it behaves when I've entered both rules as well as description:

image

avatar richard67
richard67 - comment - 4 May 2021

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

avatar YatharthVyas
YatharthVyas - comment - 4 May 2021

@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

avatar richard67
richard67 - comment - 4 May 2021

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

avatar Quy Quy - test_item - 4 May 2021 - Tested successfully
avatar Quy
Quy - comment - 4 May 2021

I have tested this item successfully on 07694c5

Awesome work! Thank you!!!


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

avatar richard67
richard67 - comment - 4 May 2021

Maybe it needs to adjust the testing instructions to the changes for other tester(s).

avatar YatharthVyas YatharthVyas - change - 5 May 2021
The description was changed
avatar YatharthVyas YatharthVyas - edited - 5 May 2021
avatar YatharthVyas YatharthVyas - change - 5 May 2021
The description was changed
avatar YatharthVyas YatharthVyas - edited - 5 May 2021
avatar YatharthVyas
YatharthVyas - comment - 5 May 2021

Testing Instructions have been updated.

avatar YatharthVyas YatharthVyas - change - 5 May 2021
The description was changed
avatar YatharthVyas YatharthVyas - edited - 5 May 2021
avatar saumyasarkar11 saumyasarkar11 - test_item - 5 May 2021 - Tested successfully
avatar saumyasarkar11
saumyasarkar11 - comment - 5 May 2021

I have tested this item successfully on 07694c5


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

avatar richard67 richard67 - change - 5 May 2021
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 5 May 2021

RTC


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

avatar chmst chmst - change - 6 May 2021
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: ?
avatar chmst chmst - close - 6 May 2021
avatar chmst chmst - merge - 6 May 2021
avatar chmst
chmst - comment - 6 May 2021

Thank you all, great team work

Add a Comment

Login with GitHub to post a comment