Closed
Bug 1130447
Opened 10 years ago
Closed 8 years ago
Hide the password manager timeLastUsed column by default
Categories
(Firefox :: Settings UI, defect)
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.
Assignee | ||
Comment 1•9 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)
Comment 2•9 years ago
|
||
I don't know of any, but maybe Ryan Feeley knows of some best practices here?
Flags: needinfo?(jaws) → needinfo?(rfeeley)
Comment 3•9 years ago
|
||
(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•9 years ago
|
||
(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•9 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)
Comment 6•9 years ago
|
||
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)
Comment 7•9 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•8 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)
Comment 12•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8792821 -
Flags: review?(MattN+bmo)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•8 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 15•8 years ago
|
||
mozreview-review |
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 16•8 years ago
|
||
mozreview-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•8 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•8 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) |
Comment 19•8 years ago
|
||
Please resolve the pending issues in MozReview so that this can autoland.
Flags: needinfo?(fliu)
Keywords: checkin-needed
Updated•8 years ago
|
Flags: needinfo?(ryanvm)
Comment 21•8 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
Comment 22•8 years ago
|
||
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•8 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•8 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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•8 years ago
|
||
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•8 years ago
|
Keywords: checkin-needed
Comment 28•8 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 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.
Description
•