1319176 - Handle clicks on the insecure field warning row

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Matthew N. [:MattN]]

We need to properly invert colours when selected and possibly have enter open the learn more link.

Comment 1

[Matthew N. [:MattN]]

Ryan, Do you want to open the SUMO article if the user selects (e.g. hits Enter on) the warning row?

Comment 2

[Ryan Feeley [:rfeeley]]

No. It's just a header row (even when there are no children). We don't link from here to the web. Users can do that from Control Center if needed.

Comment 3

[Ryan Feeley [:rfeeley]]

If it's selectable, then open Control Center to the connection info sub-panel.

Comment 4

[Matthew N. [:MattN]]

Attachment: Bug 1319176 - Open the SUMO page when the insecure login warning is clicked.

Review commit: https://reviewboard.mozilla.org/r/95204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95204/

Comment 5

[Matthew N. [:MattN]]

I'm looking into both options:
* making the row not selectable. This should be easy with @disabled but the implementation seems to not handle that properly.
* opening the control center subview. Definitely possible but currently probably requires using a private member of gIdentityHandler.

Comment 7

[Tanvi Vyas[:tanvi]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #5)
> I'm looking into both options:
> * making the row not selectable. This should be easy with @disabled but the
> implementation seems to not handle that properly.
> * opening the control center subview. Definitely possible but currently
> probably requires using a private member of gIdentityHandler.

We've gotten complaints about the fact that selection of the row doesn't do anything.  And the above two are bit complicated to get into FF 52.  Ryan, for 52, can we just open a tab to the support page?  I believe this is what Chrome does.  Then in 53 we can work on the above two?  Otherwise the UX in 52 is not great.

Sean, Tim - if Ryan says yes, can one of you take the bug?

Comment 8

[Ryan Feeley [:rfeeley]]

Attachment: control-center.png

This is likely because it has a hover state, that makes it look clickable.

Options:
1. Remove hover effect
2. Open the control center to this spot? It contains the necessary link
3. Append sentence with "Learn more" blue link (spec to come) that opens SUMO in a new page

Comment 9

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #5)
> I'm looking into both options:
> * making the row not selectable. This should be easy with @disabled but the
> implementation seems to not handle that properly.

This apparently isn't supported in the subclass we're using and looks like a lot of work to support so I propose we keep it selectable but have it do something.

Comment 10

[Tanvi Vyas[:tanvi]]

(In reply to Ryan Feeley [:rfeeley] from comment #8)
> 3. Append sentence with "Learn more" blue link (spec to come) that opens
> SUMO in a new page

I thought you asked us to remove the "Learn more" blue link?

Comment 11

[Ryan Feeley [:rfeeley]]

The options are ranked by preference. The easiest thing to do for you (I imagine) is #1.

#2 would be more work, but preferable to #3.

Comment 12

[Ryan Feeley [:rfeeley]]

Attachment: learn-more.png

With the hover state, I think that the same colour is fine. I have made the "Learn more" font a medium weight (not bold, slightly less). Is this even possible across all platforms?

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am unfortunately will not be available after CNY (early Feb).

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

To be clear, I meant to say I am not available before CNY holiday ends, that's from now to Feb 1st.

Comment 15

[Matthew N. [:MattN]]

(In reply to Ryan Feeley [:rfeeley] from comment #8)
> This is likely because it has a hover state, that makes it look clickable.

Without the hover/selected state then users don't know where their selection is when they hit down arrow from the field.

> Options:
> 1. Remove hover effect

I think this just makes the problem worse for keyboard users since I don't think we can make it unselectable (see comment 9).

> 2. Open the control center to this spot? It contains the necessary link

That is the ideal solution but I'm not sure we have time for that in 52 as we don't have any prior art of opening control center directly to a subview.

> 3. Append sentence with "Learn more" blue link (spec to come) that opens
> SUMO in a new page

Do we even need the Learn More link? Why can't we just open a tab and keep it looking selectable?

Clearing needinfo since I will handle this for now.

Comment 16

[Ryan Feeley [:rfeeley]]

Attachment: down-arrow.mov

> Without the hover/selected state then users don't know where their selection is when they hit down arrow from the field.

It should go to the first selectable item. It's only a header row, like and optgroup label in HTML, or what's shown here on OS X.

Why would we allow focus of that row if it's not selectable?

Comment 17

[Tanvi Vyas[:tanvi]]

Spoke to Ryan today and we decided to add "Learn More" to end the of string, with no elipse or period.  The Learn More should be styled slightly differently - with medium text.  Selecting the warning row should open the sumo learn more page.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8813903 [details]
Bug 1319176 - Open the SUMO page when the insecure login warning is clicked.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95204/diff/1-2/

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8813903 [details]
Bug 1319176 - Open the SUMO page when the insecure login warning is clicked.

https://reviewboard.mozilla.org/r/95204/#review107708

::: toolkit/components/satchel/AutoCompletePopup.jsm:274
(Diff revision 2)
> -   * Despite its name, handleEnter is what is called when the
> -   * user clicks on one of the items in the popup.
> +   * Despite its name, this handleEnter is only called when the user clicks on
> +   * one of the items in the popup since the popup is rendered in the parent process.
> +   * The real controller's handleEnter is called directly in the content process

By the way Sean, I guess this will be a problem for form autofill too.

Comment 20

[Matthew N. [:MattN]]

This patch just handle clicks for now since that can be uplifted as-is to 52.

I will file follow-ups for the string changes since we need different patches for 52 and 53 there.

Comment 21

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8813903 [details]
Bug 1319176 - Open the SUMO page when the insecure login warning is clicked.

https://reviewboard.mozilla.org/r/95204/#review107736

Thanks for MattN to remind me the form autofill part.
The patch looks good to me except using `Enter` key to open the tab (but mouse click works.)
We need to file a new bug to fix the behavior.

Comment 22

[Matthew N. [:MattN]]

Thanks Sean.

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #20)
> I will file follow-ups for the string changes since we need different
> patches for 52 and 53 there.

I filed bug 1333256.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96d0d062d978
Open the SUMO page when the insecure login warning is clicked. r=seanlee

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/96d0d062d978

Comment 25

[Matthew N. [:MattN]]

Hey Adrian, can you verify this on Nightly?

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8813903 [details]
Bug 1319176 - Open the SUMO page when the insecure login warning is clicked.

Approval Request Comment
[Feature/Bug causing the regression]: Insecure password contextual warning
[User impact if declined]: Users will have a hard time finding out more information about the warning.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: Not yet, I requested so.
[Needs manual test from QE? If yes, steps to reproduce]: yes, straightforward from bug summary
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: Trivial click handler on the row
[String changes made/needed]: None

Comment 27

[ovidiu boca[:Ovidiu]]

I tested on Windows 10, Mac OS and Ubuntu 16.04 and the behaviour is the same on all os's. When you click on warning message you are redirect to mozilla support page, that is the expected behaviour. If you use the down or up arrow to focus the warning message and then you hit enter nothing happens, I think here after you hit enter you should be redirect to support page. This is the intended behavior?

Comment 28

[Matthew N. [:MattN]]

Yes, see comment 20.

Comment 29

[ovidiu boca[:Ovidiu]]

Thanks and sorry for that. 

I verified this on Windows 10, Mac OS 10.10 and Ubuntu 16.04 with FF Nightly 54.0a1(2017-01-25) and I can confirm the fix.

Comment 30

[Julien Cristau [:jcristau]]

Comment on attachment 8813903 [details]
Bug 1319176 - Open the SUMO page when the insecure login warning is clicked.

open sumo page when clicking the new insecure login warning in beta52, aurora53

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f5f482af2c79

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c920ed1ee977

Comment 33

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  53.0a1, build ID:2016112103022, on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 54.0a1 (2017.02.02), 52.0b2, 53.0a2 (2017.02.02) on Windows 8.1 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.