fix(dialog): Add redlines to dialog screenshots; update to match spec (#3602)

Author: acdvorak

packages/mdc-dialog/README.md

@@ -115,7 +115,7 @@ The Simple Dialog contains a list of potential actions. It does not contain butt
    -->Choose a Ringtone<!--
  --></h2>
     <section class="mdc-dialog__content" id="my-dialog-content">
-      <ul class="mdc-list">
+      <ul class="mdc-list mdc-list--avatar-list">
         <li class="mdc-list-item" data-mdc-dialog-action="none">
           <span class="mdc-list-item__text">None</span>
         </li>
@@ -130,6 +130,8 @@ The Simple Dialog contains a list of potential actions. It does not contain butt
 </div>
 ```
 
+> Note the inclusion of the `mdc-list--avatar-list` class, which aligns with the Simple Dialog spec.
+
 ### Confirmation Dialog
 
 The Confirmation Dialog contains a list of choices, and buttons to confirm or cancel. Choices are accompanied by

packages/mdc-dialog/_variables.scss

@@ -33,6 +33,7 @@ $mdc-dialog-scroll-divider-opacity: .12;
 $mdc-dialog-min-width: 280px;
 $mdc-dialog-max-width: 560px;
 $mdc-dialog-margin: 16px;
+$mdc-dialog-title-bottom-padding: 9px;
 
 $mdc-dialog-corner-radius: 4px;
 

packages/mdc-dialog/mdc-dialog.scss

@@ -98,16 +98,22 @@
   position: relative;
   flex-shrink: 0;
   box-sizing: border-box;
-  min-height: 64px;
   margin: 0;
-  padding: 0 24px 15px;
+  padding: 0 24px $mdc-dialog-title-bottom-padding;
   border-bottom: 1px solid transparent;
 
   @include mdc-rtl(".mdc-dialog") {
     text-align: right;
   }
 }
 
+// stylelint-disable-next-line plugin/selector-bem-pattern
+.mdc-dialog--scrollable .mdc-dialog__title {
+  // Adjust bottom padding to make title height align to spec when divider is present.
+  // (Titles for alert dialogs w/o dividers align based on text baseline instead. All spec values are divisible by 4.)
+  padding-bottom: $mdc-dialog-title-bottom-padding + 6px;
+}
+
 .mdc-dialog__content {
   @include mdc-typography(body1);
 
@@ -146,6 +152,12 @@
 
 // stylelint-disable-next-line plugin/selector-bem-pattern
 .mdc-dialog__content .mdc-list:first-child:last-child {
+  // Override default .mdc-list padding for content consisting exclusively of a MDC List
+  padding: 6px 0 0; // Top padding balances with title height
+}
+
+// stylelint-disable-next-line plugin/selector-bem-pattern
+.mdc-dialog--scrollable .mdc-dialog__content .mdc-list:first-child:last-child {
   // Override default .mdc-list padding for content consisting exclusively of a MDC List
   padding: 0;
 }

packages/mdc-list/_mixins.scss

@@ -88,4 +88,5 @@
 
   width: $size;
   height: $size;
+  font-size: $size; // To support icon font
 }

test/screenshot/diffing.json

@@ -64,19 +64,6 @@
           "min_changed_pixel_count": 25,
           "fonts_loaded_reflow_delay_ms": 250
         }
-      },
-      {
-        "description": "IE consistently flakes sometimes on some dialog pages.",
-        "browser_regex_patterns": [
-          "desktop_windows_ie@11"
-        ],
-        "url_regex_patterns": [
-          "mdc-dialog/classes/overflow",
-          "mdc-dialog/mixins"
-        ],
-        "custom_config": {
-          "skip_all": true
-        }
       }
     ]
   }

test/screenshot/golden.json

@@ -33,12 +33,15 @@ window.mdc = window.mdc || {};
  *   fromSide: string,
  *   toEl: ?Element,
  *   toSide: string,
+ *   orientation: ?string,
  *   specDistancePx: number,
  *   displayOffsetPx: number,
  *   displayAlignment: string,
  *   displayTargetEl: ?Element,
  *   lineEl: ?Element,
  *   labelEl: ?Element,
+ *   flipLabel: ?boolean,
+ *   conditionFn: ?function(): boolean,
  * }} RedlineConfig
  */
 
@@ -146,12 +149,14 @@ class TestFixture {
 
     this.redlineContainerEl_.appendChild(lineEl);
 
-    this.redlineConfigs_.push(Object.assign({
+    /** @type {!RedlineConfig} */
+    const mergedConfig = Object.assign({
       lineEl,
       labelEl,
       displayOffsetPx: 0,
       displayAlignment: 'center',
-    }, config));
+      flipLabel: false,
+    }, config);
 
     mergedConfig.orientation = this.getOrientation_(mergedConfig);
 
@@ -175,21 +180,25 @@ class TestFixture {
       }
 
       this.redlineConfigs_.forEach((config) => {
-        const {lineEl, fromSide} = config;
-        lineEl.classList.remove(
-          'test-redline--vertical',
-          'test-redline--horizontal',
-          'test-redline--pass',
-          'test-redline--warn',
-          'test-redline--small',
-        );
-        if (fromSide === 'top' || fromSide === 'bottom' ||
-            fromSide === 'first-baseline' || fromSide === 'last-baseline') {
+        const {lineEl, orientation, flipLabel, conditionFn} = config;
+
+        // Remove all modifier classes
+        lineEl.className = 'test-redline';
+
+        if (conditionFn && !conditionFn()) {
+          lineEl.classList.add('test-redline--hidden');
+        }
+
+        if (flipLabel) {
+          lineEl.classList.add('test-redline--flipped');
+        }
+
+        if (orientation === 'vertical') {
           this.drawVerticalRedline_(config);
-        } else if (fromSide === 'left' || fromSide === 'right') {
+        } else if (orientation === 'horizontal') {
           this.drawHorizontalRedline_(config);
         } else {
-          throw new Error(`Unsupported \`fromSide\` value: "${fromSide}"`);
+          throw new Error(`Unsupported 'orientation' value: "${orientation}"`);
         }
       });
     });
