Closed Bug 1175273 Opened 5 years ago Closed 5 years ago

A click anywhere at the login capture doorhanger should take the focus out of the password field.

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
mozilla41
Iteration:
41.3 - Jun 29
Tracking Status
firefox38.0.5 --- unaffected
firefox39 --- wontfix
firefox40 --- verified
firefox41 --- verified

People

(Reporter: rittme, Assigned: rittme)

References

Details

Attachments

(1 file)

While focused on the password textbox, clicking anywhere else at the login capture doorhanger should unfocus the field.
This would be helpful to user needing to quickly hide their password content.
Flags: qe-verify+
Flags: firefox-backlog?
Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger
Attachment #8623393 - Flags: review?(MattN+bmo)
Flags: firefox-backlog? → firefox-backlog+
Comment on attachment 8623393 [details]
MozReview Request: Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger. r=MattN

https://reviewboard.mozilla.org/r/11509/#review9927

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:875
(Diff revision 1)
> +      // Removes focus from textboxes when we click elsewhere at the doorhanger.

Nit: s/at/on/ sounds more natural to me.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:957
(Diff revision 1)
> +              chromeDoc.getElementById("password-notification").parentElement
> +                       .addEventListener("click", onNotificationClick);

I'm a bit worried that this resolves to `<popupset id="mainPopupSet">` if the popup is in the wrong state but I guess that shouldn't happen in the showing event handler as I believe it's synchronous.
This seems to be resolving to the <panel> when run from the Browser Toolbox though which is higher in the tree than I would like. Were you seeing something different?

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:877
(Diff revision 1)
> +        chromeDoc.getElementById("password-notification-password").blur();
> +      }
> +      if (clickEvent.explicitOriginalTarget.id != "password-notification-username") {
> +        chromeDoc.getElementById("password-notification-username").blur();

Nit: `clickEvent.explicitOriginalTarget.blur();`?

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:874
(Diff revision 1)
> +    let onNotificationClick = (clickEvent) => {
> +      // Removes focus from textboxes when we click elsewhere at the doorhanger.

I wonder if there could be issues about calling blur if neither of the password fields are focused (e.g. a button is). I think we should check the focused element is one of the textboxes (even just checking the type of element) before calling blur to be safe.
Attachment #8623393 - Flags: review?(MattN+bmo)
https://reviewboard.mozilla.org/r/11509/#review9939

> I'm a bit worried that this resolves to `<popupset id="mainPopupSet">` if the popup is in the wrong state but I guess that shouldn't happen in the showing event handler as I believe it's synchronous.
> This seems to be resolving to the <panel> when run from the Browser Toolbox though which is higher in the tree than I would like. Were you seeing something different?

The problem I had is that the only thing between both elements is the box `.panel-arrowcontent` and I can't seem to capture clicks on it. I didn't see a problem of having it this high in the tree because the clicks out of the panel still get ignored. 
Maybe to be sure it resolves to the good element I can move the call to the `shown` event, or simply select the panel directly through it's id.
https://reviewboard.mozilla.org/r/11509/#review9941

> Nit: `clickEvent.explicitOriginalTarget.blur();`?

I think we want exactly the opposite behavior. We want the target to be anything other than the element we will blur. i.e. if I click the password textbox it's not going to blur, but if I click anywhere else it will.
https://reviewboard.mozilla.org/r/11509/#review9943

> I think we want exactly the opposite behavior. We want the target to be anything other than the element we will blur. i.e. if I click the password textbox it's not going to blur, but if I click anywhere else it will.

Oops, you're right.

> The problem I had is that the only thing between both elements is the box `.panel-arrowcontent` and I can't seem to capture clicks on it. I didn't see a problem of having it this high in the tree because the clicks out of the panel still get ignored. 
> Maybe to be sure it resolves to the good element I can move the call to the `shown` event, or simply select the panel directly through it's id.

Oh, I thought `.panel-arrowcontent` is what I tested with in the Browser Toolbox earlier.

It does seem to work fine like you say. We'll see if it causes any problems.
Attachment #8623393 - Flags: review?(MattN+bmo)
Comment on attachment 8623393 [details]
MozReview Request: Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger. r=MattN

Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger
https://reviewboard.mozilla.org/r/11509/#review9949

> I wonder if there could be issues about calling blur if neither of the password fields are focused (e.g. a button is). I think we should check the focused element is one of the textboxes (even just checking the type of element) before calling blur to be safe.

I'm not sure if you were just checkpointing or missed this. An example: if the main action button is focused and the suer clicks on the empty part of the panel is there harm in calling .blur on both fields? It seems like we should only call blur on a field if it's the focused element. Services.fm (focus manager) can help you with this I believe.
Comment on attachment 8623393 [details]
MozReview Request: Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger. r=MattN

Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger
https://reviewboard.mozilla.org/r/11509/#review9951

> I'm not sure if you were just checkpointing or missed this. An example: if the main action button is focused and the suer clicks on the empty part of the panel is there harm in calling .blur on both fields? It seems like we should only call blur on a field if it's the focused element. Services.fm (focus manager) can help you with this I believe.

I missed it, this new patch checks if the focused field is an `html:input`. Do you think this would be enough?
Comment on attachment 8623393 [details]
MozReview Request: Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger. r=MattN

https://reviewboard.mozilla.org/r/11509/#review9953

I think this is an improvement but this doesn't solve some cases I think:

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:878
(Diff revisions 2 - 3)
> -        chromeDoc.getElementById("password-notification-password").blur();
> +          chromeDoc.getElementById("password-notification-password").blur();
> -      }
> +        }
> -      if (clickEvent.explicitOriginalTarget.id != "password-notification-username") {
> +        if (clickEvent.explicitOriginalTarget.id != "password-notification-username") {
> -        chromeDoc.getElementById("password-notification-username").blur();
> +          chromeDoc.getElementById("password-notification-username").blur();

This still means we will blur twice (one unnecessarily) if an input is focused. Ideally both `if` clauses should check that the element being blurred is actually focused.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:876
(Diff revisions 2 - 3)
> +      if (Services.focus.focusedElement.nodeName == "html:input") {

Can `Services.focus.focusedElement` be `null`  when a click happens?
Attachment #8623393 - Flags: review?(MattN+bmo)
Comment on attachment 8623393 [details]
MozReview Request: Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger. r=MattN

Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger
Attachment #8623393 - Flags: review?(MattN+bmo)
Attachment #8623393 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8623393 [details]
MozReview Request: Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger. r=MattN

https://reviewboard.mozilla.org/r/11509/#review9955

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:878
(Diff revision 4)
> +        // Nothing is focused so there is nothing to blur.

Oops, I made a mistake here. How about:
// No input is focused so we don't need to blur
Comment on attachment 8623393 [details]
MozReview Request: Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger. r=MattN

Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger. r=MattN
Attachment #8623393 - Attachment description: MozReview Request: Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger → MozReview Request: Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger. r=MattN
Attachment #8623393 - Flags: review+ → review?(MattN+bmo)
Attachment #8623393 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/53f268e9ab04
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
QA Contact: kjozwiak
Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-07-02-03-02-07-mozilla-central/

OS's Used:

* Win 8.1 x64
* Ubuntu 14.04.2
* OSX 10.10.4

- ensured that clicking on the "you can access your password via sync" will hide the password when it's visible
- ensured that you can still left & right click in the password field without the password being hidden
- ensured that you can tab from the password field and the contents in the password field will be hidden
- ensured that when you click on a webpage, the doorhanger is dismissed and the contents in the password field are hidden
- ensured that left & right clicking anywhere on the doorhanger will hide the contents in the password field
- ensured that clicking on the "X" will dismiss the doorhanger and hide the contents in the password field
- ensured that the contents of the password field are being hidden when the drop down menu in the doorhanger is selected
- ensured that hold & clicking on the "X" or the drop down menu hides the contents of the password field if visible
Depends on: 1179870
Comment on attachment 8623393 [details]
MozReview Request: Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger. r=MattN

Approval Request Comment
[Feature/regressing bug #]: Bug 1169702
[User impact if declined]: It's harder for users to hide their revealed password. Since passwords can be sensitive to users, I really think this should go out with bug 1169702.
[Describe test coverage new/current, TreeHerder]: QE verified and baked on Aurora already.
[Risks and why]: Low risk as bugs would probably leads to reverting to older behaviour of not showing the password.
[String/UUID change made/needed]: None
Attachment #8623393 - Flags: approval-mozilla-beta?
Comment on attachment 8623393 [details]
MozReview Request: Bug 1175273 - Removes focus from the textboxes when clicking somewhere else at the capture doorhanger. r=MattN

Verified in 41,  small changes in term of lines of code, taking it.
Attachment #8623393 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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 16 on Firefox 40 Beta 4 (20150713153304) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.