Bug 167538

The images show that the test is expecting the cursor to be present in the text field, but it is missing.

Marked test as flaky in http://trac.webkit.org/projects/webkit/changeset/211994

This does not appear to be a recent regression. CC'ing test author.

It seems that the only issue is that the caret appears or not, the test is not checking anything regarding the caret. The only important thing was that with "autofocus" we were performing a simplified layout that was crashing before the patch that added the test (bug #163450). I'll modify the tests trying to get rid of this issue with the caret.

Created attachment 301146 [details] Patch Try to fix the test, let's see what EWS has to say.

Hm, I wonder if there is a more idiomatic way to force the cursor to not appear? Is the blur a pattern used by many tests to avoid this? When reading the test, I would be confused as to why the blur is present, since there's no comment. I'm kinda surprised that our test infrastructure doesn't have the ability to add two different expectations and allow the test to match either. Blinky cursors are a very common problem in automated tests and most test environments have a standard mechanism for dealing with them.

Comment on attachment 301146 [details] Patch I am tempted to mark this as reviewed, but I am wondering: Is this still an effective test? Would the call to blur have side-stepped the crash even before we fixed the bug?

Another possibility would be to enhance the test runners so that they understand the flashing caret and always take snapshots with the caret visible.

Or always with the caret invisible, or any other sensible rule.

Comment on attachment 301146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301146&action=review > LayoutTests/fast/css-grid-layout/grid-simplified-layout-positioned-expected.html:15 > <input autofocus> > +<script> > + document.querySelector("input").blur(); > +</script> Here in the expected file, maybe we can just omit autofocus instead of calling blur?

Comment on attachment 301146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301146&action=review > LayoutTests/ChangeLog:13 > + "autofocus" was needed to get the crash, so this change modifies > + the test to blur the element, so the caret is not painted anymore. autofocus was needed to get the crash.

(In reply to comment #11) > autofocus was needed to get the crash. The expected file doesn’t need to crash. That’s why I suggested omitting it *in the expected file*, not in the test file.

Created attachment 301338 [details] Patch for landing

Thanks for the reviews. Uploaded new version for landing adding a comment to explain the "blur()" and also removing "autofocus" on the "-expected" file. (In reply to comment #7) > I am tempted to mark this as reviewed, but I am wondering: Is this still an > effective test? Would the call to blur have side-stepped the crash even > before we fixed the bug? Yeah I've verified that the test is still effective with the blur() part, as the crash was triggered before.

Comment on attachment 301338 [details] Patch for landing Attachment 301338 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3077149 New failing tests: fast/css-grid-layout/grid-simplified-layout-positioned.html

Created attachment 301341 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 301338 [details] Patch for landing Attachment 301338 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3077154 New failing tests: fast/css-grid-layout/grid-simplified-layout-positioned.html

Created attachment 301342 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 301338 [details] Patch for landing Attachment 301338 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3077158 New failing tests: fast/css-grid-layout/grid-simplified-layout-positioned.html

Created attachment 301343 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Created attachment 301349 [details] New version with timeout

Oops, so in my previous version the "blur()" was run before "autofocus", so it was not useful at all. Both versions the test and the expected file were showing the caret (so probably it would be still flacky on Mac. New version uses setTimeout() on the test file to ensure that blur() is run after "autofocus". Would you mind to take a new look? Thanks.

Comment on attachment 301349 [details] New version with timeout Attachment 301349 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3077880 New failing tests: fast/css-grid-layout/grid-simplified-layout-positioned.html

Created attachment 301350 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 301349 [details] New version with timeout Attachment 301349 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3077922 New failing tests: fast/css-grid-layout/grid-simplified-layout-positioned.html

Created attachment 301354 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 301349 [details] New version with timeout Attachment 301349 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3077921 New failing tests: fast/css-grid-layout/grid-simplified-layout-positioned.html

Created attachment 301355 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 301349 [details] New version with timeout Oops so this is still not working on Mac. I'd need to take a deeper look when I find some time, removing from review queue at this point.

Created attachment 306784 [details] Patch for landing Fix test so it waits until the blur() happens, this makets it passes on Mac.

Comment on attachment 306784 [details] Patch for landing Clearing flags on attachment: 306784 Committed r215222: <http://trac.webkit.org/changeset/215222>

All reviewed patches have been landed. Closing bug.