MD Settings: Fix timezone autodetect policy indicator.

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 (function() {
   function getFakePrefs() {
     return {
       cros: {
         system: {
           timezone: {
@@ skipping 103 matching lines @@
       assertEquals(autoDetect, dateTime.$$('settings-dropdown-menu').disabled);
       assertEquals(autoDetect, dateTime.$.timeZoneAutoDetect.checked);
     }
 
     function verifyPolicy(policy) {
       Polymer.dom.flush();
       var indicator = dateTime.$$('cr-policy-pref-indicator');
 
       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);
     }
 
     function verifyTimeZonesPopulated(populated) {
       var dropdown = dateTime.$$('settings-dropdown-menu');
       if (populated)
         assertEquals(fakeTimeZones.length, dropdown.menuOptions.length);
       else
@@ skipping 19 matching lines @@
       setTimeout(function() {
         verifyTimeZonesPopulated(true);
         done();
       });
     });
 
     test('auto-detect off', function(done) {
       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);
 
       verifyAutoDetectSetting(false);
       verifyPolicy(false);
 
       setTimeout(function() {
         verifyTimeZonesPopulated(true);
 
         // Enable auto-detect.
         MockInteractions.tap(dateTime.$.timeZoneAutoDetect);
         verifyAutoDetectSetting(true);
         done();
       });
     });
 
     test('auto-detect forced on', function(done) {
       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);
 
       verifyAutoDetectSetting(true);
       verifyTimeZonesPopulated(false);
       verifyPolicy(true);
 
       // Cannot disable auto-detect.
       MockInteractions.tap(dateTime.$.timeZoneAutoDetect);
       verifyAutoDetectSetting(true);
       assertFalse(getTimeZonesCalled);
 
       // Update the policy: force auto-detect off.
       cr.webUIListenerCallback('time-zone-auto-detect-policy', true, false);
       verifyAutoDetectSetting(false);
       verifyPolicy(true);
 
       assertTrue(getTimeZonesCalled);
       setTimeout(function() {
         verifyTimeZonesPopulated(true);
         done();
       });
     });
 
     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);
 
       verifyAutoDetectSetting(false);
       verifyPolicy(true);
 
       setTimeout(function() {
         verifyTimeZonesPopulated(true);
 
         // Remove the policy so user's preference takes effect.
         cr.webUIListenerCallback('time-zone-auto-detect-policy', false);
         verifyAutoDetectSetting(true);
         verifyPolicy(false);
 
         // User can disable auto-detect.
         MockInteractions.tap(dateTime.$.timeZoneAutoDetect);
         verifyAutoDetectSetting(false);
         done();
       });
     });
 
     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() {
         assertFalse(showSetDateTimeUICalled);
         showSetDateTimeUICalled = true;
       });
 
       var setDateTimeButton = dateTime.$.setDateTime;
       assertEquals(0, setDateTimeButton.offsetHeight);
 
       // Make the date and time editable.
       cr.webUIListenerCallback('can-set-date-time-changed', true);
       assertGT(setDateTimeButton.offsetHeight, 0);
 
       assertFalse(showSetDateTimeUICalled);
       MockInteractions.tap(setDateTimeButton);
       assertTrue(showSetDateTimeUICalled);
 
       // Make the date and time not editable.
       cr.webUIListenerCallback('can-set-date-time-changed', false);
       assertEquals(setDateTimeButton.offsetHeight, 0);
     });
   });
 })();