1174815 - Capture doorhanger password field always shows caret at the beginning when clicked.

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Bernardo Rittmeyer [:rittme]]

Bug 1153217 allow editing the password content at the capture doorhanger and bug 1169702 allows showing it. When the field is clicked for editing the caret is always positioned at the beginning of the content and not based on the click position.

Comment 1

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: Bug 1174815 - Caret in capture doorhanger password field is now positioned at the click position offset

Bug 1174815 - Caret in capture doorhanger password field is now positioned at the click position offset

Comment 2

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8622816 [details]
MozReview Request: Bug 1174815 - Caret in capture doorhanger password field is now positioned at the click position offset

Bug 1174815 - Caret in capture doorhanger password field is now positioned at the click position offset

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8622816 [details]
MozReview Request: Bug 1174815 - Caret in capture doorhanger password field is now positioned at the click position offset

https://reviewboard.mozilla.org/r/11447/#review9817

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:867
(Diff revision 2)
> -      chromeDoc.getElementById("password-notification-password").type = "";
> +      var passwordField = chromeDoc.getElementById("password-notification-password");
> +      var selectionStart = passwordField.selectionStart;
> +      var selectionEnd = passwordField.selectionEnd;

Nit: `let` is the new `var`

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:874
(Diff revision 2)
> +      passwordField.removeAttribute("type");

Why did you switch from using the setter to using the attribute? It's generally preferred to use the former.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:868
(Diff revision 2)
> +      var selectionStart = passwordField.selectionStart;
> +      var selectionEnd = passwordField.selectionEnd;

Add a comment above this about what this selection code is achieving.

Comment 4

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8622816 [details]
MozReview Request: Bug 1174815 - Caret in capture doorhanger password field is now positioned at the click position offset

Bug 1174815 - Caret in capture doorhanger password field is now positioned at the click position offset

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8622816 [details]
MozReview Request: Bug 1174815 - Caret in capture doorhanger password field is now positioned at the click position offset

https://reviewboard.mozilla.org/r/11447/#review9887

Ship It!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d2eb65fdebb3

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8622816 [details]
MozReview Request: Bug 1174815 - Caret in capture doorhanger password field is now positioned at the click position offset

Approval Request Comment
[Feature/regressing bug #]: This should get fixed in the same release as bug 1143903 and bug 1145913 to avoid confusing and churning of the login capture doorhanger UX.
[User impact if declined]: The caret will be in a non-standard position upon focusing the password field.
[Describe test coverage new/current, TreeHerder]: Manual testing on Nightly (email sent to passwords-dev).
[Risks and why]: Fairly straightforward pattern of toggling the field type. Worst case would likely be continued incorrect caret position which wouldn't be a big deal.
[String/UUID change made/needed]: None

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d2eb65fdebb3

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8622816 [details]
MozReview Request: Bug 1174815 - Caret in capture doorhanger password field is now positioned at the click position offset

Improve a new feature, taking it for 40.

Comment 10

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-06-16-03-02-01-mozilla-central/

While clicking on the password field, the cursor would be placed at the beginning of the content every time.

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-06-19-03-02-04-mozilla-central/

- ensured that when you click on the password field, the cursor is placed at the end of the content
- ensured that when you click on the actual password content, the cursor is placed in the correct position
- ensured that all the content under the password field is selected when you tab into the password field
- ensured that the cursor is placed at the end of the content in the password field when it's really long and spans further than the field

OS's Used:

- OSX 10.10.3 x64
- Win 8.1 x64 (VM)
- Ubuntu 14.04.1 x64 (VM)

I also created bug # 1176257 which is a possible trivial issue relating to the cursor not being visible to the user when the password is really long.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ca8577606c6

Comment 12

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 41.0a1 (2015-06-16) under Windows 7 64-bit.
Verified fixed following the all scenarios from Comment 10 on Firefox 40 Beta 4 (20150713153304) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5.