MD Settings: Fix internet focus and styling

MD Settings: Fix internet focus and styling

BUG=668341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/72d4e57305352cfe734daa5390c1c23c91949830
Cr-Commit-Position: refs/heads/master@{#438390}

[stevenjb, 2016-12-08 23:31:54]

Description was changed from

==========
MD Settings: Fix internet focus and styling

BUG=668341
==========

to

==========
MD Settings: Fix internet focus and styling

BUG=668341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2016-12-08 23:53:32]

stevenjb@chromium.org changed reviewers:
+ dbeam@chromium.org, dschuyler@chromium.org

[stevenjb, 2016-12-08 23:53:33]

dbeam@ FYI. I had to move the .subpage-arrow styling to shared_style.css in
order to use it in the shared element. I went ahead and moved .icon-external as
well, figuring that both might be generally useful in webui (and wanting to keep
them together).

[stevenjb, 2016-12-08 23:58:33]

On 2016/12/08 23:53:33, stevenjb wrote:
> dbeam@ FYI. I had to move the .subpage-arrow styling to shared_style.css in
> order to use it in the shared element. I went ahead and moved .icon-external
as
> well, figuring that both might be generally useful in webui (and wanting to
keep
> them together).

Screenshot:

https://screenshot.googleplex.com/Tcy92G54WUO.png

[Dan Beam, 2016-12-09 00:11:38]

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_page.html (right):

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_page.html:50: <div
class="start add-no-icon" actionable>
how is this actionable?  what does it do?  this looks like a mistake, maybe? 
there's already an [actionable] on this parent

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_page.html:57: <div
class="start add-no-icon" actionable>
same

https://codereview.chromium.org/2556253007/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/shared_style_css.html (right):

https://codereview.chromium.org/2556253007/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/shared_style_css.html:50: }
sorry, but you're propagating this.

this implementation isn't great.

it leaves around buttons that have ripples but no backgrounds.

it's annoying in cases where ONLY the button should be actionable, but you have
to wrap with an extra node or add actionable on an ancestor.

additionally, not all things use the [actionable] attribute, but instead might
@apply(--cr-actionable); or @apply(--settings-actionable); directly.

can we drop the [actionable] part of the selector and figure out a better way to
do this?

[stevenjb, 2016-12-09 00:57:52]

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_page.html (right):

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_page.html:50: <div
class="start add-no-icon" actionable>
On 2016/12/09 00:11:38, Dan Beam wrote:
> how is this actionable?  what does it do?  this looks like a mistake, maybe? 
> there's already an [actionable] on this parent

You're right, it was leftover from an intermediate change, thanks. Removed.

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_page.html:57: <div
class="start add-no-icon" actionable>
On 2016/12/09 00:11:38, Dan Beam wrote:
> same

Done.

https://codereview.chromium.org/2556253007/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/shared_style_css.html (right):

https://codereview.chromium.org/2556253007/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/shared_style_css.html:50: }
On 2016/12/09 00:11:38, Dan Beam wrote:
> sorry, but you're propagating this.
> 
> this implementation isn't great.
> 
> it leaves around buttons that have ripples but no backgrounds.
> 
> it's annoying in cases where ONLY the button should be actionable, but you
have
> to wrap with an extra node or add actionable on an ancestor.
> 
> additionally, not all things use the [actionable] attribute, but instead might
> @apply(--cr-actionable); or @apply(--settings-actionable); directly.
> 
> can we drop the [actionable] part of the selector and figure out a better way
to
> do this?

The [actionable] seems unnecessary/unwanted. Dave, is there a reason it was
included in the first place?

[dschuyler, 2016-12-09 01:15:52]

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_page.html (right):

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_page.html:53: <button
class="icon-external" is="paper-icon-button-light"></button>
Should there be a:
<link rel="import"
href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button-light.html">
at the top of this file to support this?

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary_item.html
(right):

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:57:
<div class="settings-box two-line" actionable on-tap="onDetailsTap_">
optional:
I don't think there's a style guide on it, but I prefer that the attributes are
in alphabetical order.

