Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
840503002:
Remove gray background for non-focused selected list items on autofillEditAddress page. (Closed)
DescriptionRemove gray background color for non-focused selected list items on chrome://settings/autofillEditAddress page.
BUG=444770
Committed: https://crrev.com/0e73bb9ccdf505a462716bf7611308068a71accc
Cr-Commit-Position: refs/heads/master@{#312927}
Patch Set 1 #Patch Set 2 : Remove gray background. #
Total comments: 8
Patch Set 3 : CSS white -> transparent. #Messages
Total messages: 15 (4 generated)
bondd@chromium.org changed reviewers: + jhawkins@chromium.org
bondd@chromium.org changed required reviewers: + jhawkins@chromium.org
What is the purpose of the grey background? Can you attach a screenshot?
On 2015/01/08 02:03:22, James Hawkins wrote: > What is the purpose of the grey background? Can you attach a screenshot? Added screenshots and purpose of the gray background to the bug: https://code.google.com/p/chromium/issues/detail?id=444770#c5
As discussed on Friday I've removed the gray background completely from the autofillEditAddress page. No other pages are affected. I agree that your proposed solution on the bug report is ideal, but I'd like to commit this as an interim solution (as discussed on Friday). I think we agree that this solution is better than keeping the gray background. If/when this CL is approved I'll update the bug report with a description of what's been done. https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/autofill_edit_overlay.css (right): https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/autofill_edit_overlay.css:60: } Row delete button was also visible when gray background was showing. Hide it using the same settings as here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
LGTM with nit. https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/autofill_edit_overlay.css (right): https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/autofill_edit_overlay.css:60: } On 2015/01/14 03:18:48, bondd wrote: > Row delete button was also visible when gray background was showing. Hide it > using the same settings as here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... Please add a TODO to clean up the hack.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/autofill_edit_overlay.css (right): https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/autofill_edit_overlay.css:53: background-color: white; don't use white, use `none` or `transparent` https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/autofill_edit_overlay.css:53: background-color: white; i'm confused, are we overriding this rule? https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... i'd expect to see a more specific selector to override: #autofill-edit-address-overlay list > [lead][selected], #autofill-edit-address-overlay list:focus > [lead][selected] { background-color: /* lighter */; } but maybe for all dialogs (instead of just this one). https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/autofill_edit_overlay.css:60: } On 2015/01/14 17:34:34, James Hawkins wrote: > On 2015/01/14 03:18:48, bondd wrote: > > Row delete button was also visible when gray background was showing. Hide it > > using the same settings as here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > Please add a TODO to clean up the hack. how is this a hack?
https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/autofill_edit_overlay.css (right): https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/autofill_edit_overlay.css:53: background-color: white; On 2015/01/22 22:38:55, Dan Beam wrote: > don't use white, use `none` or `transparent` Done. https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/autofill_edit_overlay.css:53: background-color: white; On 2015/01/22 22:38:55, Dan Beam wrote: > i'm confused, are we overriding this rule? > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > > i'd expect to see a more specific selector to override: > > #autofill-edit-address-overlay list > [lead][selected], > #autofill-edit-address-overlay list:focus > [lead][selected] { > background-color: /* lighter */; > } > > but maybe for all dialogs (instead of just this one). We are overriding this rule: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... not(:hover) added so that item will still have gray background when hovering. Originally you and I were planning to only lighten the gray, but jhawkins@ convinced me that this specific dialog (not all list dialogs) would be less confusing for sighted users if the highlight were removed completely. See discussion on bug https://crbug.com/444770 That is why it's only done for this one dialog instead of all of them. If we were to lighten the gray instead, then I think we should do it for all dialogs. https://codereview.chromium.org/840503002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/autofill_edit_overlay.css:60: } On 2015/01/22 22:38:55, Dan Beam wrote: > On 2015/01/14 17:34:34, James Hawkins wrote: > > On 2015/01/14 03:18:48, bondd wrote: > > > Row delete button was also visible when gray background was showing. Hide it > > > using the same settings as here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > > > Please add a TODO to clean up the hack. > > how is this a hack? It's a hack if you agree that the old list behavior is the "right" way, and having the new list behavior in this dialog is a bug that should be fixed eventually. See jhawkins@ comments on https://crbug.com/444770 If you consider the new way to be just as right as the old way (due to considerations for webui consistency, better for AT users, etc.) then it's not a hack.
lgtm
The CQ bit was checked by bondd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/840503002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0e73bb9ccdf505a462716bf7611308068a71accc Cr-Commit-Position: refs/heads/master@{#312927} |
