Can't override autocomplete on PasswordField

[jellygnite]

Affected Version

v4.7.0

Description

I couldn't set the attribute 'autocomplete'='new-password' on the ConfirmPasswordField. Seems to only allow on or off.

I suggest transposing the two arrays in the array_merge below in PasswordField.php

public function getAttributes()
    {
        $attributes = [];

        if (!$this->getAllowValuePostback()) {
            $attributes['value'] = null;
        }

        $autocomplete = $this->config()->get('autocomplete');

        if ($autocomplete) {
            $attributes['autocomplete'] = 'on';
        } else {
            $attributes['autocomplete'] = 'off';
        }

        return array_merge(
            parent::getAttributes(),
            $attributes
        );
    }

to

        return array_merge(
            $attributes,
           parent::getAttributes()
          
        );

Apologies if this isn't

PRs

• IMPR: Allow to customize autocomplete attribute #9824

[emteknetnz]

Thanks for your suggestion! Just to let you know we're closing this feature request as GitHub issues are not the best place to discuss potential enhancements.

autocomplete has multiple valid option beyond on and off https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete. There would probably need to me more thought in to a robust solution rather than just flipping the order of array_merge().

You can read more about the guide on contributing issues and you are welcome to raise new feature ideas on the Silverstripe forum instead.

[kinglozzer]

If PasswordField only sets 2 keys, we could probably simplify the method by removing the array_merge:

public function getAttributes()
{
    $attributes = parent::getAttributes();

    if (!$this->getAllowValuePostback()) {
        $attributes['value'] = null;
    }

    if (!array_key_exists('autocomplete', $attributes)) {
        $attributes['autocomplete'] = $this->config()->get('autocomplete') ? 'on' : 'off';
    }

    return $attributes;
}

Fancy submitting a PR to fix this @jellygnite? It’s arguably a fix, so I can’t see any reason not to submit it against the 4.7 branch

[emteknetnz]

@kinglozzer I'm not sure about that solution above, because it would change the existing behavior so that a parent class value of 'autocomplete' would override any configuration value on the class for 'autocomplete'. This may break some existing sites.

To me it seems like a better solution would be to test if the config value of autocomplete looks true (true|1|on|yes) = 'on' or looks false (false|0|off}no) to not change behaviour on any existing sites, otherwise if the config value is set to something else such as 'new-password' then use that

[kinglozzer]

I'm not sure about that solution above, because it would change the existing behavior so that a parent class value of 'autocomplete' would override any configuration value on the class for 'autocomplete'. This may break some existing sites.

I’m not sure what you mean by this? There’s no default value set in the parent class (TextField), so someone would’ve had to manually called ->setAttribute('autocomplete', 'foo') for any value to be set. In that case, they’d be expecting that value to appear so if PasswordField is overwriting it with on/off then that’s a bug IMO.

otherwise if the config value is set to something else such as 'new-password' then use that

I’d rather avoid using the config value for anything other than a fallback when autocomplete hasn’t manually been set. Otherwise it becomes very difficult to set different autocomplete values for different instances of the field. E.g. you might have “old password” and “new password” in the same form - you’d want different autocomplete values for those.

[emteknetnz]

@kinglozzer ug, sorry my understanding of this was very poor. I confused setting a field on a parent class with simply setting PasswordField::create()->setAttribute('autocomplete', 'myvalue') - sorry about that!

Your approach does look good.

[kinglozzer]

No worries @emteknetnz 😄. I’ll re-open this and submit a PR soon if @jellygnite doesn’t pick this up first

[emteknetnz]

This might be worth a read before you got too far with this silverstripe/cwp-core#94