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

Enh: Autocomplete Attribute Enhancement #3652

Merged
merged 18 commits into from
Jan 19, 2024

Conversation

thabaum
Copy link
Contributor

@thabaum thabaum commented Jan 14, 2024

Autocomplete Attribute Enhancement Pull Request

Description

This PR implements the enhancement proposed in issue #[Issue_Number]: [Link to Enhancement Issue]

Changes Made

  • Added a new setting or property to profile properties to control the autocomplete attribute.
  • Implemented the logic to conditionally include the autocomplete attribute based on the new setting.
  • Also Adds the Autocomplete attribute for "name" and "email" to the user profile account tab input elements.

Related Issue

Closes #3651

Screenshots

image

image

How to Test

  1. Visit Profile Management
  2. Add text to the autocomplete input
  3. Visit your profile and view the source to see the autocomplete attributed added only if you have it set. Blank will not generate the autocomplete attribute.

Additional Context

I can set some default autocomplete settings for the default profile properties, or this can be done later or leave it to admins.

The order of where it is on the Profile Edit Form can be changed as well if requested.

This PR also refactored the code in the OnInitializedAsync method of the Index component to obtain user settings earlier, ensuring their availability for subsequent operations. SettingService.GetUserSettingsAsync
for input elements:
image
for select elements:
image

If the autocomplete is empty, this is the result for an example:

image

notice no "autocomplete" attribute included.

Cheers!

@sbwalker
Copy link
Member

@thabaum there are a number of functional issues with this PR:

  • the changes to the Profile model class are missing
  • the migration to add the new property to the database is missing

From a higher level the property is technical and could be confusing to users so it needs to be better explained in the help text ie. "The HTML autocomplete attribute allows you to specify browser behavior for automated assistance in filling out form field values"

@sbwalker
Copy link
Member

@thabaum from a backward compatibility perspective the new property should not affect legacy behavior - so by default, existing profile fields should not specify the autocomplete attribute.

@thabaum
Copy link
Contributor Author

thabaum commented Jan 15, 2024

@sbwalker I Forgot to upload the new file, and I want to add this to the "Options" element.

existing will not be included, because they are blank. Blank skips adding the autocomplete.

Thanks for the quick review, I was too busy yesterday to resolve these. I tested my script for migration and works as expected I am going to upload that file today I forgot. I will make that change for this enhancement to also be added to the options field for things like country and state/province.

The one thing I thought I could add that I wanted to ask that related to populated default values is adding to the create site script some default values. Not adding values to the migration script as this would make legacy behavior issues.

If that sounds like a way to go I can put in the common names used in the autocomplete for them. Or just let admins do this each time which may not be as fun.

So that is my one question back at you is whether or not to populate new sites with a value.

Again to be clear if it is set to autocomplete="" this attribute is not created and left out of the input and select ui elements.

@sbwalker
Copy link
Member

@thabaum I would prefer to NOT enable this for new sites - it should be opt-in for Administrators AFTER they create a site.

@sbwalker
Copy link
Member

@thabaum the framework does not use migration "scripts" so I assume you mean a code-based migration class.

@sbwalker
Copy link
Member

@thabaum just a note that it is generally not a good practice to submit an incomplete PR. In this case the PR does not even compile because the code references a property in a model which is not included. And the feature is not functional because it has no database support. So if I had merged the PR, it would have broken the dev branch for all developers.

@thabaum
Copy link
Contributor Author

thabaum commented Jan 15, 2024

@sbwalker my bad, it was a late night and I thought I had merged all the changes into this PR. I was trying to beat you to it here prior to review but your on the east coast.

It was an accident to forget this file again I had it working and will provide the last changes in a minute.

@thabaum
Copy link
Contributor Author

thabaum commented Jan 15, 2024

@sbwalker wow really dropped the ball on this PR with adding my property logic to it. I should of tested the build myself first, this was my bad and I will do this going forward, especially on these that add to database schema.

Ready to take for a test spin.

Thank you.

@sbwalker
Copy link
Member

@thabaum is there a reason why the database field is declared with a length of 200 and the input maxlength is 50? What is a realistic max field length for this item?

@thabaum
Copy link
Contributor Author

thabaum commented Jan 16, 2024

