MD Settings: text control cleanup (links, buttons) |
||||||||||||||
Issue descriptionWe have several buttons that could be called "action links". In MD Settings, an action link should: * be styled like a link (blue, no underline) * participate in tab order * when focused, have a focus rectangle tightly wrapping the link text * use the "pointer" cursor * NOT change on press (aside from focusing): no ripple or text decoration * NOT be tappable when disabled * grey out when disabled We currently have at least two ways of doing this, neither is perfect: 1. A <div class="list-item list-button"> when inside a list. Problems: * Doesn't participate in tab order * Focus state would be an ugly rectangle around the entire row * No disabled state; still responds to taps when disabled 2. An <a is="action-link">. Problems: * Not greyed out when disabled * Still tappable when disabled (keyboard is handled correctly, but not mouse) Some pages (the Autofill Settings page at autofill_section.html, for one) use a deprecated <link rel="import" type="css" href="chrome://resources/css/action_link.css"> to pull action-link styling into their local DOM. This provides additional styling, some of which is correct (greyed out when disabled), some of which is incorrect (underlined when pressed). My suggestion would be to add the [disabled] rule to the a[is=action-link] styles in settings_shared_css.html and figure out a way to make action links not tappable, then convert the <div>s to <a is="action-link">s.
,
Nov 29 2016
why can't we just do
[is='action-link'][disabled] {
color: #999;
pointer-events: none;
}
and skip the list-item?
,
Nov 29 2016
,
Dec 6 2016
#2: that's basically what I proposed (although I hadn't thought of pointer-events: none at the time).
,
Dec 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/10719a79d0d6cf3cee559771cb5cca8f78024b5d commit 10719a79d0d6cf3cee559771cb5cca8f78024b5d Author: michaelpg <michaelpg@chromium.org> Date: Wed Dec 07 09:05:21 2016 Language settings: fix focus issues * Allow traversal through iron-list elements with up/down arrow keys. For input methods, <Enter> selects that input method. * Tapping anywhere on a togglable row toggles that row. * Make the buttons to open a subpage keyboard-focusable by wrapping their content in <a is="action-link">. * Don't focus the Add Language dialog's Cancel button on opening. * Other minor UI fixes. BUG= 664106 , 668307 , 658279 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2523403003 Cr-Commit-Position: refs/heads/master@{#436894} [modify] https://crrev.com/10719a79d0d6cf3cee559771cb5cca8f78024b5d/chrome/browser/resources/settings/languages_page/add_languages_dialog.html [modify] https://crrev.com/10719a79d0d6cf3cee559771cb5cca8f78024b5d/chrome/browser/resources/settings/languages_page/add_languages_dialog.js [modify] https://crrev.com/10719a79d0d6cf3cee559771cb5cca8f78024b5d/chrome/browser/resources/settings/languages_page/compiled_resources2.gyp [modify] https://crrev.com/10719a79d0d6cf3cee559771cb5cca8f78024b5d/chrome/browser/resources/settings/languages_page/edit_dictionary_page.html [modify] https://crrev.com/10719a79d0d6cf3cee559771cb5cca8f78024b5d/chrome/browser/resources/settings/languages_page/languages_page.html [modify] https://crrev.com/10719a79d0d6cf3cee559771cb5cca8f78024b5d/chrome/browser/resources/settings/languages_page/languages_page.js [modify] https://crrev.com/10719a79d0d6cf3cee559771cb5cca8f78024b5d/ui/webui/resources/cr_elements/compiled_resources2.gyp [add] https://crrev.com/10719a79d0d6cf3cee559771cb5cca8f78024b5d/ui/webui/resources/cr_elements/cr_expand_button/compiled_resources2.gyp
,
Dec 10 2016
,
Dec 10 2016
@michaelpg can I assign this to you since it looks like you're making progress on it?
,
Dec 12 2016
I fixed the language settings. I think there's still more work to do to make all these links cosmetically and technically similar, but it's not something I can spend time on right now. I think to fix the beta blocker (if it's really a blocker), we just need to do an audit and make the links all look correct (e.g., Autofill link shouldn't underline on press).
,
Dec 17 2016
,
Jan 11 2017
,
Jan 11 2017
OK, there's a few things we need to consider here. major themes: 1) does the control point to a URL? if so: probably use <a href> so the user can copy the link / open in new tab or window, etc. (that you get for free with context menus for <a href>) 2) does the control need a ripple on down? because if so it'll need some form of <paper-ripple> (either by using a control from Polymer with a ripple already, or adding one declaratively or imperatively) 3) can I access the control without a mouse? see also: go/gar-web sections 1.1, 1.2, 1.4, 1.13
,
Jan 13 2017
Issue 594831 has been merged into this issue.
,
Jan 13 2017
,
Jan 17 2017
,
Jan 17 2017
Regarding .list-button:
.list-button is intended to be a button within a list of things (e.g. "english", "chinese", "add new language..." <- the button).
Currently there are a few ways .list-item buttons are structured:
a) <div class="list-item list-button" on-tap="[[action]]">
[[text]]
</div>
b) <div class="list-item list-button">
<a is="action-link" on-tap"[[action]]">[[text]]</a>
</div>
c) <a class="list-item list-button" is="action-link"
on-tap="[[action]]">[[text]]</a>
All three of which behave a little bit differently in terms of interaction with mouse and also focus square size/placements. I'm discussing with bettes@ to see which behavior we prefer, and will unify all .list-button implementations to it once confirmed.
Regarding dbeam@'s comment:
--- why can't we just do
---
--- [is='action-link'][disabled] {
--- color: #999;
--- pointer-events: none;
--- }
---
--- and skip the list-item?"
In the case of <div class="list-item list-button"><a is="action-link" /></div>, the whole div containing the action-link would still appear clickable, which is why I think michaelpg@ originally placed the pointer-events:none on the .list-button. But in any case, I'll try to group the css rule to the simplest possible once I find out from bettes@ what we want to do.
,
Jan 17 2017
,
Jan 18 2017
According to bettes@: "The focus ring should reflect the touch target. Is the over-arching principle here. Regardless of it's aesthetic, I want the whole row to be highlighted in the blue text case". Will implement based on that.
,
Jan 21 2017
After consolidating all the usage of .list-button, I came to:
<div class="list-item">
<a is="action-link" class="list-button" on-tap="[[handler]]">
text
</a>
</div>
Also added a disabled style that would apply to [is="action-link"], which should be sufficient to block tab/clicks for action-links and list-buttons (note: there's no .list-button that is not also an action-link).
,
Jan 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b commit a41c7aae93287688ee2e3b345b46d70b1e5f8e4b Author: scottchen <scottchen@chromium.org> Date: Thu Jan 26 05:40:57 2017 MD Settings: clean up text controls (links, buttons) Making styles and behaviors consistent for <a href>, .action-link, .list-button, and other varieties of text-based action items. BUG= 668307 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation NOTRY=true # blame dbeam@ Review-Url: https://codereview.chromium.org/2639373002 Cr-Commit-Position: refs/heads/master@{#446236} [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/md_downloads/item.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/device_page/stylus.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/internet_page/network_summary_item.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/languages_page/languages_page.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/on_startup_page/startup_urls_page.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/people_page/lock_screen.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/people_page/users_page.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/printing_page/cups_printers.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/privacy_page/privacy_page.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/reset_page/reset_profile_dialog.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/search_engines_page/search_engines_page.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/settings_shared_css.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/browser/resources/settings/site_settings/site_details.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/test/data/webui/settings/quick_unlock_authenticate_browsertest_chromeos.js [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/test/data/webui/settings/search_engines_page_test.js [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/chrome/test/data/webui/settings/startup_urls_page_test.js [add] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/ui/webui/resources/html/action_link_css.html [modify] https://crrev.com/a41c7aae93287688ee2e3b345b46d70b1e5f8e4b/ui/webui/resources/webui_resources.grd
,
Jan 26 2017
,
May 8 2017
,
Jun 2 2017
Issue 729141 has been merged into this issue. |
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by michae...@chromium.org
, Nov 23 2016This seems to work, for list item action buttons: <link rel="import" href="chrome://resources/html/action_link.html"> <!-- .... begin dom-module template --> <div class="list-item list-button" on-tap="onButtonTap" disabled="[[disabled]]"> <a is="action-link" disabled="[[disabled]]">Tap me</a> </div> Assuming we add these rules to settings_shared_css.html: .list-button[disabled] { pointer-events: none; } [is='action-link'][disabled] { color: #999; } Thoughts?