https://codereview.chromium.org/2556253007/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/shared_style_css.html (right):

https://codereview.chromium.org/2556253007/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/shared_style_css.html:50: }
On 2016/12/09 00:57:52, stevenjb wrote:
> On 2016/12/09 00:11:38, Dan Beam wrote:
> > sorry, but you're propagating this.
> > 
> > this implementation isn't great.
> > 
> > it leaves around buttons that have ripples but no backgrounds.
> > 
> > it's annoying in cases where ONLY the button should be actionable, but you
> have
> > to wrap with an extra node or add actionable on an ancestor.
> > 
> > additionally, not all things use the [actionable] attribute, but instead
might
> > @apply(--cr-actionable); or @apply(--settings-actionable); directly.
> > 
> > can we drop the [actionable] part of the selector and figure out a better
way
> to
> > do this?
> 
> The [actionable] seems unnecessary/unwanted. Dave, is there a reason it was
> included in the first place?
> 

The reason it was initially done is that the issues mentioned above where not
known to me/us and it seemed to be correct. Other than the @apply() vs
[actionable] was known as something that could need and extra attribute, but
wasn't an issue as used.

The bug at 655292 may also be considered - it asks that those buttons not have
ripples at all.

[stevenjb, 2016-12-09 01:29:30]

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_page.html (right):

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_page.html:53: <button
class="icon-external" is="paper-icon-button-light"></button>
On 2016/12/09 01:15:52, dschuyler wrote:
> Should there be a:
> <link rel="import"
>
href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button-light.html">
> at the top of this file to support this?

Yes. Done.

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary_item.html
(right):

https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary_item.html:57:
<div class="settings-box two-line" actionable on-tap="onDetailsTap_">
On 2016/12/09 01:15:52, dschuyler wrote:
> optional:
> I don't think there's a style guide on it, but I prefer that the attributes
are
> in alphabetical order.

Yeah, there's not. I tend to prefer: id class attributes-in-alpha-order events.

I can change if there is a different consensus.

https://codereview.chromium.org/2556253007/diff/20001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/shared_style_css.html (right):

https://codereview.chromium.org/2556253007/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/shared_style_css.html:50: }
On 2016/12/09 01:15:52, dschuyler wrote:
> On 2016/12/09 00:57:52, stevenjb wrote:
> > On 2016/12/09 00:11:38, Dan Beam wrote:
> > > sorry, but you're propagating this.
> > > 
> > > this implementation isn't great.
> > > 
> > > it leaves around buttons that have ripples but no backgrounds.
> > > 
> > > it's annoying in cases where ONLY the button should be actionable, but you
> > have
> > > to wrap with an extra node or add actionable on an ancestor.
> > > 
> > > additionally, not all things use the [actionable] attribute, but instead
> might
> > > @apply(--cr-actionable); or @apply(--settings-actionable); directly.
> > > 
> > > can we drop the [actionable] part of the selector and figure out a better
> way
> > to
> > > do this?
> > 
> > The [actionable] seems unnecessary/unwanted. Dave, is there a reason it was
> > included in the first place?
> > 
> 
> The reason it was initially done is that the issues mentioned above where not
> known to me/us and it seemed to be correct. Other than the @apply() vs
> [actionable] was known as something that could need and extra attribute, but
> wasn't an issue as used.
> 
> The bug at 655292 may also be considered - it asks that those buttons not have
> ripples at all.

I think we should address 655292 separately, and eliminate the [actionable] part
of these selectors, since I can't imagine we would ever not want e.g.
button[is='paper-icon-button-light'].subpage-arrow not to have the specified
background. Sound good?

[Dan Beam, 2016-12-09 01:30:56]

