Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
601083004:
Enable a11y audit for chrome://print and fix failing tests. (Closed)
DescriptionEnable a11y audit for chrome://print and fix failing tests.
Enabled a11y audit test.
Made the tests async in order to run the a11y audit after the UI is settled down.
Fix issue where decorative icon had no descriptive text. Added alt="".
Fix issue where margins were obscured by a warning dialog but were still accessible. When elements are obscured by the overlay, they are now hidden.
Cleaned up redundant code by moving it into functions.
BUG=423178
Committed: https://crrev.com/8102b1b83fa3b33624a19bc89fd578566ffe573c
Cr-Commit-Position: refs/heads/master@{#302105}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Debugging Failures #
Total comments: 12
Patch Set 4 : Possible fix of failures and address dbeam@'s comments #Patch Set 5 : #
Total comments: 30
Patch Set 6 : #
Total comments: 19
Patch Set 7 : #Patch Set 8 : Change getElementsByTagName to querySelector #Patch Set 9 : Removed c++ dependency from js for async test #
Total comments: 1
Patch Set 10 : Attempt to fix flaky test found by trybots #
Total comments: 4
Patch Set 11 : Fix ChromeOS timeout #
Messages
Total messages: 29 (6 generated)
hcarmona@chromium.org changed reviewers: + dbeam@chromium.org, dmazzoni@chromium.org
Enabled the a11y audit for the print dialog. One a11y failure was ignored because it appears to be a false positive. The a11y audit reports that a control is hidden but focus-able. After manually inspecting this (and running the a11y library through chrome's console) it is not possible to reproduce this failure outside the browser test. I found another instance where a similar failure was ignored. There was a reference to a bug in the a11y audit: https://github.com/GoogleChrome/accessibility-developer-tools/issues/69
lgtm
lgtm (can you explain why that false positive exists a little bit, though?)
hcarmona@chromium.org changed reviewers: + abodenha@chromium.org
https://codereview.chromium.org/601083004/diff/40001/chrome/browser/resources... File chrome/browser/resources/print_preview/previewarea/margin_control.js (left): https://codereview.chromium.org/601083004/diff/40001/chrome/browser/resources... chrome/browser/resources/print_preview/previewarea/margin_control.js:377: onWebkitTransitionEnd_: function(event) { not lgtm, add back before you land https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:101: setUpPreview: function() { rename to setUp, add /** @override */ and remove all the "this.setUpPreview()" calls https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w... https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:117: // end the test once all expected transitions are done why not when a specific transition ends, e.g. e.target == elementWeCareAbout && e.propertyName == 'prop-we-care-about' https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:439: testDone(); function PrintPreviewWebUIAsyncTest() {} PrintPreviewWebUIAsyncTest.prototype = { __proto__: PrintPreviewWebUITest, /** @override */ isAsync: true, };
https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:125: ensureTransitionEndEvent(document, PrintPreviewWebUITest.TIMEOUT); we should not have to do this https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:439: testDone(); On 2014/10/15 21:37:31, Dan Beam wrote: > function PrintPreviewWebUIAsyncTest() {} > > PrintPreviewWebUIAsyncTest.prototype = { > __proto__: PrintPreviewWebUITest, > > /** @override */ > isAsync: true, > }; (so you don't have to put all these useless testDone() calls everywhere)
Added Albert as code reviewer to look at the print preview changes I made. I've updated to reflect Dan's feedback and addressed the flakiness in the previous upload. https://codereview.chromium.org/601083004/diff/40001/chrome/browser/resources... File chrome/browser/resources/print_preview/previewarea/margin_control.js (left): https://codereview.chromium.org/601083004/diff/40001/chrome/browser/resources... chrome/browser/resources/print_preview/previewarea/margin_control.js:377: onWebkitTransitionEnd_: function(event) { On 2014/10/15 21:37:31, Dan Beam wrote: > not lgtm, add back before you land > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... Acknowledged. https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:101: setUpPreview: function() { Can't do this because not all tests need to call setupPreview(). Also some tests need to change some options before the preview is set up. Added to this the async class because all async tests needed to call setupPreview() and none had to change options before the call. On 2014/10/15 21:37:31, Dan Beam wrote: > rename to setUp, add /** @override */ and remove all the "this.setUpPreview()" > calls > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w... https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:117: // end the test once all expected transitions are done On 2014/10/15 21:37:31, Dan Beam wrote: > why not when a specific transition ends, e.g. > > e.target == elementWeCareAbout && e.propertyName == 'prop-we-care-about' Done. https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:125: ensureTransitionEndEvent(document, PrintPreviewWebUITest.TIMEOUT); On 2014/10/15 21:39:19, Dan Beam wrote: > we should not have to do this Acknowledged. https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:439: testDone(); On 2014/10/15 21:37:31, Dan Beam wrote: > function PrintPreviewWebUIAsyncTest() {} > > PrintPreviewWebUIAsyncTest.prototype = { > __proto__: PrintPreviewWebUITest, > > /** @override */ > isAsync: true, > }; Done. https://codereview.chromium.org/601083004/diff/40001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:439: testDone(); On 2014/10/15 21:39:18, Dan Beam wrote: > On 2014/10/15 21:37:31, Dan Beam wrote: > > function PrintPreviewWebUIAsyncTest() {} > > > > PrintPreviewWebUIAsyncTest.prototype = { > > __proto__: PrintPreviewWebUITest, > > > > /** @override */ > > isAsync: true, > > }; > > (so you don't have to put all these useless testDone() calls everywhere) Acknowledged.
I haven't looked at the Print Preview code in ages. Aleksey would probably be a better choice. Am I still in OWNERS on that? I thought I'd removed myself. On Thu, Oct 16, 2014 at 6:35 PM, <hcarmona@chromium.org> wrote: > Added Albert as code reviewer to look at the print preview changes I made. > > I've updated to reflect Dan's feedback and addressed the flakiness in the > previous upload. > > > https://codereview.chromium.org/601083004/diff/40001/ > chrome/browser/resources/print_preview/previewarea/margin_control.js > File > chrome/browser/resources/print_preview/previewarea/margin_control.js > (left): > > https://codereview.chromium.org/601083004/diff/40001/ > chrome/browser/resources/print_preview/previewarea/ > margin_control.js#oldcode377 > chrome/browser/resources/print_preview/previewarea/margin_control.js:377: > onWebkitTransitionEnd_: function(event) { > On 2014/10/15 21:37:31, Dan Beam wrote: > >> not lgtm, add back before you land >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/chrome/browser/resources/print_preview/previewarea/ > margin_control.css&q=transition.*opacity%20print_ > preview&sq=package:chromium&type=cs&l=6 > > Acknowledged. > > https://codereview.chromium.org/601083004/diff/40001/ > chrome/test/data/webui/print_preview.js > File chrome/test/data/webui/print_preview.js (right): > > https://codereview.chromium.org/601083004/diff/40001/ > chrome/test/data/webui/print_preview.js#newcode101 > chrome/test/data/webui/print_preview.js:101: setUpPreview: function() { > Can't do this because not all tests need to call setupPreview(). Also > some tests need to change some options before the preview is set up. > > Added to this the async class because all async tests needed to call > setupPreview() and none had to change options before the call. > > On 2014/10/15 21:37:31, Dan Beam wrote: > >> rename to setUp, add /** @override */ and remove all the >> > "this.setUpPreview()" > >> calls >> > > > https://code.google.com/p/chromium/codesearch#chromium/ > src/chrome/test/data/webui/test_api.js&q=testing.Test&sq= > package:chromium&type=cs&l=283 > > https://codereview.chromium.org/601083004/diff/40001/ > chrome/test/data/webui/print_preview.js#newcode117 > chrome/test/data/webui/print_preview.js:117: // end the test once all > expected transitions are done > On 2014/10/15 21:37:31, Dan Beam wrote: > >> why not when a specific transition ends, e.g. >> > > e.target == elementWeCareAbout && e.propertyName == >> > 'prop-we-care-about' > > Done. > > https://codereview.chromium.org/601083004/diff/40001/ > chrome/test/data/webui/print_preview.js#newcode125 > chrome/test/data/webui/print_preview.js:125: > ensureTransitionEndEvent(document, PrintPreviewWebUITest.TIMEOUT); > On 2014/10/15 21:39:19, Dan Beam wrote: > >> we should not have to do this >> > > Acknowledged. > > https://codereview.chromium.org/601083004/diff/40001/ > chrome/test/data/webui/print_preview.js#newcode439 > chrome/test/data/webui/print_preview.js:439: testDone(); > On 2014/10/15 21:37:31, Dan Beam wrote: > >> function PrintPreviewWebUIAsyncTest() {} >> > > PrintPreviewWebUIAsyncTest.prototype = { >> __proto__: PrintPreviewWebUITest, >> > > /** @override */ >> isAsync: true, >> }; >> > > Done. > > https://codereview.chromium.org/601083004/diff/40001/ > chrome/test/data/webui/print_preview.js#newcode439 > chrome/test/data/webui/print_preview.js:439: testDone(); > On 2014/10/15 21:39:18, Dan Beam wrote: > >> On 2014/10/15 21:37:31, Dan Beam wrote: >> > function PrintPreviewWebUIAsyncTest() {} >> > >> > PrintPreviewWebUIAsyncTest.prototype = { >> > __proto__: PrintPreviewWebUITest, >> > >> > /** @override */ >> > isAsync: true, >> > }; >> > > (so you don't have to put all these useless testDone() calls >> > everywhere) > > Acknowledged. > > https://codereview.chromium.org/601083004/ > -- Albert Bodenhamer | Software Engineer | abodenha@chromium. <abodenha@google.com>org To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yup, you're in the OWNERS file. I'll change the reviewer to Aleksey. Thanks! Hector On Mon, Oct 20, 2014 at 9:18 AM, Albert Bodenhamer <abodenha@chromium.org> wrote: > I haven't looked at the Print Preview code in ages. Aleksey would probably > be a better choice. > > Am I still in OWNERS on that? I thought I'd removed myself. > > On Thu, Oct 16, 2014 at 6:35 PM, <hcarmona@chromium.org> wrote: > >> Added Albert as code reviewer to look at the print preview changes I made. >> >> I've updated to reflect Dan's feedback and addressed the flakiness in the >> previous upload. >> >> >> https://codereview.chromium.org/601083004/diff/40001/ >> chrome/browser/resources/print_preview/previewarea/margin_control.js >> File >> chrome/browser/resources/print_preview/previewarea/margin_control.js >> (left): >> >> https://codereview.chromium.org/601083004/diff/40001/ >> chrome/browser/resources/print_preview/previewarea/ >> margin_control.js#oldcode377 >> chrome/browser/resources/print_preview/previewarea/margin_control.js:377: >> onWebkitTransitionEnd_: function(event) { >> On 2014/10/15 21:37:31, Dan Beam wrote: >> >>> not lgtm, add back before you land >>> >> >> https://code.google.com/p/chromium/codesearch#chromium/ >> src/chrome/browser/resources/print_preview/previewarea/ >> margin_control.css&q=transition.*opacity%20print_ >> preview&sq=package:chromium&type=cs&l=6 >> >> Acknowledged. >> >> https://codereview.chromium.org/601083004/diff/40001/ >> chrome/test/data/webui/print_preview.js >> File chrome/test/data/webui/print_preview.js (right): >> >> https://codereview.chromium.org/601083004/diff/40001/ >> chrome/test/data/webui/print_preview.js#newcode101 >> chrome/test/data/webui/print_preview.js:101: setUpPreview: function() { >> Can't do this because not all tests need to call setupPreview(). Also >> some tests need to change some options before the preview is set up. >> >> Added to this the async class because all async tests needed to call >> setupPreview() and none had to change options before the call. >> >> On 2014/10/15 21:37:31, Dan Beam wrote: >> >>> rename to setUp, add /** @override */ and remove all the >>> >> "this.setUpPreview()" >> >>> calls >>> >> >> >> https://code.google.com/p/chromium/codesearch#chromium/ >> src/chrome/test/data/webui/test_api.js&q=testing.Test&sq= >> package:chromium&type=cs&l=283 >> >> https://codereview.chromium.org/601083004/diff/40001/ >> chrome/test/data/webui/print_preview.js#newcode117 >> chrome/test/data/webui/print_preview.js:117: // end the test once all >> expected transitions are done >> On 2014/10/15 21:37:31, Dan Beam wrote: >> >>> why not when a specific transition ends, e.g. >>> >> >> e.target == elementWeCareAbout && e.propertyName == >>> >> 'prop-we-care-about' >> >> Done. >> >> https://codereview.chromium.org/601083004/diff/40001/ >> chrome/test/data/webui/print_preview.js#newcode125 >> chrome/test/data/webui/print_preview.js:125: >> ensureTransitionEndEvent(document, PrintPreviewWebUITest.TIMEOUT); >> On 2014/10/15 21:39:19, Dan Beam wrote: >> >>> we should not have to do this >>> >> >> Acknowledged. >> >> https://codereview.chromium.org/601083004/diff/40001/ >> chrome/test/data/webui/print_preview.js#newcode439 >> chrome/test/data/webui/print_preview.js:439: testDone(); >> On 2014/10/15 21:37:31, Dan Beam wrote: >> >>> function PrintPreviewWebUIAsyncTest() {} >>> >> >> PrintPreviewWebUIAsyncTest.prototype = { >>> __proto__: PrintPreviewWebUITest, >>> >> >> /** @override */ >>> isAsync: true, >>> }; >>> >> >> Done. >> >> https://codereview.chromium.org/601083004/diff/40001/ >> chrome/test/data/webui/print_preview.js#newcode439 >> chrome/test/data/webui/print_preview.js:439: testDone(); >> On 2014/10/15 21:39:18, Dan Beam wrote: >> >>> On 2014/10/15 21:37:31, Dan Beam wrote: >>> > function PrintPreviewWebUIAsyncTest() {} >>> > >>> > PrintPreviewWebUIAsyncTest.prototype = { >>> > __proto__: PrintPreviewWebUITest, >>> > >>> > /** @override */ >>> > isAsync: true, >>> > }; >>> >> >> (so you don't have to put all these useless testDone() calls >>> >> everywhere) >> >> Acknowledged. >> >> https://codereview.chromium.org/601083004/ >> > > > > -- > Albert Bodenhamer | Software Engineer | abodenha@chromium. > <abodenha@google.com>org > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
hcarmona@chromium.org changed reviewers: + alekseys@chromium.org - abodenha@chromium.org
https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... File chrome/browser/resources/print_preview/previewarea/preview_area.css (right): https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/previewarea/preview_area.css:83: visibility: hidden !important; is this debug code? we try not to use !important whenever possible https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/previewarea/preview_area.css:84: } nit: no \n at end of file https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/previewarea/preview_area.js:520: // Hide all controls that will be overlapped when the overlay is visible. will overlap https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/previewarea/preview_area.js:531: }, seems like you could combine {show/hide}Overlay code, e.g. setOverlayVisible(visible) https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... File chrome/test/data/webui/print_preview.h (right): https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.h:19: class PrintPreviewWebUIAsyncTest : public PrintPreviewWebUITest { why can't we do the inheritance in JS only? https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:668: document.createElementNS('http://www.w3.org/1999/xhtml', 'style'); indent off https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:668: document.createElementNS('http://www.w3.org/1999/xhtml', 'style'); er, why not just document.createElement('style')? https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:676: var heads = document.getElementsByTagName('head'); we control the markup, so there should always be a <head>... document.getElementsByTagName('head').appendChild(noAnimationStyle); https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:677: if (heads.length) { no curlies https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:686: waitForUI: function() { can this be tearDown? (so this.waitForAnimationsToEnd() doesn't need to be called everywhere) https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:686: waitForUI: function() { waitForAnimationsToEnd() https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:688: document.addEventListener('webkitAnimationEnd', function finishTest(e) { remove "finishTest" https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:703: var moreSettingsDiv = $('more-settings'); no reason for "Div" in these names, also no reason to gather these variables before they're used (or at all, could probably just use $('id') inline everywhere) https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:734: otherOptionsDiv.querySelector('.header-footer-container'); ugh, this is a pervasive pattern apparently... https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:882: nit: remove \n
Uploaded new CL with changes based on Dan's feedback. https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... File chrome/browser/resources/print_preview/previewarea/preview_area.css (right): https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/previewarea/preview_area.css:83: visibility: hidden !important; On 2014/10/20 20:15:24, Dan Beam wrote: > is this debug code? we try not to use !important whenever possible Don't need "!important", removing it. This is not debug code. It is necessary because we can't reuse the "invisible" class as it can be overwritten and we don't want to interfere with other functionality when hiding elements with the overlay. The visibility of all elements should remain unchanged when the overlap is gone. https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/previewarea/preview_area.css:84: } On 2014/10/20 20:15:24, Dan Beam wrote: > nit: no \n at end of file Acknowledged. https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/previewarea/preview_area.js:520: // Hide all controls that will be overlapped when the overlay is visible. On 2014/10/20 20:15:24, Dan Beam wrote: > will overlap Acknowledged. https://codereview.chromium.org/601083004/diff/80001/chrome/browser/resources... chrome/browser/resources/print_preview/previewarea/preview_area.js:531: }, On 2014/10/20 20:15:24, Dan Beam wrote: > seems like you could combine {show/hide}Overlay code, e.g. > setOverlayVisible(visible) Acknowledged. https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... File chrome/test/data/webui/print_preview.h (right): https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.h:19: class PrintPreviewWebUIAsyncTest : public PrintPreviewWebUITest { On 2014/10/20 20:15:25, Dan Beam wrote: > why can't we do the inheritance in JS only? Don't know, but I was getting compile errors in the unit test without the change here. I will look into it. https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:668: document.createElementNS('http://www.w3.org/1999/xhtml', 'style'); On 2014/10/20 20:15:25, Dan Beam wrote: > indent off Acknowledged. https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:668: document.createElementNS('http://www.w3.org/1999/xhtml', 'style'); On 2014/10/20 20:15:25, Dan Beam wrote: > er, why not just document.createElement('style')? Changed to document.createElement('style') https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:676: var heads = document.getElementsByTagName('head'); On 2014/10/20 20:15:25, Dan Beam wrote: > we control the markup, so there should always be a <head>... > > document.getElementsByTagName('head').appendChild(noAnimationStyle); Acknowledged. https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:677: if (heads.length) { On 2014/10/20 20:15:25, Dan Beam wrote: > no curlies Acknowledged. https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:686: waitForUI: function() { On 2014/10/20 20:15:25, Dan Beam wrote: > waitForAnimationsToEnd() Acknowledged. https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:686: waitForUI: function() { On 2014/10/20 20:15:25, Dan Beam wrote: > can this be tearDown? (so this.waitForAnimationsToEnd() doesn't need to be > called everywhere) Can't be in tearDown because tearDown happens after we call testDone(); https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:688: document.addEventListener('webkitAnimationEnd', function finishTest(e) { On 2014/10/20 20:15:25, Dan Beam wrote: > remove "finishTest" Acknowledged. https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:703: var moreSettingsDiv = $('more-settings'); On 2014/10/20 20:15:25, Dan Beam wrote: > no reason for "Div" in these names, also no reason to gather these variables > before they're used (or at all, could probably just use $('id') inline > everywhere) Acknowledged. https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:734: otherOptionsDiv.querySelector('.header-footer-container'); On 2014/10/20 20:15:25, Dan Beam wrote: > ugh, this is a pervasive pattern apparently... Acknowledged. https://codereview.chromium.org/601083004/diff/80001/chrome/test/data/webui/p... chrome/test/data/webui/print_preview.js:882: On 2014/10/20 20:15:25, Dan Beam wrote: > nit: remove \n Acknowledged.
note: Acknowledged means "I get what you mean but didn't make substantial code changes" (or otherwise didn't address the issue). Done means "I fixed it". https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... chrome/browser/resources/print_preview/previewarea/preview_area.js:514: * Set the visibility of the message overlay. @param {boolean} visible Whether to make the overlay visible or not. https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... chrome/browser/resources/print_preview/previewarea/preview_area.js:528: visible); nit: marginControls[i].classList.toggle(PreviewArea.Classes_.OVERLAYED, visible); or marginControls[i].classList.toggle(PreviewArea.Classes_.OVERLAYED, visible); are both less lines (if they fit) https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:397: checkSectionVisible(otherOptions, true); nit: just inline, e.g. checkSectionVisible($('other-options-settings'), true); here and above https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:666: document.createElement('style'); unwrap (e.g. var noAnimationStyle = document.createElement('style');) https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:675: head.appendChild(noAnimationStyle); nit: document.querySelector('head').appendChild(noAnimationStyle); https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:707: checkElementDisplayed(fitToPage, false); lower var mediaSize = $('media-size-settings'); to here (first use) https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:730: otherOptions.querySelector('.header-footer-container'); can be 1 line
lgtm https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_list_item.html (right): https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list_item.html:3: <img class="destination-list-item-icon" alt=""> Shouldn't we add role="presentation" here too? https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... File chrome/browser/resources/print_preview/settings/destination_settings.html (right): https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... chrome/browser/resources/print_preview/settings/destination_settings.html:13: <img class="destination-settings-icon" alt=""> Shouldn't we add role="presentation" here too?
Replied to feedback and uploaded changes. https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... File chrome/browser/resources/print_preview/previewarea/preview_area.js (right): https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... chrome/browser/resources/print_preview/previewarea/preview_area.js:514: * Set the visibility of the message overlay. On 2014/10/21 01:16:31, Dan Beam wrote: > @param {boolean} visible Whether to make the overlay visible or not. Done. https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... chrome/browser/resources/print_preview/previewarea/preview_area.js:528: visible); On 2014/10/21 01:16:31, Dan Beam wrote: > nit: > > marginControls[i].classList.toggle(PreviewArea.Classes_.OVERLAYED, visible); > > or > > marginControls[i].classList.toggle(PreviewArea.Classes_.OVERLAYED, > visible); > > are both less lines (if they fit) Done. https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_list_item.html (right): https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list_item.html:3: <img class="destination-list-item-icon" alt=""> On 2014/10/21 01:58:36, Aleksey Shlyapnikov wrote: > Shouldn't we add > > role="presentation" > > here too? alt="" is equivalent to role="presentation". We could use either one, but I chose alt="" because it exists in more places in the code. https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... File chrome/browser/resources/print_preview/settings/destination_settings.html (right): https://codereview.chromium.org/601083004/diff/100001/chrome/browser/resource... chrome/browser/resources/print_preview/settings/destination_settings.html:13: <img class="destination-settings-icon" alt=""> On 2014/10/21 01:58:36, Aleksey Shlyapnikov wrote: > Shouldn't we add > > role="presentation" > > here too? Acknowledged. https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:397: checkSectionVisible(otherOptions, true); On 2014/10/21 01:16:31, Dan Beam wrote: > nit: just inline, e.g. > > checkSectionVisible($('other-options-settings'), true); > > here and above otherOptions is used multiple times. https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:666: document.createElement('style'); On 2014/10/21 01:16:31, Dan Beam wrote: > unwrap (e.g. var noAnimationStyle = document.createElement('style');) Done. https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:675: head.appendChild(noAnimationStyle); On 2014/10/21 01:16:31, Dan Beam wrote: > nit: document.querySelector('head').appendChild(noAnimationStyle); Done. https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:707: checkElementDisplayed(fitToPage, false); On 2014/10/21 01:16:31, Dan Beam wrote: > lower > > var mediaSize = $('media-size-settings'); > > to here (first use) The 3 variables otherOptions, fitToPage, and mediaSize are used together. I think it makes sense to declare all 3, then use all 3. I did move the moreSettings related things into a function to avoid code duplication. https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:730: otherOptions.querySelector('.header-footer-container'); On 2014/10/21 01:16:31, Dan Beam wrote: > can be 1 line Done.
lgtm https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/100001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:675: head.appendChild(noAnimationStyle); On 2014/10/21 21:32:37, Hector Carmona wrote: > On 2014/10/21 01:16:31, Dan Beam wrote: > > nit: document.querySelector('head').appendChild(noAnimationStyle); > > Done. note the querySelector() instead of getElementsByTagName()
what's holding this CL up?
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/601083004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
Trybots caught some flaky tests. I fixed the issue by making all tests async and waiting for the correct animation to finish. There were 5 tests that did not trigger animations and they call testDone(). The other tests need to wait for the animation to finish on specific elements. Since most tests are async there wasn't much benefit to sub-classing and and I made the original test asyc and restored the test order. https://codereview.chromium.org/601083004/diff/160001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/160001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:630: }); Re-ordered the tests to match their original order since the secondary class no longer exists. This will make it easier to track changes before this CL. https://codereview.chromium.org/601083004/diff/180001/chrome/browser/resource... File chrome/browser/resources/print_preview/settings/other_options_settings.html (right): https://codereview.chromium.org/601083004/diff/180001/chrome/browser/resource... chrome/browser/resources/print_preview/settings/other_options_settings.html:7: <div id="other-options-collapsible" class="collapsible"> Creating this DIV and giving it an ID makes it easier to test this UI. This div was previously created programatically in print_preview_animations.js. The code there already has a check to see if the div exists. https://codereview.chromium.org/601083004/diff/180001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/180001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:55: isAsync: true, Since only 5 tests don't need to wait on an animation, I got rid of the secondary class to keep the test simple. Having testDone() in the 5 tests that don't wait for an animation seems better than having the extra class. Thoughts on this?
lgtm https://codereview.chromium.org/601083004/diff/180001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/180001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:55: isAsync: true, On 2014/10/29 22:24:46, Hector Carmona wrote: > Since only 5 tests don't need to wait on an animation, I got rid of the > secondary class to keep the test simple. Having testDone() in the 5 tests that > don't wait for an animation seems better than having the extra class. Thoughts > on this? I'm ok with that.
lgtm https://codereview.chromium.org/601083004/diff/180001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/601083004/diff/180001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:55: isAsync: true, On 2014/10/29 22:24:46, Hector Carmona wrote: > Since only 5 tests don't need to wait on an animation, I got rid of the > secondary class to keep the test simple. Having testDone() in the 5 tests that > don't wait for an animation seems better than having the extra class. Thoughts > on this? that's fine
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/601083004/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8102b1b83fa3b33624a19bc89fd578566ffe573c Cr-Commit-Position: refs/heads/master@{#302105} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
