MD Settings: Fix subpage visibility and add appearance page tests

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

Index: chrome/browser/resources/settings/appearance_page/appearance_page.html
diff --git a/chrome/browser/resources/settings/appearance_page/appearance_page.html b/chrome/browser/resources/settings/appearance_page/appearance_page.html
index fcc30552e0f7c06a18b6a4641c156b9c1bab9162..b50842c80bb4db9d5d382773ea83e1ba741adff0 100644
--- a/chrome/browser/resources/settings/appearance_page/appearance_page.html
+++ b/chrome/browser/resources/settings/appearance_page/appearance_page.html
@@ -22,6 +22,12 @@
 <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;
       }
@@ -41,9 +47,9 @@
         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">
@@ -54,13 +60,9 @@
               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">
@@ -104,8 +106,8 @@
           </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}"
@@ -116,7 +118,8 @@
           </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]]"
@@ -144,23 +147,23 @@
             </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>
-        <div class="settings-box">
 </if>
+        <div class="settings-box">
           <div class="start">$i18n{fontSize}</div>
           <settings-dropdown-menu id="defaultFontSize" label="$i18n{fontSize}"
               pref="{{prefs.webkit.webprefs.default_font_size}}"
@@ -175,7 +178,8 @@
           <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"