On 2016/12/09 01:15:52, dschuyler wrote:
>
https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/internet_page/internet_page.html
(right):
> 
>
https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/internet_page/internet_page.html:53: <button
> class="icon-external" is="paper-icon-button-light"></button>
> Should there be a:
> <link rel="import"
>
href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button-light.html">
> at the top of this file to support this?
> 
>
https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/internet_page/network_summary_item.html
> (right):
> 
>
https://codereview.chromium.org/2556253007/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/internet_page/network_summary_item.html:57:
> <div class="settings-box two-line" actionable on-tap="onDetailsTap_">
> optional:
> I don't think there's a style guide on it, but I prefer that the attributes
are
> in alphabetical order.
> 
>
https://codereview.chromium.org/2556253007/diff/20001/ui/webui/resources/cr_e...
> File ui/webui/resources/cr_elements/shared_style_css.html (right):
> 
>
https://codereview.chromium.org/2556253007/diff/20001/ui/webui/resources/cr_e...
> ui/webui/resources/cr_elements/shared_style_css.html:50: }
> On 2016/12/09 00:57:52, stevenjb wrote:
> > On 2016/12/09 00:11:38, Dan Beam wrote:
> > > sorry, but you're propagating this.
> > > 
> > > this implementation isn't great.
> > > 
> > > it leaves around buttons that have ripples but no backgrounds.
> > > 
> > > it's annoying in cases where ONLY the button should be actionable, but you
> > have
> > > to wrap with an extra node or add actionable on an ancestor.
> > > 
> > > additionally, not all things use the [actionable] attribute, but instead
> might
> > > @apply(--cr-actionable); or @apply(--settings-actionable); directly.
> > > 
> > > can we drop the [actionable] part of the selector and figure out a better
> way
> > to
> > > do this?
> > 
> > The [actionable] seems unnecessary/unwanted. Dave, is there a reason it was
> > included in the first place?
> > 
> 
> The reason it was initially done is that the issues mentioned above where not
> known to me/us and it seemed to be correct. Other than the @apply() vs
> [actionable] was known as something that could need and extra attribute, but
> wasn't an issue as used.

The best way to enforce this isn't bypassed would be to remove the
--cr-actionable mixin and ONLY apply these rules to [actionable].

> 
> The bug at 655292 may also be considered - it asks that those buttons not have
> ripples at all.

yeah, i'm still not convinced on that front

[dschuyler, 2016-12-10 01:09:18]

https://codereview.chromium.org/2556253007/diff/60001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/shared_style_css.html (right):

https://codereview.chromium.org/2556253007/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/shared_style_css.html:15: }
Without [actionable], I believe the arrows will appear enabled when they are
un-clickable. Such as when prefs.profile.password_manager_enmabled.value is
false. There are a few places that may be impacted by this change. I think they
can be found by searching in the settings folder for actionable$=" (i.e. A
binding that conditionally sets actionable).

