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.0] Firefox: Correcting email field considered as a username field #18911

Merged
merged 3 commits into from
Dec 2, 2017

Conversation

infograf768
Copy link
Member

@infograf768 infograf768 commented Nov 29, 2017

See #18863

This patch should solve 2 issues (tested in Firefox Quantum)

  1. Displaying the email field after the username and before the password field makes the browser proposes passwords instead of mails in that field. The order was always correct in 3.x.

2. Autocomplete set to off is broken for the passwords fields because of the new js with an "eye" icon letting display in clear the password.
I.e. for example when user registers but also when creating a new user in backend or editing profile.
eye

Summary of Changes

  1. Installation and creating new user in backend: changing order of fields to normalizes that order as already existing for registration where the issue is not present:

Existing Registration:
registration

After patch

Back-end (the grey bar is not created by this PR):

backendusercreation

Installation:

installation

2. Correcting autocomplete off for passwords
In 3.x the code was
<?php // Disables autocomplete ?> <input type="password" style="display:none">
But as I guess the js to hide/show the password depends on the type, we just have to use instead
<input autocomplete="off" style="display:none"> when the field type is password
After patch, this is corrected in Installation, registration, creating new user in back-end and profile edit.

Testing Instructions

Installation:
Just install with Firefox using an already used/saved username that was recorded by Firefox.
I used "admin"
Test before patch (screenshots already available in #18863
After patch:
screen shot 2017-11-29 at 10 37 46
screen shot 2017-11-29 at 10 41 49
emailfield

Test display or not of password in clear when clicking on the "eye" icon

Registration
Allow registration by user in User Manager Options.
Create a Registration menu item in frontend.
(if you want to use Module Login form, you still have to create a menu item as Trying to register from the module is for now broken)

test before and after patch.

Test also editing profile and creating user in back-end before and after patch.

@Webdongle @dgt41

@@ -34,6 +34,9 @@
<?php echo $field->label; ?>
</div>
<div class="controls">
<?php if ($field->type === 'Password') : ?>
Copy link
Contributor

@Quy Quy Nov 29, 2017

Choose a reason for hiding this comment

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

Remove extra space before :. Fix in other instances too.

Is the comment necessary? The code next to it is self-explanatory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Extra space taken off.
I kept the comment as it was in 3.x
We can take it off if the PR is accepted. 😄

@Webdongle
Copy link
Contributor

I have not tested this item.

Tested Full Success


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

@brianteeman
Copy link
Contributor

This pr is not correct.

https://developer.mozilla.org/en-US/docs/Web/Security/Securing_your_site/Turning_off_form_autocompletion

@infograf768
Copy link
Member Author

infograf768 commented Nov 29, 2017

What is incorrect? It does prevent autocompletion as stated.
The browser still proposes possible values but it DOES NOT autocomplete.
Please explain what is wrong.

@mbabker
Copy link
Contributor

mbabker commented Nov 29, 2017

Introduces major accessibility issues by using hidden input fields for starters.

@brianteeman
Copy link
Contributor

Read the link I posted and then you will see why. It is written in plain English

@brianteeman
Copy link
Contributor

For those who don't want to click a link to read.

many modern browsers do not support autocomplete="off" for login fields:

If a site sets autocomplete="off" for a form, and the form includes username and password input fields, then the browser will still offer to remember this login, and if the user agrees, the browser will autofill those fields the next time the user visits the page.
If a site sets autocomplete="off" for username and password input fields, then the browser will still offer to remember this login, and if the user agrees, the browser will autofill those fields the next time the user visits the page.
This is the behavior in Firefox (since version 38), Google Chrome (since 34), and Internet Explorer (since version 11).

@dgrammatiko
Copy link
Contributor

This is the behavior in Firefox (since version 38), Google Chrome (since 34), and Internet Explorer (since version 11).

Also same for Safari. So basically ALL major browsers. I guess we shouldn't cancel default browsers behaviour, either we like it or not!

@brianteeman
Copy link
Contributor

You can of course change the firefox behaviour and tell it that you want it to obey the autocomplete setting by doing the following

  1. In a new tab, type or paste about:config in the address bar and press Enter. Click the button promising to be careful.

(2) In the search box above the list, type or paste signon and pause while the list is filtered

(3) Double-click the signon.storeWhenAutocompleteOff preference to switch it from true to false

@infograf768
Copy link
Member Author

Introduces major accessibility issues by using hidden input fields for starters.

that one from @mbabker may make sense. I am not a specialist of accessibility. Then why not modify it in 3.8 as we use there a similar code and this for ages?

@ others my test here show, for these specific pages, that Firefox does NOT autofill after saving the password. please just test.

in any case, if we do not merge this or something similar, it will autofill and therefore we will never have autocomplete off for joomla users.

but maybe that’s what some people want.

@dgrammatiko
Copy link
Contributor

@infograf768 autofill is irrelevant in the installer!

@brianteeman
Copy link
Contributor

Please note that password managers also do what browsers do and completely ignore the autocomplete=off setting on a password field - for obvious reasons

Your pr is just exploiting a "bug" that if the field before the password is empty then the browser doesnt know what to autofill the field with.

@brianteeman
Copy link
Contributor

The only valid place to disable autofill of the username and password is in the administrator usermanager

This can probably be achieved by simply giving the field a default value of space

  	default=" "

@brianteeman
Copy link
Contributor

From Microsoft in 2009

You might wonder: "Why does IE11 ignore the AUTOCOMPLETE attribute on login forms while continuing to respect the attribute on all other AutoComplete input elements (e.g. address, credit cards, phone #, etc)?"

The answer is that Password Managers improve real-world security, and the IE team felt it was important to put users in control. Users rely on their password manager to permit them to comfortably use strong passwords. Password managers encourage strong, unique password creation per site, but unique, strong passwords are often difficult to remember and type on touch devices. If the browser doesn't offer to autocomplete a password, the user assumes that the browser is broken. The user will then either use another browser that ignores the attribute, or install a password manager plugin that ignores it. I'm an example of this myself-- until IE11, I used a weak password on the site that publishes my book because I couldn't convince them to remove the AUTOCOMPLETE attribute from their login form. Now, with IE11, that worry is gone and I can use a strong password on that site without a major hassle.

source: https://blogs.msdn.microsoft.com/ieinternals/2009/09/10/why-wont-ie-remember-my-login-info/

@Webdongle
Copy link
Contributor

Webdongle commented Nov 29, 2017

If the user chooses to save the the form details then they save email/password not admin password.
Try this and see
Usenamer: anewuser
email: me@xxx.yyy.com
Password: qwerty

Save the form when prompted

Once installed you can not select autofill for
Usenamer: anewuser
Password: qwerty

The issue here is not how the autofill operates but the positioning of the fields in the form. At the moment the email field is paired with the password field and thus is being recognised as (and treated as) the username field.

Having the browser recognise the email field as the username field (that matches the password) is incorrect behaviour. By the user saving that combination they will have not autosaved the correct combination to the Joomla login.

The patch rectifies the problem caused by placing the fields correctly.

@Webdongle
Copy link
Contributor

Problem also exists in create new user ... the email is parsed to the username field of the autofill

new user 01

@infograf768 infograf768 changed the title [4.0] Correcting password autocomplete and email field considered as a password field [4.0] Correcting email field considered as a password field Nov 30, 2017
@infograf768
Copy link
Member Author

infograf768 commented Nov 30, 2017

I have now deleted all code implementing autocomplete to off.
Therefore, this PR just reorders where necessary the passwords and email fields.

Note: If autocomplete off should never be used in joomla, I suggest to take off all code related to that in core: xml, field library, etc. I let this to those who know better.

Note 2: I still consider not setting autocomplete to off for new registration, creating new user in back-end and profile edit is a mistake as long as we let autofill to yes for login, which is the case with the PR.

@Webdongle
Copy link
Contributor

Webdongle commented Nov 30, 2017

@brianteeman
You confusion arises because you think the name of the field defines the username stored in the login autofill for the browser. When in fact it is the field type=password that defines the field that defines the username stored in the login autofill for the browser.
https://www.weblinksonline.co.uk/testing/testinstall.htm

@brianteeman
Copy link
Contributor

I am not installing flash for anything

@infograf768
Copy link
Member Author

No need for flash.

Here is a screenshot before this patch:
screen shot 2017-11-30 at 17 16 30

Anyone who does not understand needs serious help.

@brianteeman
Copy link
Contributor

At no point in the OP do you ever say anything about the email address being stored as the username

@infograf768 infograf768 changed the title [4.0] Correcting email field considered as a password field [4.0] Correcting email field considered as a username field Nov 30, 2017
@infograf768
Copy link
Member Author

infograf768 commented Nov 30, 2017

Modified title to fit indeed. But @Webdongle explained it a lot

It was only correct in the sense that it was "proposing" passwords to fill the email field, which is a consequence of the wrong order.

@brianteeman
Copy link
Contributor

Is anything in your OP correct? Everything in the OP seems different to that commented on by @Webdongle

@infograf768
Copy link
Member Author

See #18911 (comment)

and please, let's not discuss this anymore (we call this in French Enculer les mouches)

You now know the problem and why we need to reorder the fields. Please test and mark the Test OK so that we can move on.

@Webdongle
Copy link
Contributor

Webdongle commented Nov 30, 2017

@brianteeman

At no point in the OP do you ever say anything about the email address being stored as the username

I did tell you ages ago #18863 (comment)
And #18863 (comment) but you refused to listen and locked that topic

it is irrelevant to your issue. your issue is your browser as explained.
I am now locking this

@infograf768 is saying the same as me but you are not seeing it.

When there is a password field the browser treats the field (immediately) in front of it as the login Password field. Consequently it offers to save the contents of the password field and the one field (immediately) in front of it as the login username field.

Thus with fields in the default.php
getLabel admin_user ... Type text
getLabel admin_email ... type email
getLabel admin_password ... type password

The field with getLabel admin_user is just a text field to the browser. The confusion is those who (mistakenly) think that because a text field has a getLabel admin_user that the browser autofills it with stored username data

The field with getLabel admin_email the field (immediately) in front of it the login Password field is stored as username by the browser. Thus the browser is using the getLabel admin_email as the field that contains the username which is to be saved for login purposes.

Their confusion arises because they think the name of the field defines the username stored in the login autofill for the browser. When in fact it is the field type=password which defines the field that defines the username stored in the login autofill for the browser.

@mbabker
Copy link
Contributor

mbabker commented Nov 30, 2017

At no point has it been communicated in this way. Everything before the last hour basically came across as a "the browser autocomplete is broken so we need to disable it because I find this confusing" commentary.

@infograf768
Copy link
Member Author

infograf768 commented Nov 30, 2017

@mbabker
this is because the original error (order of these fields) is masking the wrong proposals to fill the field.
In a way or another (not in the login forms evidently), we have to correct and set autofill to false to avoid getting automatically a passord\mail we used for another site when using same username. It is specially important on localhost. My solution may not have been the right one, but we need one.

@mbabker
Copy link
Contributor

mbabker commented Nov 30, 2017

we have to correct and set autofill to false to avoid getting automatically a passord\mail we used for another site when using same username

That is not our issue to fix (we are not going to be messing with browser behaviors). As long as we are setting form fields in the correct order we are doing our due diligence and anything further is going beyond what should be a reasonable scope of responsibility.

(And for what it is worth, password manager and autofill type stuff have logic to function based on the domain you're on, if I stored all of my joomla.org passwords under the root domain then I would have to manually select the right one for each subdomain but because they are correctly stored under a subdomain then I am only prompted the credentials for that specific site)

@Webdongle
Copy link
Contributor

@mbabker

At no point has it been communicated in this way. Everything before the last hour basically came across as a "the browser autocomplete is broken so we need to disable it because I find this confusing" commentary.

Eight days ago said it was the order of the fields that was the problem. It was Brian that turned the focus to the autofill

As long as we are setting form fields in the correct order we are doing our due diligence and anything further is going beyond what should be a reasonable scope of responsibility.

(And for what it is worth, password manager and autofill type stuff have logic to function based on the domain you're on, if I stored all of my joomla.org passwords under the root domain then I would have to manually select the right one for each subdomain but because they are correctly stored under a subdomain then I am only prompted the credentials for that specific site)

That is the point ... the autofill is being told to use the getLabel admin_email ... type email field as the Username in the Login screen. So it saves the email address as the username instead of the username. The default.php has the fields in the wrong order. The getLabel admin_email ... type email field is where the getLabel admin_user ... Type text should be.

@mbabker
Copy link
Contributor

mbabker commented Nov 30, 2017

*sigh*

We are not telling the autofill system what to use. The browser is determining that, as you have been so keen on pointing out. So yes, it is a browser limitation as it relates to autofill behaviors that we are patching around by changing our UI. It is NOT a Joomla bug. There is no "wrong order" as it relates to form fields (as far as data structure goes, UX and workflow is another thing altogether).

@Webdongle
Copy link
Contributor

@mbabker

We are not telling the autofill system what to use. The browser is determining that

Yes we are because the browser is 'determining' the value to be saved as the Login username because the default.php has the getLabel admin_email ... type email directly above the getLabel admin_password ... type password

@dgrammatiko
Copy link
Contributor

How about joomla follows common logic?
How about username === email address as the rest of the web?

@brianteeman
Copy link
Contributor

whoever knew that people other joomla might have a bug in their software

@mbabker
Copy link
Contributor

mbabker commented Nov 30, 2017

:eyeroll:

We are not communicating this to the browser. The browser is inferring it. End of story. There is no magical markup syntax that says "this field should be used as a username for autofill".

To be crystal clear. This patch, while it might fix an undesired behavior, IS NOT A BUG IN JOOMLA. Go yell at Mozilla if you don't like the way their browser's autofill system makes inferences. I suggest testing the autofill systems of other browsers or password managers and lodging the same complaint with them.

@brianteeman
Copy link
Contributor

https://stackoverflow.com/questions/10738090/why-firefox-autocomplete-even-with-different-input-name/10745884#10745884

So firefox does NOT do what it should do and store the field with the username - it tries to be clever and ignores the standards and saves the field before a password

@Webdongle
Copy link
Contributor

Webdongle commented Nov 30, 2017

We are not communicating this to the browser. The browser is inferring it. End of story. There is no magical markup syntax that says "this field should be used as a username for autofill".

Yes we are because we know that the field above the type=password is a 'reserved position' . It is 'Reserved' by the browser for storing the username. By putting the email field in the position that the browser uses to autofill Login Username you are supplying the browser with the wrong data to store.

There is no logic in separating the fields (that store the) Username and Password data and every reason to place the field (that stores the) Username data in a Position (that browsers reserve) for a Username.

@mbabker
Copy link
Contributor

mbabker commented Nov 30, 2017

I'm too stressed for this crap to keep going around in meaningless circles, and unless this project is going to pay for the hospitalization I am on my way toward from excessive stress, anxiety, and unhealthy weight loss, I am unsubscribing after this comment from this thread before I really do lose it this time.

You show me where in the HTML standard or ANY browser structure that there is a definition to say "this field should be used for the username in autofill". I will eat my own words if you can find the existence of such a standard. Until that exists, Joomla CANNOT communicate what you are saying it does. As I said, and as has been proven by your own actions, the browser is INFERRING this. AGAIN, NOT A JOOMLA BUG

@Webdongle
Copy link
Contributor

I'm too stressed for this crap to keep going around in meaningless circles,

So am I. When J4 is out and users cant login because email/password was (as a result of your refusal to accept default.php has the fields in the wrong order) saved ... then there will be a great big Told you so. One last time to try and explain where the error in your logic is. By placing a field (Username) where a program (browser) can't find it you are not supplying that program (browser) with the data it needs to produce the desired affect.

test

@Webdongle
Copy link
Contributor

install screen 05

@infograf768
Copy link
Member Author

@mbabker @dgt41 @brianteeman
I agree that it is a Firefox issue and not an issue in other browsers.

Now, if we can solve it for Firefox (as we did in 3.x) without creating issue for other browsers, why not do it? I just tested this PR in Safari and it saves username and password fine.

safari

I am sure you do not really want an important part of the user base to get into trouble just because the fields order is wrong for Firefox.

@infograf768
Copy link
Member Author

As a reminder, don't forget Joomla adapts itself to specific browsers like IE concerning CSS and JS.
Also for our queries where we adapt to postgresql.

Therefore it is not unreasonable to adapt to Firefox.

Thanks for your attention

@infograf768 infograf768 changed the title [4.0] Correcting email field considered as a username field [4.0] Firefox: Correcting email field considered as a username field Dec 1, 2017
@mbabker
Copy link
Contributor

mbabker commented Dec 1, 2017

To be clear, at no point have I suggested this not be merged now that the exact workflow and logic have been explained. But, it also needs to be clear that this is not the result of a bug in Joomla, no matter what some people want to believe. As stated and demonstrated, this is the result of a logic bug in the browser making an inference that causes an undesired workflow.

@infograf768
Copy link
Member Author

totally agree with you, @mbabker

I guess our former Plt remarked that issue, and this is the reason why it is coded this way in 3.x and before.
I propose to merge this and see if we can find a solution later for the autofill stuff in these specific pages without a new <input as done in 3.x

@infograf768
Copy link
Member Author

@wilsonge
Now that everyone understands the issue, could you please merge this?

@wilsonge wilsonge merged commit 3d8d440 into joomla:4.0-dev Dec 2, 2017
@wilsonge wilsonge added this to the Joomla 4.0 milestone Dec 2, 2017
@wilsonge
Copy link
Contributor

wilsonge commented Dec 2, 2017

Yup :) sorry for not replying on here earlier - it's been a very long week in the office!

@Webdongle
Copy link
Contributor

Webdongle commented Dec 2, 2017

@wilsonge

... it's been a very long week in the office!

Yep - been a long 10 days on here as well 😄 . Thanks

And thanks @infograf768 for sticking with me on this one and creating the patch.

@infograf768 infograf768 deleted the 40_pass_mail branch December 2, 2017 17:18
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.

None yet

9 participants