Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.4] Fixed missing maxlength attribute in textarea.php #34646

Open
wants to merge 23 commits into
base: 4.4-dev
Choose a base branch
from

Conversation

machadoug
Copy link

Summary of Changes

The Max Length was printing just the NUMBER as the attribute instead of maxlength="NUMBER"

Testing Instructions

See screenshot

Actual result BEFORE and AFTER applying this Pull Request

fixed-textarea-max-length-attribute.mp4

Documentation Changes Required

None

The Max Length was printing just the NUMBER as the attribute instead of maxlength="NUMBER"
Co-authored-by: Quy <quy@fluxbb.org>
@Quy
Copy link
Contributor

Quy commented Jun 29, 2021

I am not able to reproduce this issue.

Created a custom text area field with maxlength value.
The markup has the maxlength attribute.

@machadoug
Copy link
Author

@Quy , according to the textarea field layout code this will only work if the $maxlength has the entire attribute instead of just an integer, for example, if the $maxlength = 'maxlength="1000"'

I just noticed there's no @var documentation, but IMHO it should work just like the other variables, which contain the value only instead of the attribute_name="attribute_value"

@Quy
Copy link
Contributor

Quy commented Jun 29, 2021

Your PR will break the custom textarea field. See markup.

<textarea name="jform[com_fields][text-area]" id="jform_com_fields_text_area"   class="form-control"           maxlength=" maxlength="1000""   ></textarea>

@brianteeman
Copy link
Contributor

The character count requires two attributes in the xml

				maxlength="160"
				charcounter="true"

@Quy
Copy link
Contributor

Quy commented Jul 5, 2021

@machadoug You have the same issue with rows (4) and cols (20) attributes displaying only the values. It is not happening in core so it must be on your end. Please provide steps/code to reproduce.

@Fedik
Copy link
Member

Fedik commented Jul 5, 2021

@machadoug I suspect you use the textarea layout with your own field,
because for core textarea field the maxlength is set in the class:

$rows = $this->rows ? ' rows="' . $this->rows . '"' : '';
$maxlength = $this->maxlength ? ' maxlength="' . $this->maxlength . '"' : '';
$extraData = array(
'maxlength' => $maxlength,
'rows' => $rows,
'columns' => $columns,
'charcounter' => $this->charcounter
);

It looks strange to me, but it works like that since ~3.7 version

@machadoug
Copy link
Author

I was indeed using a custom Textarea field in my extension. I was under the impression that we should use only the attribute value, not attribute="value". Like the onchange, onclick and autocomplete attributes.

Maybe we should include something in the code documentation explaining it's a string and not an integer?

 * @var   string   $rows         rows attribute for the field.
 * @var   string   $cols         cols attribute for the field.
 * @var   string   $maxlength    maxlength attribute for the field.

@Fedik
Copy link
Member

Fedik commented Jul 5, 2021

I was under the impression that we should use only the attribute value, not attribute="value". Like the onchange, onclick and autocomplete attributes.

That is correct expectation.
But I cannot say why it like that for textarea.

Would be good to fix it. Only need to care about backward compatibility here.
The fix should move this

$columns = $this->columns ? ' cols="' . $this->columns . '"' : '';
$rows = $this->rows ? ' rows="' . $this->rows . '"' : '';
$maxlength = $this->maxlength ? ' maxlength="' . $this->maxlength . '"' : '';

to the layout.

And in layout doing something like:

$maxlengthAttr = '';

if ($maxlengt)
{
  $maxlengthAttr = is_numeric($maxlength) ? ' maxlength="' . $maxlength . '"' : $maxlength;
}

The same for rows and cols attribute.

Copy link
Author

@machadoug machadoug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added attributes to doc block and fixed according to @Fedik suggestions.

I just didn't include the changes to joomla-cms/libraries/src/Form/Field/TextareaField.php because I don't know how to include it to this PR.

@Fedik
Copy link
Member

Fedik commented Jul 10, 2021

@machadoug I have added that changes to your PR

It ready for testing:
Make sure textarea field still working as before.

@Quy Quy added the JBS label Jan 16, 2022
@chmst chmst changed the base branch from 4.0-dev to 4.1-dev January 31, 2022 15:47
@HLeithner HLeithner removed the psr12 label Oct 23, 2022
@Hackwar Hackwar added Small A PR which only has a small change and removed Conflicting Files labels Feb 21, 2023
@Quy Quy changed the base branch from 4.2-dev to 4.3-dev March 24, 2023 01:44
@Quy Quy added PR-4.3-dev and removed PR-4.2-dev labels Mar 24, 2023
@Quy Quy removed the PBF Pizza, Bugs and Fun label Mar 25, 2023
@Hackwar Hackwar added the bug label Apr 6, 2023
@Hackwar Hackwar added the PBF Pizza, Bugs and Fun label Aug 25, 2023
@HLeithner HLeithner changed the base branch from 4.3-dev to 4.4-dev September 30, 2023 22:45
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 4.4-dev.

@MacJoom MacJoom self-assigned this Oct 24, 2023
@MacJoom MacJoom enabled auto-merge (squash) October 30, 2023 12:03
@MacJoom MacJoom disabled auto-merge October 30, 2023 12:04
@HLeithner HLeithner changed the title Fixed missing maxlength attribute in textarea.php [4.4] Fixed missing maxlength attribute in textarea.php Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug PBF Pizza, Bugs and Fun PR-4.4-dev Small A PR which only has a small change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet