MD Settings: Fix subpage visibility and add appearance page tests

chrome/browser/resources/settings/appearance_page/appearance_page.html

 <link rel="import" href="chrome://resources/html/i18n_behavior.html">
 <link rel="import" href="chrome://resources/html/md_select_css.html">
 <link rel="import" href="chrome://resources/html/polymer.html">
 <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout-classes.html">
 <link rel="import" href="chrome://resources/polymer/v1_0/neon-animation/neon-animatable.html">
 <link rel="import" href="chrome://resources/polymer/v1_0/paper-button/paper-button.html">
 <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button-light.html">
 <link rel="import" href="appearance_browser_proxy.html">
 <link rel="import" href="../controls/controlled_radio_button.html">
 <link rel="import" href="../controls/extension_controlled_indicator.html">
 <link rel="import" href="../controls/settings_dropdown_menu.html">
 <link rel="import" href="../controls/settings_radio_group.html">
 <link rel="import" href="../controls/settings_toggle_button.html">
 <link rel="import" href="../route.html">
 <link rel="import" href="../settings_page/settings_animated_pages.html">
 <link rel="import" href="../settings_page/settings_subpage.html">
 <link rel="import" href="../settings_shared_css.html">
 <link rel="import" href="../settings_vars_css.html">
 <link rel="import" href="appearance_fonts_page.html">
 <link rel="import" href="home_url_input.html">
 
 <dom-module id="settings-appearance-page">
   <template>
     <style include="settings-shared md-select iron-flex">
+      /* Conditional logic makes identifying the first settings-box complicated
+         so use first-of-type here. */
+      settings-box:first-of-type {

[dpapad, 2017/05/03 23:02:24]

I don't think this works at all. :first-of-type can only be used with valid HTML
Types (div, span etc). It can't even be used with classes, so even

.settings-box:first-of-type {...} would do nothing AFAIK.


Are you trying to fix a bug that already exists in the code? Meaning that the
<div class="settings-box first...> on the left exists only on CrOS, which means
that on non-ChromeOS there is no settings-box marked with |first|?

[stevenjb, 2017/05/03 23:31:43]

Ugh, you're right. It's super subtle when at the top of a page/section so I
didn't notice.

It's supposed to be .settings-box:first-of-type, which does work and which we
use elsewhere. :first-of-type "represents the first sibling of its type in the
list of children of its parent element". So in this case, for anything matching
.settings-box (which will be a div), if it is the first div of its siblings we
apply the styling (which is exactly what we want).

Did you take a look at the <if> hackery that this had before? It was super
challenging to parse, and could still be wrong if we changed what we hide e.g.
in Guest mode.

[stevenjb, 2017/05/03 23:33:16]

Oh, ugh, I need to check [hidden] also... one moment...

[stevenjb, 2017/05/03 23:36:51]

aaaaand this only works with dom-if, right. Which is why I switched to dom-if.
Sigh.

Guess I'll just upload the minimal patch to fix this, I'm sick of it.

+        border-top: none;
+      }
+
       .secondary-button ~ .secondary-button {
         -webkit-margin-start: 12px;
       }
 
       #custom-input {
         margin-bottom: 1rem;
       }
 
       #customHomePage {
         --paper-input-container: {
           width: 200px;
         };
         width: 100%;  /* Pushes policy indicators to end. */
       }
     </style>
     <settings-animated-pages id="pages" section="appearance"
         focus-config="[[focusConfig_]]">
       <neon-animatable route-path="default">
 <if expr="chromeos">
-        <div class="settings-box first two-line" id="wallpaperButton"
+        <div class="settings-box two-line" id="setWallpaper" 
             on-tap="openWallpaperManager_" actionable
-            hidden="[[!pageVisibility.setWallpaper]]">
+            hidden="[[!showPage_(pageVisibility.setWallpaper)]]">

[dpapad, 2017/05/03 23:02:24]

Isn't this equivalent to what is already on the left?

hidden="[[!pageVisibility.setWallpaper]]">

Essentially the showPage_() wrapper only made sense when we were switching from
hidden to dom-if. Does it add any value anymore (here and elsewhere)?

[stevenjb, 2017/05/03 23:31:43]

The change makes this more robust and, imho, more readable.

I really didn't like that using hidden *happens* to work when the property was
undefined. It requires a pretty deep understanding of Polymer to recognize that
when setWallpaper is 'undefined' (which is usually falsy) hidden never gets set
and therefore does not get initialized to false. (The first time I looked at
this code I was surprised by this. and theoretically I have a pretty decent
understanding of Polymer at this point...)

Plus now if someone comes along and switches this to use dom-if, it will still
work.

           <div class="start">
             $i18n{setWallpaper}
             <div class="secondary" id="wallpaperSecondary">
               $i18n{openWallpaperApp}
             </div>
           </div>
           <button class="icon-external" is="paper-icon-button-light"
               aria-label="$i18n{setWallpaper}"
               aria-describedby="wallpaperSecondary"></button>
         </div>
-        <div class="settings-box two-line"
-            hidden="[[!pageVisibility.setTheme]]">
 </if>
-<if expr="not chromeos">
-        <div class="settings-box two-line first"
-            hidden="[[!pageVisibility.setTheme]]">
-</if>
+        <div id="setTheme" class="settings-box two-line"
+            hidden="[[!showPage_(pageVisibility.setTheme)]]">
           <a class="start two-line inherit-color no-outline" tabindex="-1"
               target="_blank" href$="[[getThemeHref_(themeUrl_)]]">
             <div class="flex">
               $i18n{themes}
               <div class="secondary" id="themesSecondary">
                 [[themeSublabel_]]
               </div>
             </div>
             <button class="icon-external" is="paper-icon-button-light"
                 actionable aria-label="$i18n{themes}"
