Opened 9 years ago
Closed 9 years ago
#32886 closed defect (bug) (fixed)
Better Passwords freezes Internet Explorer 8
Reported by: | afercia | Owned by: | markjaquith |
---|---|---|---|
Milestone: | 4.3 | Priority: | high |
Severity: | major | Version: | 4.3 |
Component: | Administration | Keywords: | needs-patch |
Focuses: | ui, javascript | Cc: |
Description
WordPress is still officially supporting Internet Explorer 8, though layout and visual things may visually degrade on occasion. Unfortunately looks like the new Better Passwords UI is not functional at all in IE 8, since this browser doesn't allow to change the 'type' of an input or button.
In the screenshots below you will notice the JS error in the console but the worst thing is even if you're fast enough to click on the "Show password" button IE 8 will completely freeze after a couple of seconds.
Attachments (21)
Change History (51)
#3
follow-up:
↓ 4
@
9 years ago
This seems ideal for feature detection, start with an input[type=password]
and try
setting it to input[type=text]
before adding the toggle.
Super-rough example at http://jsbin.com/tumefe/edit?html,js,output
JS Bin doesn't support old-ie, use http://output.jsbin.com/tumefe/quiet for testing in it.
#4
in reply to:
↑ 3
;
follow-up:
↓ 5
@
9 years ago
Thanks for the suggestion @peterwilsoncc!
So for ie8 users the feature would silently fail? it might be worth the extra effort to use two fields so ie8 users can still use this feature completely. Do you see a disadvantage to doing this?
Replying to peterwilsoncc:
This seems ideal for feature detection, start with an
input[type=password]
andtry
setting it toinput[type=text]
before adding the toggle.
Super-rough example at http://jsbin.com/tumefe/edit?html,js,output
JS Bin doesn't support old-ie, use http://output.jsbin.com/tumefe/quiet for testing in it.
#5
in reply to:
↑ 4
@
9 years ago
Replying to adamsilverstein:
So for ie8 users the feature would silently fail? it might be worth the extra effort to use two fields so ie8 users can still use this feature completely. Do you see a disadvantage to doing this?
That's what I was thinking, yes.
On further thought, your suggestion of a second field seems better. Disabling switching & defaulting to type=password
prevents generating the strong password. Which is bad.
#6
@
9 years ago
- Keywords has-patch dev-feedback added; needs-patch removed
In 32886.1.diff :
- toggle button replaces input field rather than changes attribute
- feature detection for
Element.setSelectionRange
This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.
9 years ago
#8
follow-up:
↓ 10
@
9 years ago
Thanks @peterwilsoncc - this is getting closer. I verified the switching between plain and password field works as expected. The only issue now is that because you are creating a new element entirely, we are losing the event handlers on the original element. This breaks things, for example the password meter no longer updates as you type.
I think the simplest solution is to clone create the new element at startup, attach the handlers to it as well as the original field, then have the swap routine simple show/hide instead of creating new elements each call. I can work on this in the next day or two if you don't get to it, thanks for your work so far!
This ticket was mentioned in Slack in #core-passwords by mikehansenme. View the logs.
9 years ago
#10
in reply to:
↑ 8
@
9 years ago
Replying to adamsilverstein:
That's a really daft miss, sorry about that.
I won't have a chance to refactor with a second field before the weekend, sorry. As an alternative, in 32886.3.diff
I have moved the events to the form and added a descendant selector. As you'll see in this reduced case IE8 doesn't bubble `input` events so I've added in the keyup
event.
With the keyup
event, the input
event may be unnecessary, feedback welcome if you chose this approach. My personal preference is to avoid hiding but happy to take an architects/feature leads direction on this.
#11
@
9 years ago
Based on the discussion in yesterday's core chat, several things need to change with the approach to the install screen.
We want to keep the install process as simple as possible (simpler!).
Here is the approach proposed in the meeting I gleaned from reading the slack logs.
- When loading install show the password initially
- Add text below password field instructing user to record the password, it's important, they need it to log in!
- No reset link on email
- Remove language telling user a password reset link will be emailed
- Make copying the password easier (maybe select contents on focus so you can tab in, hit copy), also on mobile!?
This ticket was mentioned in Slack in #core-passwords by adamsilverstein. View the logs.
9 years ago
#13
@
9 years ago
- Show the password on install screen initially
- Updated language instructing user they need the password to log in and to securely store it
- Verified copying password easy, tabbing into field selects password
- Includes fix for ie8 compatibility by syncing two fields, one text and one password, and showing/hiding based on adding/removing a parent class - need to test this in ie8, should work :)
Happy to work to merge these changes into the other ongoing work on passwords.
@ryan: Screenshots/screencast incoming!
#15
@
9 years ago
One issue I just noticed and haven't fixed: the Title field looses focus when the password setup fires.
Also, a question: should we offer a 'generate password' button in case the user wants to regenerate?
#16
@
9 years ago
In 32886.4.diff add styles to forms.css for password/text switching functionality so it works correctly on user profile page.
Another thing missing now: selection of text after hitting generate password.
This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by obenland. View the logs.
9 years ago
#19
@
9 years ago
- refresh against trunk after r33353
Some screencasts, Mac chrome - looking at these next to each other, I see some colors got adjusted, for contrast? Should we move all the colors to a central location, where both pages can load them?
This ticket was mentioned in Slack in #core-passwords by adamsilverstein. View the logs.
9 years ago
#24
@
9 years ago
tested 32886.11.diff on both install and profile page, tested generating password with lastpass, works great!
#26
@
9 years ago
- Keywords needs-patch added; has-patch dev-feedback removed
- Resolution fixed deleted
- Status changed from closed to reopened
It seems like there is a loop which still crashes IE8. (Not a regression after [33450].)
To reproduce add console.log('loooop');
before the setTimeout()
call in trunk/src/wp-admin/js/user-profile.js@33450#L52 and see how it fills the browser console. There is also a huge lag when entering a custom password.
#27
@
9 years ago
no-timeout.3.diff fixes the loop for me:
The stack still looks a bit different as in Chrome, but no loop.
// IE8 bindPasswordForm() bindPass1() bindToggleButton() check_pass_strength() $pass1.on( 'input propertychange') check_pass_strength() $pass1.on( 'input propertychange') same password detected check_pass_strength() $pass1.on( 'input propertychange') same password detected // Chrome: bindPasswordForm() bindPass1() bindToggleButton() check_pass_strength() $pass1.on( 'input propertychange')
#29
@
9 years ago
- Add a new event watch for
pwupdate
, causing the strength meter to update, similar to when the propertychange (ie8 only) or input (all other browsers) - Trigger
pwupdate
when plaintext password field changes - Fix some spaces -> tabs
- Removed the call to
$('#pass2').val('').on( 'input propertychange', check_pass_strength );
- I don't believe this is required as handle this at L161
Tested in browserstack ie8 & chrome, needs more testing!
This is caused because internet explorer 8 makes the type attribute read only. Two possible workarounds basically involve swapping out the element each time it is switched from password to text mode:
I think the second solution will lead to a more accessible, maintainable result.