Closed Bug 1130447 Opened 8 years ago Closed 6 years ago

Hide the password manager timeLastUsed column by default

Categories

(Firefox :: Preferences, defect)

37 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: aleth, Assigned: Fischer)

References

Details

Attachments

(2 files)

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.
Depends on: 1247404
Depends on: 1243002
@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.
Flags: needinfo?(jaws)
I don't know of any, but maybe Ryan Feeley knows of some best practices here?
Flags: needinfo?(jaws) → needinfo?(rfeeley)
(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.
(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.
@Tina & Jack,
Please check the size of dialog window inside Preference page.
Let us know if the size is fine or needs adjustment.
Thanks
Flags: needinfo?(thsieh)
Flags: needinfo?(jalin)
I have no idea how these sizes are decided, and neither does shorlander. I'm fine with an increase in size FWIW.
Flags: needinfo?(rfeeley)
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 :)
Flags: needinfo?(thsieh)
Flags: needinfo?(jalin)
Duplicate of this bug: 1247404
Duplicate of this bug: 1243002
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
Attachment #8792821 - Flags: review?(MattN+bmo)
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.
Flags: needinfo?(thsieh)
Attachment #8792821 - Flags: review?(MattN+bmo)
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.
Flags: needinfo?(thsieh)
Attachment #8792821 - Flags: review?(MattN+bmo)
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
Attachment #8792821 - Flags: review?(MattN+bmo) → review+
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
Assignee: nobody → fliu
Summary: in-content preferences dialog boxes are too small → Hide the password manager timeLastUsed column by default
(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
Keywords: checkin-needed
Please resolve the pending issues in MozReview so that this can autoland.
Flags: needinfo?(fliu)
Keywords: checkin-needed
Fixed, thanks
Flags: needinfo?(fliu) → needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ae0e76aa85b6
Hide the password manager timeLastUsed column by default. r=MattN
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
(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.
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.
Flags: needinfo?(ryanvm)
Not sure, try asking in #mozreview.
Flags: needinfo?(ryanvm)
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
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9f68358e0ed5
Hide the password manager timeLastUsed column by default. r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9f68358e0ed5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.