Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2829383002:
PolymerTest.clearBody: don't remove vulcanized styles (Closed)
DescriptionPolymerTest.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}
Committed: https://chromium.googlesource.com/chromium/src/+/2008e5c933a8a197aacc44422a65ac26c9814692
Patch Set 1 #
Total comments: 3
Patch Set 2 : simplify #Patch Set 3 : comment #
Dependent Patchsets: Messages
Total messages: 22 (11 generated)
The CQ bit was checked by michaelpg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thoughts? Code would be simpler if we kept the whole div intact, and maybe that's better. But it would mean more changes from the status quo, so I'm on the fence.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2829383002/diff/1/chrome/test/data/webui/poly... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/2829383002/diff/1/chrome/test/data/webui/poly... chrome/test/data/webui/polymer_browser_test_base.js:195: var vulcanizeDiv = document.querySelector('body > div[hidden][by-vulcanize]'); Would it be simpler to do the following? var toRemove = document.body.querySelectorAll('[hidden][by-vulcanize] :not(style)'); // iterate toRemove and call remove() https://codereview.chromium.org/2829383002/diff/1/chrome/test/data/webui/poly... chrome/test/data/webui/polymer_browser_test_base.js:200: if (!vulcanizeDiv.hasAttribute('processed-by-test')) { What is "processed-by-test"?
https://codereview.chromium.org/2829383002/diff/1/chrome/test/data/webui/poly... File chrome/test/data/webui/polymer_browser_test_base.js (right): https://codereview.chromium.org/2829383002/diff/1/chrome/test/data/webui/poly... chrome/test/data/webui/polymer_browser_test_base.js:195: var vulcanizeDiv = document.querySelector('body > div[hidden][by-vulcanize]'); On 2017/04/24 at 18:35:15, dpapad wrote: > Would it be simpler to do the following? > > var toRemove = document.body.querySelectorAll('[hidden][by-vulcanize] :not(style)'); > // iterate toRemove and call remove() Or maybe even better to use the following selector: '[hidden][by-vulcanize] > :not(style)'
The CQ bit was checked by michaelpg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
If we don't care about only preserving <style> tags -- I don't see the harm in preserving all of the div's content, e.g. <dom-module> tags -- this is a simpler way.
On 2017/05/04 at 22:55:12, michaelpg wrote: > If we don't care about only preserving <style> tags -- I don't see the harm in preserving all of the div's content, e.g. <dom-module> tags -- this is a simpler way. If that works, SGTM. BTW, we have some tests that directly replace innerHTML, and for those ones the styles would still be missing. Example https://cs.chromium.org/search/?q=innerHTML+file:%5Esrc/chrome/test/data/webu... (I don't think we need to change them, until it proves to be a problem).
On 2017/05/04 23:09:48, dpapad wrote: > On 2017/05/04 at 22:55:12, michaelpg wrote: > > If we don't care about only preserving <style> tags -- I don't see the harm in > preserving all of the div's content, e.g. <dom-module> tags -- this is a simpler > way. > > If that works, SGTM. BTW, we have some tests that directly replace innerHTML, > and for those ones the styles would still be missing. Example > https://cs.chromium.org/search/?q=innerHTML+file:%5Esrc/chrome/test/data/webu... > (I don't think we need to change them, until it proves to be a problem). Good catch. I'd say they shouldn't be doing that ;-) In particular, testing the search feature with the <div> intact could catch issues if for some reason search started crawling the whole <body>, right?
On 2017/05/04 at 23:12:26, michaelpg wrote: > On 2017/05/04 23:09:48, dpapad wrote: > > On 2017/05/04 at 22:55:12, michaelpg wrote: > > > If we don't care about only preserving <style> tags -- I don't see the harm in > > preserving all of the div's content, e.g. <dom-module> tags -- this is a simpler > > way. > > > > If that works, SGTM. BTW, we have some tests that directly replace innerHTML, > > and for those ones the styles would still be missing. Example > > https://cs.chromium.org/search/?q=innerHTML+file:%5Esrc/chrome/test/data/webu... > > (I don't think we need to change them, until it proves to be a problem). > > Good catch. I'd say they shouldn't be doing that ;-) > > In particular, testing the search feature with the <div> intact could catch issues if for some reason search started crawling the whole <body>, right? In theory yes, in practice, search feature starts searching from a route that is explicitly passed (see https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/search_s..., 2nd param). I don't think is worth updating that test to implicitly cover that "body" is not searched. I would much rather prefer an explicit test that did that, if that was important to check.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/04 23:26:47, dpapad wrote: > On 2017/05/04 at 23:12:26, michaelpg wrote: > > On 2017/05/04 23:09:48, dpapad wrote: > > > On 2017/05/04 at 22:55:12, michaelpg wrote: > > > > If we don't care about only preserving <style> tags -- I don't see the > harm in > > > preserving all of the div's content, e.g. <dom-module> tags -- this is a > simpler > > > way. > > > > > > If that works, SGTM. BTW, we have some tests that directly replace > innerHTML, > > > and for those ones the styles would still be missing. Example > > > > https://cs.chromium.org/search/?q=innerHTML+file:%5Esrc/chrome/test/data/webu... > > > (I don't think we need to change them, until it proves to be a problem). > > > > Good catch. I'd say they shouldn't be doing that ;-) > > > > In particular, testing the search feature with the <div> intact could catch > issues if for some reason search started crawling the whole <body>, right? > > In theory yes, in practice, search feature starts searching from a route that is > explicitly passed (see > https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/search_s..., > 2nd param). I don't think is worth updating that test to implicitly cover that > "body" is not searched. I would much rather prefer an explicit test that did > that, if that was important to check. Definitely. I think we're on the same page -- does this LGTY?
On 2017/05/04 at 23:59:08, michaelpg wrote: > On 2017/05/04 23:26:47, dpapad wrote: > > On 2017/05/04 at 23:12:26, michaelpg wrote: > > > On 2017/05/04 23:09:48, dpapad wrote: > > > > On 2017/05/04 at 22:55:12, michaelpg wrote: > > > > > If we don't care about only preserving <style> tags -- I don't see the > > harm in > > > > preserving all of the div's content, e.g. <dom-module> tags -- this is a > > simpler > > > > way. > > > > > > > > If that works, SGTM. BTW, we have some tests that directly replace > > innerHTML, > > > > and for those ones the styles would still be missing. Example > > > > > > https://cs.chromium.org/search/?q=innerHTML+file:%5Esrc/chrome/test/data/webu... > > > > (I don't think we need to change them, until it proves to be a problem). > > > > > > Good catch. I'd say they shouldn't be doing that ;-) > > > > > > In particular, testing the search feature with the <div> intact could catch > > issues if for some reason search started crawling the whole <body>, right? > > > > In theory yes, in practice, search feature starts searching from a route that is > > explicitly passed (see > > https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/search_s..., > > 2nd param). I don't think is worth updating that test to implicitly cover that > > "body" is not searched. I would much rather prefer an explicit test that did > > that, if that was important to check. > > Definitely. I think we're on the same page -- does this LGTY? Yes, this is still LGTM
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493942519265470,
"parent_rev": "e21cf8f0b7ad21d58dcd2c6d06b35ce722e7e94f", "commit_rev":
"2008e5c933a8a197aacc44422a65ac26c9814692"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/2008e5c933a8a197aacc44422a65... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2008e5c933a8a197aacc44422a65... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
