Closed Bug 1319176 Opened 3 years ago Closed 3 years ago

Handle clicks on the insecure field warning row

Categories

(Toolkit :: Password Manager, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox51 --- unaffected
firefox52 --- verified
firefox53 --- verified
firefox54 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

Details

Attachments

(4 files)

We need to properly invert colours when selected and possibly have enter open the learn more link.
Ryan, Do you want to open the SUMO article if the user selects (e.g. hits Enter on) the warning row?
Flags: needinfo?(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.
Flags: needinfo?(rfeeley)
If it's selectable, then open Control Center to the connection info sub-panel.
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.
Blocks: 1320401
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Duplicate of this bug: 1326224
(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?
Flags: needinfo?(timdream)
Flags: needinfo?(selee)
Flags: needinfo?(rfeeley)
Attached image 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
Flags: needinfo?(rfeeley)
(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.
(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?
Flags: needinfo?(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.
Flags: needinfo?(rfeeley)
Attached image 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?
To be clear, I meant to say I am not available before CNY holiday ends, that's from now to Feb 1st.
(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.
Flags: needinfo?(selee)
Attached video 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?
Flags: needinfo?(MattN+bmo)
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.
Attachment #8813903 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo)
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.
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 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.
Attachment #8813903 - Flags: review?(selee) → review+
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.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/96d0d062d978
Open the SUMO page when the insecure login warning is clicked. r=seanlee
https://hg.mozilla.org/mozilla-central/rev/96d0d062d978
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Hey Adrian, can you verify this on Nightly?
Flags: qe-verify+
Flags: needinfo?(adrian.florinescu)
Summary: Handle selection of the insecure field warning row → Handle clicks on the insecure field warning row
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
Attachment #8813903 - Flags: approval-mozilla-beta?
Attachment #8813903 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(adrian.florinescu)
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 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
Attachment #8813903 - Flags: approval-mozilla-beta?
Attachment #8813903 - Flags: approval-mozilla-beta+
Attachment #8813903 - Flags: approval-mozilla-aurora?
Attachment #8813903 - Flags: approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.