1130447 - Hide the password manager timeLastUsed column by default

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[aleth [:aleth]]

In in-content preferences, e.g. click on "Saved passwords" in the Security tab. The resulting popup dialog does not make good use of the available screen space. In particular, it is so narrow that all the column entries are cut off and hard to read, and the height is so small that only a few entries are visible at any one time.

It would be better if the dialog size was calculated relative to the size of the window.

Comment 1

[Fischer [:Fischer]]

@Jared,
About the dialog window size bugs, do we have any approach or process to decide the size ?
Because this kind of thing is a bit subjective, we need some metric or discussion to see if this is really a issue for most users.
Thanks.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

I don't know of any, but maybe Ryan Feeley knows of some best practices here?

Comment 3

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

(In reply to Fischer [:Fischer] from comment #1)
> @Jared,
> About the dialog window size bugs, do we have any approach or process to
> decide the size ?
> Because this kind of thing is a bit subjective, we need some metric or
> discussion to see if this is really a issue for most users.
> Thanks.

IMHO, just come up with your own default width/height, put a screenshot here, and ask for ui-review.

We can always improve the overall experiences later when there is a new spec.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #3)
> (In reply to Fischer [:Fischer] from comment #1)
> > @Jared,
> > About the dialog window size bugs, do we have any approach or process to
> > decide the size ?
> > Because this kind of thing is a bit subjective, we need some metric or
> > discussion to see if this is really a issue for most users.
> > Thanks.
> 
> IMHO, just come up with your own default width/height, put a screenshot
> here, and ask for ui-review.

I agree, this is a sufficient approach as well.
> 
> We can always improve the overall experiences later when there is a new spec.

Comment 5

[Fischer [:Fischer]]

@Tina & Jack,
Please check the size of dialog window inside Preference page.
Let us know if the size is fine or needs adjustment.
Thanks

Comment 6

[Ryan Feeley [:rfeeley]]

I have no idea how these sizes are decided, and neither does shorlander. I'm fine with an increase in size FWIW.

Comment 7

[Tina Hsieh[:Tina_Hsieh] UX]

Hi Fischer,

Currently we don't have a guideline for window size, so we're going to use existing window size to keep consistency. Please follow these two ways to avoid truncated content:
1. Increase the size of the window. Follow the size of the Exceptions window under Security>General
2. Show Site and Username by default only. Hide Last Used & Last Changed until a user turns it on.

Thank you :)

Comment 10

[Fischer [:Fischer]]

Attachment: Bug 1130447 - Hide the password manager timeLastUsed column by default.

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

Comment 11

[Fischer [:Fischer]]

Comment on attachment 8792821 [details]
Bug 1130447 - Hide the password manager timeLastUsed column by default.

Hi Matt,
This patch makes some improvement on the Saved Logins dialog based on comment #7:
  - Enlarge the width the same as that of the Allowed Sites dialog [1][2]
  - Hide Last Used & Last Changed initially until a user turns it on. 

Thanks

[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/permissions.xul#16
[2] https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/permissions.dtd#6

Comment 12

[Matthew N. [:MattN]]

I don't think it's necessary to hide Last Used & Last Changed if we make the initial dialog larger. I think Last Changed is useful for security to know if you should rotate your password. I also think the dialog will look bare with just the site name and username by default. Can we simply make it a bit wider and not change the columns? It seems like that should be sufficient.

Comment 13

[Fischer [:Fischer]]

Comment on attachment 8792821 [details]
Bug 1130447 - Hide the password manager timeLastUsed column by default.

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

Comment 14

[Fischer [:Fischer]]

Comment on attachment 8792821 [details]
Bug 1130447 - Hide the password manager timeLastUsed column by default.

Show Last Changed by default so as to remind user time to change per the offline discussion with :MattN and :Tina_Hsieh, thanks.

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8792821 [details]
Bug 1130447 - Hide the password manager timeLastUsed column by default.

https://reviewboard.mozilla.org/r/79718/#review79358

I think there may be some tests that check which columns are shown. You can double-check with: ./mach mochitest toolkit/components/passwordmgr/test/browser/

Thanks

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8792821 [details]
Bug 1130447 - Hide the password manager timeLastUsed column by default.

https://reviewboard.mozilla.org/r/79718/#review79360

::: toolkit/components/passwordmgr/content/passwordManager.xul:1
(Diff revision 2)
>  <?xml version="1.0"?> <!-- -*- Mode: SGML; indent-tabs-mode: nil -*- -->

Can you also update the commit message to describe what is actually changing? e.g. 
Bug 1130447 - Hide the password manager timeLastUsed column by default. r=MattN

Comment 17

[Fischer [:Fischer]]

(In reply to Matthew N. [:MattN] (In Taipei until Sep. 23) from comment #15)
> Comment on attachment 8792821 [details]
> Bug 1130447 - in-content preferences dialog boxes are too small
> 
> https://reviewboard.mozilla.org/r/79718/#review79358
> 
> I think there may be some tests that check which columns are shown. You can
> double-check with: ./mach mochitest
> toolkit/components/passwordmgr/test/browser/

Thanks for reminding.

Had run "./mach mochitest toolkit/components/passwordmgr/test/browser/" locally and did not see failure.

Did not see failure about passwordmgr on Try either.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2a1fd7ea6c2&selectedJob=28037652

Comment 18

[Fischer [:Fischer]]

Comment on attachment 8792821 [details]
Bug 1130447 - Hide the password manager timeLastUsed column by default.

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please resolve the pending issues in MozReview so that this can autoland.

Comment 20

[Fischer [:Fischer]]

Fixed, thanks

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae0e76aa85b6
Hide the password manager timeLastUsed column by default. r=MattN

Comment 22

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/b67dc49095dc because browser_passwordmgr_fields.js explicitly tests that the lastused column is displayed, https://treeherder.mozilla.org/logviewer.html#?job_id=4197984&repo=autoland

Comment 23

[Fischer [:Fischer]]

(In reply to Phil Ringnalda (:philor) from comment #22)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/b67dc49095dc
> because browser_passwordmgr_fields.js explicitly tests that the lastused
> column is displayed,
> https://treeherder.mozilla.org/logviewer.html#?job_id=4197984&repo=autoland
Thanks for pointing out this.
Find that in the local test, the failure message of this didn't appear in the end of test logs but appears in the middle of logs. As a result, unable to spot it in the first place. As for the try, need to include browser-chrome test to test it. Will update the test case.

Comment 24

[Fischer [:Fischer]]

Hi Ryan,
I have updated the test case.
But it seems that the old patch is marked as landed so the Review board doesn't allow the new update push.
How to un-mark that and get the new update pushed?
Thanks.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Not sure, try asking in #mozreview.

Comment 26

[Fischer [:Fischer]]

Comment on attachment 8792821 [details]
Bug 1130447 - Hide the password manager timeLastUsed column by default.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79718/diff/3-4/

Comment 27

[Fischer [:Fischer]]

Attachment: passwordmgr_test_browser_local_20161003.log

Updated toolkit/components/passwordmgr/test/browser/browser_passwordmgr_fields.js
Submit the passed local test and the passed Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3a56c4ccdf0

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f68358e0ed5
Hide the password manager timeLastUsed column by default. r=MattN

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f68358e0ed5