Maybe those could be changed to a hidden$=" on the button itself?
(I'd also welcome a different disabled behavior for rows that are un-clickable,
though I'd recommend a separate CL for that bit).

Those could be changed in this CL or as a prequel-CL to this CL.

[dschuyler, 2016-12-10 01:14:01]

https://codereview.chromium.org/2556253007/diff/60001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/shared_style_css.html (right):

https://codereview.chromium.org/2556253007/diff/60001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/shared_style_css.html:15: }
On 2016/12/10 01:09:18, dschuyler wrote:
> Without [actionable], I believe the arrows will appear enabled when they are
> un-clickable. Such as when prefs.profile.password_manager_enmabled.value is
> false. There are a few places that may be impacted by this change. I think
they
> can be found by searching in the settings folder for actionable$=" (i.e. A
> binding that conditionally sets actionable).
> 
> Maybe those could be changed to a hidden$=" on the button itself?
> (I'd also welcome a different disabled behavior for rows that are
un-clickable,
> though I'd recommend a separate CL for that bit).
> 
> Those could be changed in this CL or as a prequel-CL to this CL.
> 

In case I'm giving the wrong impression when I said, "changed to a hidden". We'd
want to do the conditional hidden$= in addition to the actionable$= (i.e. not
change/replace that with hidden); so that the mouse pointer does the correct
thing as well as hiding/showing the icon.

[stevenjb, 2016-12-12 17:54:36]

On 2016/12/10 01:14:01, dschuyler wrote:
>
https://codereview.chromium.org/2556253007/diff/60001/ui/webui/resources/cr_e...
> File ui/webui/resources/cr_elements/shared_style_css.html (right):
> 
>
https://codereview.chromium.org/2556253007/diff/60001/ui/webui/resources/cr_e...
> ui/webui/resources/cr_elements/shared_style_css.html:15: }
> On 2016/12/10 01:09:18, dschuyler wrote:
> > Without [actionable], I believe the arrows will appear enabled when they are
> > un-clickable. Such as when prefs.profile.password_manager_enmabled.value is
> > false. There are a few places that may be impacted by this change. I think
> they
> > can be found by searching in the settings folder for actionable$=" (i.e. A
> > binding that conditionally sets actionable).
> > 
> > Maybe those could be changed to a hidden$=" on the button itself?
> > (I'd also welcome a different disabled behavior for rows that are
> un-clickable,
> > though I'd recommend a separate CL for that bit).
> > 
> > Those could be changed in this CL or as a prequel-CL to this CL.
> > 
> 
> In case I'm giving the wrong impression when I said, "changed to a hidden".
We'd
> want to do the conditional hidden$= in addition to the actionable$= (i.e. not
> change/replace that with hidden); so that the mouse pointer does the correct
> thing as well as hiding/showing the icon.

At this point I am confused as to what the problem we are trying to fix here is.

Dan, since you brought it up, would you mind filing a separate issue and
assigning it to me or Dave to address separately? I'm fine with fixing it before
moving the CSS, but we're conflating issues at this point and in the meanwhile
the changes I am attempting to make here are not getting reviewed :(

[stevenjb, 2016-12-13 22:34:02]

Patchset #5 (id:80001) has been deleted

[stevenjb, 2016-12-13 22:36:15]

Rebased with https://codereview.chromium.org/2570783002/

PTAL

[Dan Beam, 2016-12-13 22:57:20]

lgtm

[dschuyler, 2016-12-13 23:00:05]

lgtm

[stevenjb, 2016-12-13 23:14:46]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-12-13 23:15:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 00:42:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 00:42:47]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2016-12-14 01:14:35]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-12-14 01:14:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2556253007/#ps120001
(title: "Restore cr:settings")

[commit-bot: I haz the power, 2016-12-14 01:15:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 02:19:24]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481678075646250,
"parent_rev": "713a0ea0151f7c1d3d65413399b066198cfb0241", "commit_rev":
"c45e40847983d35b39f772d28abb53395852e33a"}

[commit-bot: I haz the power, 2016-12-14 02:20:11]

Description was changed from

==========
MD Settings: Fix internet focus and styling

BUG=668341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix internet focus and styling

BUG=668341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2556253007
==========

[commit-bot: I haz the power, 2016-12-14 02:20:12]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-12-14 02:21:53]

Description was changed from

==========
MD Settings: Fix internet focus and styling

BUG=668341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2556253007
==========

to

==========
MD Settings: Fix internet focus and styling

BUG=668341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/72d4e57305352cfe734daa5390c1c23c91949830
Cr-Commit-Position: refs/heads/master@{#438390}
==========

[commit-bot: I haz the power, 2016-12-14 02:21:54]

Patchset 6 (id:??) landed as
https://crrev.com/72d4e57305352cfe734daa5390c1c23c91949830
Cr-Commit-Position: refs/heads/master@{#438390}