Hide the password manager timeLastUsed column by default

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: aleth, Assigned: Fischer)

Tracking

37 Branch
Firefox 52
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1247404
(Assignee)

Updated

2 years ago
Depends on: 1243002
(Assignee)

Comment 1

2 years ago
@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.
(Assignee)

Comment 5

2 years ago
@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)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1247404
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1243002
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

2 years ago
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)

Updated

2 years ago
Assignee: nobody → fliu
Summary: in-content preferences dialog boxes are too small → Hide the password manager timeLastUsed column by default
(Assignee)

Comment 17

2 years ago
(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
Comment hidden (mozreview-request)
Please resolve the pending issues in MozReview so that this can autoland.
Flags: needinfo?(fliu)
Keywords: checkin-needed
(Assignee)

Comment 20

2 years ago
Fixed, thanks
Flags: needinfo?(fliu) → needinfo?(ryanvm)
Flags: needinfo?(ryanvm)

Comment 21

2 years ago
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
(Assignee)

Comment 23

2 years ago
(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.
(Assignee)

Comment 24

2 years ago
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

2 years ago
Created attachment 8797882 [details]
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
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 28

2 years ago
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

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9f68358e0ed5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.