New issue
Advanced search Search tips

Issue 714359 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 641400



Sign in to add a comment

PolymerTest.clearBody removes CSS variable definitions

Project Member Reported by michae...@chromium.org, Apr 22 2017

Issue description

Tests for MD Settings and other Polymer WebUI use a helper method, PolymerTest.clearBody, which clears document.body.innerHTML. This removes any content from the page, allowing mocha tests to remove some of the state they created.

However, in a vulcanized build, it also removes root-level CSS variable declarations, permanently. This means any CSS variables declared on :root inside imported styles, like shared_vars_css.html or settings_vars_css.html, no longer apply. This can break test assumptions.

This happens because:

1. Variable declarations on :root normally come from HTML imports, like shared_vars_css.html or settings_vars_css.html.
2. Vulcanize inlines these imports into the main page, moving the <style> tags that declare these :root CSS variables into a <div> that it manages.
3. PolymerTest.clearBody() removes that div, and therefore those <style> tags.

When a test creates a Polymer element and adds it to <body>, because the root no longer has definitions for the CSS variables we expect to have globally, all CSS declarations that use these CSS variables are affected. They will either be invalid (thus ignored), or take on a fallback value if they have one (different from what we expect).

A workaround Demetrious and I came up with: update PolymerTest.clearBody() so instead of blindly removing everything, it preserves all the <style>s inside the special <div hidden by-vulcanize> that vulcanize adds. (Removing <link>s, <script>s and <dom-module>s doesn't seem to break anything, so we'll continue to remove those in order to minimize the impact of this change on our tests.)

Marking UI>Settings as it's "blocking" the MD Settings page's Languages test refactoring, but hasn't caused any other problems that we know of.
 
CL at https://codereview.chromium.org/2829383002

Attaching two screenshots, each showing what test that attaches a <settings-languages-page> element after calling PolymerTest.clearBody() might look like:
 * unvulcanized-standalone.png - in a use_vulcanize = false build, it mostly looks normal
 * vulcanized-standalone.png - in a use_vulcanize = true build, all bets are off

unvulcanized-standalone.png
50.1 KB View Download
vulcanized-standalone.png
48.3 KB View Download

Comment 2 by dpa...@chromium.org, Apr 24 2017

Labels: Hotlist-MD-Settings-Tests
Busy gardening this week. Will finish CL on Monday: https://codereview.chromium.org/2837703002/
Status: Assigned (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, May 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2008e5c933a8a197aacc44422a65ac26c9814692

commit 2008e5c933a8a197aacc44422a65ac26c9814692
Author: michaelpg <michaelpg@chromium.org>
Date: Fri May 05 00:09:53 2017

PolymerTest.clearBody: don't remove vulcanized styles

Vulcanize removes HTML imports of global style definitions and inlines
those <style> elements directly in a special <div> on the page. Removing
this <div> in tests means those <style>s go away. Without those styles,
elements being tested no longer have access to the CSS variables or
global style rules they expect, which can break things in all sorts
of ways.

This hasn't affected any tests that I know of (besides one WIP), because
we don't normally test the details of our colors and layouts in tests that
call PolymerTest.clearBody. But it's worth fixing regardless in order to
decrease the delta between tests in vulcanized vs. unvulcanized builds.

BUG= 714359 
R=dpapad@chromium.org

Review-Url: https://codereview.chromium.org/2829383002
Cr-Commit-Position: refs/heads/master@{#469534}

[modify] https://crrev.com/2008e5c933a8a197aacc44422a65ac26c9814692/chrome/test/data/webui/polymer_browser_test_base.js

Status: Fixed (was: Assigned)

Sign in to add a comment