MD Settings: Fix timezone autodetect policy indicator.

chrome/test/data/webui/settings/date_time_page_tests.js

Index: chrome/test/data/webui/settings/date_time_page_tests.js
diff --git a/chrome/test/data/webui/settings/date_time_page_tests.js b/chrome/test/data/webui/settings/date_time_page_tests.js
index 7cf96a7794abc390bd77fd2c29aab763c8481650..118a37624f9ddb3c4e11b6b03538585cb77e8bca 100644
--- a/chrome/test/data/webui/settings/date_time_page_tests.js
+++ b/chrome/test/data/webui/settings/date_time_page_tests.js
@@ -121,11 +121,13 @@
 
       if (policy) {
         assertTrue(!!indicator);
-        assertGT(indicator.clientHeight, 0);

[michaelpg, 2017/02/08 00:07:12]

would indicator.$$('iron-icon').clientHeight be sufficient?

I thought checking for clientHeight was a more robust way to detect whether or
not an element is hidden, because there are a bunch of ways to do so (display:
none, hidden = true, etc.)

[dpapad, 2017/02/08 01:53:50]

Actually it is not at all robust. Here is why:

The [hidden] attribute works in custom Polymer elements, only because some
transitive dependency is leaking the following CSS rule 
html /deep/ [hidden] { display: none !important;}
(one of those two
https://cs.chromium.org/search/?q=%22html+/deep/+%5Bhidden%5D%22+file:%5Esrc/...).

You can verify this from the devtools, see http://imgur.com/a/HnzDM, as well as
from the discussion at
https://github.com/Polymer/polymer/issues/3711#issuecomment-226172158.

Our test is loading a specific URL that does not depend on those elements, which
means that applying the hidden property does really nothing in terms of hiding
the iron-icon, and its clientHeight is still 20px (you can also verify this by
applying this CL locally, reverting the one line fix in date_time_page.js and
use console.log here).

[michaelpg, 2017/02/08 02:24:25]

Jeez. Can't wait to get rid of those iron-flex global hidden attributes.

Can you add a TODO to replace these checks with clientHeight-type checks,
blocked on 635633 and/or 498405?

Thanks.

[dpapad, 2017/02/08 22:03:40]

 > Can you add a TODO to replace these checks with clientHeight-type checks,
blocked on 635633 and/or 498405?
Michael do you still this TODO is worth it? The code now uses a dedicated
cr-policy-pref-indicator#isActive() method, so I don't think we need to revert
to checking clientHeight in the future. (We should add unit tests for
cr-pref-policy-indicator though.)

[michaelpg, 2017/02/09 22:09:02]

SGTM.

+        assertNotEquals('none', indicator.style.display);

[michaelpg, 2017/02/08 00:07:12]

generally we should check getComputedStyle, because e.g.

    <cr-policy-pref-indicator hidden>

would result in a computed display of none, but the element's "display" style
property would still be empty.

[dpapad, 2017/02/08 01:53:50]

Tried getComputedStyle() and it throws 

TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not
of type 'Element'.
    at value
(chrome://resources/polymer/v1_0/web-animations-js/web-animations-next-lite.min.js:16:12769)
    at verifyPolicy (date_time_page_tests.js:134:32)
    at Context.<anonymous> (date_time_page_tests.js:157:7)
", source: mocha_adapter.js (48)

which is very odd, since |indicator| is a Polymer element. I tried the following
to see
console.log(indicator instanceof Element); // false
console.log(indicator instanceof HTMLElement); // false
console.log(indicator.appendChild) // function appendChild() { [native code] }

I don't know why this is happening, but I'd rather not block this CL on it.

[michaelpg, 2017/02/08 02:24:25]

Yeah I'm at a loss there.

+        assertTrue(!!indicator.$$('iron-icon'));

[Dan Beam, 2017/02/08 02:14:23]

why are we looking into indicator itself?

[dpapad, 2017/02/08 02:26:35]

In lack of a better way to verify if the on/off state of the policy pref had any
effect on the DOM. cr-policy-pref-indicator does not expose that info (for
example via an attribute) AND does not have unit tests.

Summarizing offline discussions we have 3 options
1) Update cr-policy-pref-indicator to expose and |active| HTML attribute. Assert
on that. Add unit tests for cr-policy-pref-indicator.
2) Migrate to settings-toggle (if possible), which already exposes a |checked|
attribute (and has unit tests). Assert on the |checked| attribute here, avoiding
querying any private state.
3) Let this CL through, since it fixes a bug, improves broken tests, and allows
me to stay focused on the vulcanized stuff, instead of going of course.

[dpapad, 2017/02/08 19:18:34]

@dbeam PTAL: I added an isActive() method on cr-policy-pref-indicator. I also
added a "restamp" on the surrounding dom-if so that we don't need to check for
display or the presence of the element. This simplifies things a lot, and
addresses your concerns of querying private state during tests (no need to deal
with iron-icons anymore).

FWIW, I also tried exposing an |active| HTML attribute, instead of the
isActive() method, but the former was problematic for multiple reasons.
 - The attribute is unnecessary calculated even if nobody is querying for it.
 - The attribute is editable from the outside, but the indicator state should
only be dictated by the pref.
For those reasons the isActive() method seems like the best approach.

+        assertFalse(indicator.$$('iron-icon').hidden);
       } else {
         // Indicator should be missing or hidden.
         if (indicator)
-          assertEquals(0, indicator.clientHeight);
+          assertEquals('none', indicator.style.display);
       }
 
       assertEquals(policy, dateTime.$.timeZoneAutoDetect.disabled);
@@ -165,6 +167,7 @@
       var prefs = getFakePrefs();
       prefs.settings.resolve_timezone_by_geolocation.value = false;
       dateTime = initializeDateTime(prefs, false);
+      cr.webUIListenerCallback('time-zone-auto-detect-policy', false);
 
       assertTrue(dateTimePageReadyCalled);
       assertTrue(getTimeZonesCalled);
@@ -186,6 +189,7 @@
       var prefs = getFakePrefs();
       prefs.settings.resolve_timezone_by_geolocation.value = false;
       dateTime = initializeDateTime(prefs, true, true);
+      cr.webUIListenerCallback('time-zone-auto-detect-policy', true, true);
 
       assertTrue(dateTimePageReadyCalled);
       assertFalse(getTimeZonesCalled);
@@ -214,6 +218,7 @@
     test('auto-detect forced off', function(done) {
       var prefs = getFakePrefs();
       dateTime = initializeDateTime(prefs, true, false);
+      cr.webUIListenerCallback('time-zone-auto-detect-policy', true, false);
 
       assertTrue(dateTimePageReadyCalled);
       assertTrue(getTimeZonesCalled);
@@ -238,6 +243,7 @@
 
     test('set date and time button', function() {
       dateTime = initializeDateTime(getFakePrefs(), false);
+      cr.webUIListenerCallback('time-zone-auto-detect-policy', false);
 
       var showSetDateTimeUICalled = false;
       registerMessageCallback('showSetDateTimeUI', null, function() {