@sbwalker The only reason I did this was because it was allowed in the other fields as well in Profile so I followed it thinking it can be regulated with validation.

I am glad you touched on this as I wasn't sure if I should put 50, my original setting, but seen 200 set for others and followed that as a guide. I don't think anyone will need more than 30 characters. I can put this limit i the UI maxLength around 30 and explain in help text.

"The HTML autocomplete attribute allows you to specify browser behavior for automated assistance in filling out form field values, with a character limit of 30 to ensure concise and effective usage."

Let me know what you think this should be and I can make this adjustment. I think we will need to adjust the form itself to reflect any issues of something entered that is too long.

Below gives a lot of examples of use cases.

https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete

I made the changes to the UI input for autocomplete to only allow max length of 30 instead of 50 as it was to keep these short to the point and able to use all examples shown in Mozilla. 20 was too small, 50 maybe a bit much, 30 seemed to cover all examples. I have not adjusted this in the database until I hear a final ruling on how we wish to proceed.

I updated the database to 50 character limit.

I also want to be sure you can see I updated the Profile email and display name to hold "email" and "name" autocomplete attributes as well. However I can remove these if you feel it is not necessary or should be avoided. We cannot currently add these to the framework programmatically as an option. So I hard coded them in. I want to do this for the installation form as well so I don't have to type an email every time I test. The form can have an autocomplete itself along with the username, password, email address settings. I would love to not have to type these in every time developing.

Thanks again for taking your time to review and provide our course of action to allow it to merge with the Oqtane Framework.

@sbwalker
Copy link
Member

sbwalker commented Jan 18, 2024

@thabaum there are actually 3 different use cases where a user profile UI is shown... and there are 4 different razor components:

  1. Oqtane.Client/Modules/Admin/UserProfile/Index.razor - user profile UI for registered users
  2. Oqtane.Client/Modules/Admin/Register/Index.razor - registration form for new users (if enabled in Site Settings)
  3. Oqtane.Client/Modules/Admin/Users/Add.razor - create new user UI for admins
  4. Oqtane.Client/Modules/Admin/Users/Edit.razor - edit existing user UI for admins

Correct me if I am wrong... but the autocomplete feature would only be applicable to cases where a user is creating/managing their own profile - not cases where an admin is managing profiles for other users. So this means it should be implemented for 1 & 2 but not 3 or 4. Currently the logic is consistent across all 4 razor components - but with the addition of this enhancement, the logic will diverge... which means care will need to be taken in the future when addressing issues in this area.

@thabaum
Copy link
Contributor Author

thabaum commented Jan 18, 2024

@sbwalker I will review this more closely tomorrow, as you are correct Admins should not have autocomplete attribute to allow the browser to auto fill in the form, but a user of any type should if editing their own personal profile properties. I will see what I can do to control the logic if admin or host is viewing and it is not their profile they are looking at Oqtane will not add this attribute.

@sbwalker
Copy link
Member

@thabaum you don't need to modify the logic, as you only implemented autocomplete in Oqtane.Client/Modules/Admin/UserProfile/Index.razor - which only allows a user to manage their own profile. However it should be implemented in Oqtane.Client/Modules/Admin/Register/Index.razor as well - as that is the registration form for anonymous users.

@thabaum
Copy link
Contributor Author

thabaum commented Jan 18, 2024

@sbwalker I went ahead and added this to the installer.razor file since we are going ahead with adding these autocomplete attribute to missing user related input elements that relate to a user profile. For developing I will enjoy not typing the same email over and over ;)

@sbwalker
Copy link
Member

sbwalker commented Jan 18, 2024

@thabaum my apologies... I was mistaken about Oqtane.Client/Modules/Admin/Register/Index.razor - it does not allow the user to enter any Profile fields other than the default user fields - so it does not need any autocomplete modifications.

The goal of this enhancement is that it does NOT force any change in behavior in the framework. So autocomplete should ONLY be added to Profile fields where the administrator has the ability to enable/disable the feature. It should NOT be included on non-Profile fields, as there is no way to control its existence. So it should be removed from username, email, etc... as this would force a change in behavior which we are trying to avoid.

@thabaum
Copy link
Contributor Author

thabaum commented Jan 18, 2024