@@ -200,7 +209,9 @@ class TestFixture {
    * @private
    */
   drawVerticalRedline_(config) {
-    const {lineEl, labelEl, fromEl, fromSide, toEl, toSide, specDistancePx, displayOffsetPx, displayAlignment} = config;
+    const {lineEl, labelEl, fromEl, toEl, fromSide, toSide} = config;
+    const {specDistancePx, displayOffsetPx, displayAlignment} = config;
+
     lineEl.classList.add('test-redline--vertical');
 
     const fromRect = fromEl.getBoundingClientRect();
@@ -235,7 +246,7 @@ class TestFixture {
       labelEl.innerHTML = `${actualDistancePx}px`;
       lineEl.classList.add('test-redline--pass');
     } else if (Math.abs(actualDistancePx - specDistancePx) <= 1) {
-      labelEl.innerHTML = `Spec: ${specDistancePx}px<br>Actual: ${actualDistancePx}px`;
+      labelEl.innerHTML = `${actualDistancePx}px`;
       lineEl.classList.add('test-redline--warn');
     } else {
       labelEl.innerHTML = `Spec: ${specDistancePx}px<br>Actual: ${actualDistancePx}px`;
@@ -296,7 +307,7 @@ class TestFixture {
       labelEl.innerHTML = `${actualDistancePx}px`;
       lineEl.classList.add('test-redline--pass');
     } else if (Math.abs(actualDistancePx - specDistancePx) <= 1) {
-      labelEl.innerHTML = `Spec: ${specDistancePx}px<br>Actual: ${actualDistancePx}px`;
+      labelEl.innerHTML = `${actualDistancePx}px`;
       lineEl.classList.add('test-redline--warn');
     } else {
       labelEl.innerHTML = `Spec: ${specDistancePx}px<br>Actual: ${actualDistancePx}px`;
@@ -320,6 +331,7 @@ class TestFixture {
     const borderTopWidth = parseInt(getComputedStyle(el).borderTopWidth, 10);
     const borderLeftWidth = parseInt(getComputedStyle(el).borderLeftWidth, 10);
     const borderRightWidth = parseInt(getComputedStyle(el).borderRightWidth, 10);
+    const scrollbarWidth = this.getScrollbarWidth_(el);
 
     if (side === 'top') {
       return rect.top + borderTopWidth;
@@ -331,7 +343,7 @@ class TestFixture {
       return rect.left + borderLeftWidth;
     }
     if (side === 'right') {
-      return rect.right - borderRightWidth;
+      return rect.right - borderRightWidth + scrollbarWidth;
     }
 
     if (side === 'first-baseline' || side === 'last-baseline') {

test/screenshot/spec/fixture.js

@@ -175,7 +175,11 @@ $test-layout-cell-grid-color: #ddd;
 }
 
 .test-redline--warn {
-  color: deeppink;
+  color: darkorange;
+}
+
+.test-redline--hidden {
+  display: none;
 }
 
 .test-redline__tick {
@@ -237,14 +241,34 @@ $test-layout-cell-grid-color: #ddd;
   transform: translateX(-50%);
 }
 
-.test-redline--small.test-redline--vertical .test-redline__label {
+.test-redline--vertical.test-redline--flipped .test-redline__label {
+  right: auto;
+  left: 2px;
+}
+
+.test-redline--horizontal.test-redline--flipped .test-redline__label {
+  bottom: auto;
+  left: 50%;
+}
+
+.test-redline--vertical.test-redline--small .test-redline__label {
   right: 13px;
 }
 
-.test-redline--small.test-redline--horizontal .test-redline__label {
+.test-redline--horizontal.test-redline--small .test-redline__label {
   bottom: 13px;
 }
 
+.test-redline--vertical.test-redline--small.test-redline--flipped .test-redline__label {
+  right: auto;
+  left: 13px;
+}
+
+.test-redline--horizontal.test-redline--small.test-redline--flipped .test-redline__label {
+  top: 13px;
+  bottom: auto;
+}
+
 .test-baseline-probe {
   display: inline-block;
   width: 0;

test/screenshot/spec/fixture.scss

@@ -39,7 +39,7 @@
 
       <button class="test-dialog-open-button" data-test-dialog-id="test-dialog" autofocus>Open Dialog</button>
 
-      <div class="mdc-dialog mdc-dialog--open test-dialog"
+      <div class="mdc-dialog mdc-dialog--open test-dialog test-dialog--simple"
            role="alertdialog"
            aria-modal="true"
            aria-labelledby="test-dialog__title--simple"
@@ -56,7 +56,7 @@ <h2 class="mdc-dialog__title test-dialog__title" id="test-dialog__title--simple"
                   Inline `style="list-style-type: none"` needed to prevent rendering bug in Edge and IE 11.
                   See https://stackoverflow.com/a/23717689/467582
                 -->
-                <ul class="mdc-list" aria-orientation="vertical" style="list-style-type: none;">
+                <ul class="mdc-list mdc-list--avatar-list" aria-orientation="vertical" style="list-style-type: none;">
                   <li class="mdc-list-item" tabindex="0" data-mdc-dialog-action="wifi">
                     <i class="material-icons mdc-list-item__graphic">network_wifi</i>
                     <span class="test-list-item__label">Wi-Fi</span>