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

Remove chrome autofilling detection #7050

Merged

Conversation

jredrejo
Copy link
Member

Summary

After the latest changes in the Sign In page, an ugly message appears in the browser console:
TypeError: this$1.$refs.username is undefined
image

It was caused by a logic executed after the component is mounted to detect if Chrome had autofilled the password.
However:

  1. This was not working if you have a Chrome theme applied in the browser
  2. This is not working anymore as the username input that was used to detect the bg color has been removed from the component

This PR removes all the associated logic to this brittle detection.

Reviewer guidance

Can we assume removing this detection?

Contributor Checklist

PR process:

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Testing:

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

Reviewer Checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@jredrejo jredrejo requested review from cpauya and rtibbles June 12, 2020 17:01
@jredrejo jredrejo added this to the android-mvp milestone Jun 12, 2020
@jredrejo jredrejo added the TODO: needs review Waiting for review label Jun 12, 2020
@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #7050 into app-support will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted Files Coverage Δ
...plugins/user/assets/src/views/SignInPage/index.vue 29.44% <ø> (+0.34%) ⬆️
kolibri/core/content/utils/check_schema_db.py 100.00% <0.00%> (+8.69%) ⬆️

@indirectlylit
Copy link
Contributor

Before merging, let's test to make sure we haven't introduced a regression on #2673

ref: #3352

@mrpau-dev would you mind testing the new flows using a saved username and password in Chrome?

@cpauya
Copy link
Contributor

cpauya commented Jun 13, 2020

Hi @jredrejo I tried your branch in a fresh* Kolibri pipenv and got some errors in Chrome console.

  1. Error/s when filling-in the username then pressing for the password field.
  2. Error/s when just clicking on the Sign In button for the username and password fields.
  3. The saved username and password didn't seem to work as I expected them to auto-fill the username and password fields. The .gif shows Chrome asking to save the account and I did. When I logged-out, the username should have been filled right?

I auto-saved the account when Chrome asked, tried the import feature, logged-out, then did the login again.

2020-06-13-sat--chrome test jose pr 4

Some log files:
0.0.0.0-1592043224661.log
debug.txt
kolibri.txt

My env:

  • Deleted my ~/.kolibri/ folder and followed Setting up Kolibri for devt
  • Branch: jredrejo/remove_chrome_autofilling
  • Chrome v83.0
  • Python v3.7.7 on macOS v10.14.6

@cpauya
Copy link
Contributor

cpauya commented Jun 13, 2020

Just an FYI, I tested on release-v0.14.x branch and it also has the errors at the Chrome Console.

  1. This shows when I enter the username and press <Enter>.
  2. Also shows when I just click on the Sign In button for the username and password fields without filling-in.

Screen Shot 2020-06-13 at 7 40 52 PM

Console log file 0.0.0.0-1592048569201.log

@jredrejo
Copy link
Member Author

Hi @cpauya for the errors you see in the console, I don't know the reason for them but I am almost sure they are not related to this change. It seems a problem in the backend with the session.
In any case, I am not able to reproduce them. First of all: how did you do to have an input text to type the username? because in 0.14 I only see a button with the list of users :
image
This is the reason to remove the code I removed: the input does not exist anymore.
Would you mind executing make clean before testing the code, or even better, using a new fresk kolibri home folder?

@jredrejo
Copy link
Member Author

Before merging, let's test to make sure we haven't introduced a regression on #2673

@indirectlylit I have checked the password placeholder does not appear.
In any case, if it appears we need to find a new workaround. Current one uses the username input background color as a reference, and that input has disappeared after the new design to login has been implemented. That's the reason for the error you see in the browser console in the login screen in 0.14.

@cpauya
Copy link
Contributor

cpauya commented Jun 15, 2020

I ran make clean, then re-run yarn devserver but the same results. I still see the username textbox.

I've been using release-v0.14.x branch, latest commit 2b93b686.

I ran Clear browsing data in Chrome to delete cookies, etc. After that I deleted my ~/.kolibri/ folder. Then I just followed the instructions for Kolibri Setup for Devt. So that is my a "fresh" setup.

Then when Kolibri ran, I did a Quick setup, and when I logoff I am presented with a login screen asking for a username with pic like the one in my previous post.

Radina mentioned about app-mode and that you gave her this code to activate it:

kolibri manage shell
from kolibri.core.device.models import DeviceAppKey
DeviceAppKey.get_app_key()

I removed the dashes in the UUID, then went to http://localhost:8000/app/api/initialize/(app_key_here) - but I got a Page not found page instead.
Screen Shot 2020-06-15 at 6 41 28 PM

With @radinamatic's setup it's working. So with this: I may have missed something, a step, or a setting perhaps?

@indirectlylit
Copy link
Contributor

In any case, if it appears we need to find a new workaround. Current one uses the username input background color as a reference, and that input has disappeared after the new design to login has been implemented.

Yes of course. Presumably it would reference the password field instead.

Sorry @cpauya I don't have much guidance - perhaps post in slack?

In my testing, I can no longer reproduce the issue described in #2673 and addressed in #3352. I'm also noticing that the color has changed from yellow to blue, which would prevent the 250, 255, 189 hack from working anyway.

Here are some references for what the original issue was and how other people addressed it in case this re-surfaces in another form:

here's the current behavior in Kolibri that I see:

2020-06-16 16 58 23

@indirectlylit indirectlylit merged commit 3c45240 into learningequality:app-support Jun 17, 2020
@jredrejo jredrejo deleted the remove_chrome_autofilling branch June 17, 2020 08:06
@indirectlylit indirectlylit modified the milestones: android-mvp, 0.14.0 Jul 6, 2020
@jonboiser jonboiser removed the TODO: needs review Waiting for review label Aug 5, 2020
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

4 participants