@sbwalker OK, I will remove these, but I liked the results when testing in edge. I will clean up all the autocomplete attributes hard coded for now.

@thabaum
Copy link
Contributor Author

thabaum commented Jan 18, 2024

@sbwalker I have not removed the ones from the installer thinking maybe this one can be left without hurting the behaviors since it is only the installer page? I fixed a typo on that page anyhow I will leave, but just wondering if this would be a breaking behavior change even though it is just with the installer page? If so I can remove the username and email autocomplete I recently just added.

Other than that this PR just handles the profile management by admins to include autocomplete on the profile elements they create.


On another note:

Two issues I can see you have pointed out that we can make a solution for.

Issue 1.

Register page does not allow the ability to include profile properties created.

Solution:

We include a "Profile" property setting that can be set for each profile property created to enhance the legacy forms or not. A Yes/No UI control used can be a select, a checkbox or radio buttons for these two options: "Include in Registration Form?" and if "Is Required?".

Issue 2.

Many legacy form elements are missing the autocomplete attribute.

Solution:

We add a "Site" property setting that "Enhance Site Forms Autocomplete" which would enable all the autocomplete attributes in Oqtane form elements to avoid a change in behavior unless it is desired.

Only form we may not be able to dynamically modify is the installer.razor form which may not be able to get the property setting to know whether or not to include the autocomplete attribute.

I believe we can merge this one as is, and I can work on a second enhancement I believe we could introduce these next if these either of these solutions would be acceptable.

@thabaum
Copy link
Contributor Author

thabaum commented Jan 19, 2024

I went ahead and removed the hard coded changes to this PR but left the spelling correction. I will keep all this for a separate time since it is really not needed. Also if you like I can create another PR that only has these changes now showing.

I will be focused on the file upload issues this weekend.

@sbwalker
Copy link
Member

sbwalker commented Jan 19, 2024

@thabaum I am going to merge this as is.

@sbwalker sbwalker merged commit ab08732 into oqtane:dev Jan 19, 2024
1 check passed
@sbwalker
Copy link
Member

sbwalker commented Jan 19, 2024

@thabaum for Issue 2 above are you saying that most legacy forms in Oqtane are missing the autocomplete="off" specification - and it should be added to prevent undesired behavior? This would actually make sense as the Oqtane issues raised in the past where field values in hidden sections have been overridden and caused problems may have been a result of this (or they could also have been a result of browser password manager plugins which also auto populate fields unexpectedly).

@sbwalker
Copy link
Member

sbwalker commented Jan 19, 2024

@thabaum I have been thinking a lot about this enhancement. I feel like it is the beyond the technical abilities of most administrators in terms of how to use it effectively. Basically they would need to do some research into the HTML autocomplete attribute to understand the behavior and to determine the appropriate standardized taxonomy values to specify in order to get any benefit. So as a result, in the majority of cases it will not be utilized. In general I am not a big fan of maintaining code long term unless a feature provides sufficient value to a large number of users. So I have been wrestling with how to get more value from this.

Despite the fact that modern browsers are inconsistent in their behavior for assisting users with populating forms, they all seem to attempt to provide this functionality by default (ie. it is enabled by default which sometimes leads to poor outcomes). The autocomplete attribute essentially allows developers to exert some control over the behavior by providing hints to the browser.

So perhaps the addition of these attributes in Oqtane should not be viewed as a breaking behavioral change but rather as a fix to an existing UX issue. If we took this perspective then we do not need an option to enable the new functionality - it becomes the expected/default functionality. For example, it would make sense to include default autocomplete values for the default Profile fields when creating a new Site. And it would make sense to include them on the non-Profile input elements for username, email, etc... in the specific forms where a user is expected to enter their own personal information. And the majority of other forms in Oqtane for non-personal information should specify autocomplete="off" ( as you suggested above). And if we did this, then the enhancement would provide much more value and would be used by the majority of users.

Perhaps this is what you were trying to say all along... however I did not have the time to really concentrate on this item until today.

This article was helpful: https://developer.mozilla.org/en-US/docs/Web/Security/Securing_your_site/Turning_off_form_autocompletion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enh: Profile Autocomplete Attribute Added To Profile Properties
2 participants