@@ skipping 23 matching lines @@
             <template is="dom-if" if="[[showUseSystem_(
                 prefs.extensions.theme.id.value, useSystemTheme_)]]" restamp>
               <paper-button id="useSystem" on-tap="onUseSystemTap_"
                   class="secondary-button">
                 $i18n{useSystemTheme}
               </paper-button>
             </template>
           </div>
 </if>
         </div>
-        <div class="settings-box"
-            hidden="[[!pageVisibility.homeButton]]">
+        <div id="homeButton" class="settings-box"
+            hidden="[[!showPage_(pageVisibility.homeButton)]]">
           <settings-toggle-button class="start" elide-label
               pref="{{prefs.browser.show_home_button}}"
               label="$i18n{showHomeButton}"
               sub-label="[[getShowHomeSubLabel_(
                   prefs.browser.show_home_button.value,
                   prefs.homepage_is_newtabpage.value,
                   prefs.homepage.value)]]">
           </settings-toggle-button>
         </div>
         <template is="dom-if" if="[[prefs.browser.show_home_button.value]]">
-          <div class="list-frame" hidden="[[!pageVisibility.homeButton]]">
+          <div class="list-frame"
+              hidden="[[!showPage_(pageVisibility.homeButton)]]">
             <settings-radio-group pref="{{prefs.homepage_is_newtabpage}}">
               <controlled-radio-button class="list-item" name="true"
                   pref="[[prefs.homepage_is_newtabpage]]"
                   label="$i18n{homePageNtp}" no-extension-indicator>
               </controlled-radio-button>
               <controlled-radio-button id="custom-input" class="list-item"
                   name="false" pref="[[prefs.homepage_is_newtabpage]]"
                   no-extension-indicator>
                 <!-- TODO(dbeam): this can show double indicators when both
                      homepage and whether to use the NTP as the homepage are
                      managed. -->
                 <home-url-input id="customHomePage" pref="{{prefs.homepage}}"
                     can-tab="[[!prefs.homepage_is_newtabpage.value]]">
                 </home-url-input>
               </controlled-radio-button>
               <template is="dom-if" if="[[prefs.homepage.extensionId]]">
                 <extension-controlled-indicator
                     extension-id="[[prefs.homepage.extensionId]]"
                     extension-can-be-disabled="[[
                         prefs.homepage.extensionCanBeDisabled]]"
                     extension-name="[[prefs.homepage.controlledByName]]"
                     on-disable-extension="onDisableExtension_">
                 </extension-controlled-indicator>
               </template>
             </settings-radio-group>
           </div>
         </template>
-        <div class="settings-box"
-            hidden="[[!pageVisibility.bookmarksBar]]">
+        <div id="bookmarksBar" class="settings-box"
+            hidden="[[!showPage_(pageVisibility.bookmarksBar)]]">
           <settings-toggle-button class="start"
               pref="{{prefs.bookmark_bar.show_on_all_tabs}}"
               label="$i18n{showBookmarksBar}">
           </settings-toggle-button>
         </div>
-        <div class$="settings-box [[getFirst_(pageVisibility.bookmarksBar)]]">

[dpapad, 2017/05/03 23:02:24]

Isn't this addressing the problem of determining which settings-box should have
|first|? I don't think this is being addressed anymore, erroneously.

[stevenjb, 2017/05/03 23:31:43]

The new CSS does the right thing now, always, regardless of which sections we
decide to hide, and with better readability. I *hate* this pattern, especially
the mess around line 60.

 <if expr="is_linux and not chromeos">
+        <div class$="settings-box">
           <settings-toggle-button class="start"
               pref="{{prefs.browser.custom_chrome_frame}}"
               label="$i18n{showWindowDecorations}"
               inverted>
           </settings-toggle-button>
         </div>
+</if>
         <div class="settings-box">
-</if>
           <div class="start">$i18n{fontSize}</div>
           <settings-dropdown-menu id="defaultFontSize" label="$i18n{fontSize}"
               pref="{{prefs.webkit.webprefs.default_font_size}}"
               menu-options="[[fontSizeOptions_]]">
           </settings-dropdown-menu>
         </div>
         <div id="customize-fonts-subpage-trigger" class="settings-box"
             on-tap="onCustomizeFontsTap_" actionable>
           <div class="start">
             $i18n{customizeFonts}
           </div>
           <button class="subpage-arrow" is="paper-icon-button-light"
               aria-label="$i18n{customizeFonts}"></button>
         </div>
-        <div class="settings-box" hidden="[[!pageVisibility.pageZoom]]">
+        <div class="settings-box"
+            hidden="[[!showPage_(pageVisibility.pageZoom)]]">
           <div id="pageZoom" class="start">$i18n{pageZoom}</div>
           <div class="md-select-wrapper">
             <select id="zoomLevel" class="md-select" aria-labelledby="pageZoom"
                 on-change="onZoomLevelChange_">
               <template is="dom-repeat" items="[[pageZoomLevels_]]">
                 <option value="[[item]]"
                     selected="[[zoomValuesEqual_(item, defaultZoom_)]]">
                   [[formatZoom_(item)]]%
                 </option>
               </template>
             </select>
             <span class="md-select-underline"></span>
           </div>
         </div>
       </neon-animatable>
       <template is="dom-if" route-path="/fonts">
         <settings-subpage
             associated-control="[[$$('#customize-fonts-subpage-trigger')]]"
             page-title="$i18n{customizeFonts}">
           <settings-appearance-fonts-page prefs="{{prefs}}">
           </settings-appearance-fonts-page>
         </settings-subpage>
       </template>
     </settings-animated-pages>
   </template>
   <script src="appearance_page.js"></script>
 </dom-module>