Bug 54634

Looks like this test still references media-file.js from its old location, probably because it was added the patch for r78778 was created (with http://trac.webkit.org/changeset/78680)

We are woking on the fix right now.

Looks like this has been caught and is being handled: https://bugs.webkit.org/show_bug.cgi?id=54631

For some reason, the test was skipped, once I unskipped it with http://trac.webkit.org/changeset/80293 it started to failed with text+image diffs: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20chromium.org&tests=media%2Fvideo-controls-in-media-document.html&showExpectations=true

It was skipped because of this change http://trac.webkit.org/changeset/78811/trunk/LayoutTests/platform/chromium/test_expectations.txt

(In reply to comment #5) > It was skipped because of this change > http://trac.webkit.org/changeset/78811/trunk/LayoutTests/platform/chromium/test_expectations.txt I think you mean http://trac.webkit.org/changeset/78809/trunk/LayoutTests/platform/chromium/test_expectations.txt, but yes. Mikhail, I don't think we normally want to skip tests that we want to eventually fix (unless they're impact tests that run after them), since otherwise it's hard to track changes in behavior (e.g. this test used to time out, but now it has a text/image diff).

Created attachment 85258 [details] Patch

> LayoutTests/media/video-controls-in-media-document.html:9 > + layoutTestController.waitUntilDone(); WebKit JavaScript generally uses 4 space indents, like the rest of WebKit code. > LayoutTests/media/video-controls-in-media-document.html:26 > + // the rendered video+controls but the controls are part of the shadow DOM and Dimitri might have ideas about getting at these dimensions in a test without having to use pixel output.

Created attachment 85334 [details] Patch

(In reply to comment #8) > > LayoutTests/media/video-controls-in-media-document.html:9 > > + layoutTestController.waitUntilDone(); > WebKit JavaScript generally uses 4 space indents, like the rest of WebKit code. Done. > > LayoutTests/media/video-controls-in-media-document.html:26 > > + // the rendered video+controls but the controls are part of the shadow DOM and > Dimitri might have ideas about getting at these dimensions in a test without having to use pixel output. That'd be awesome. I hate that this is a pixel test.

Comment on attachment 85334 [details] Patch Will r+/cq+ for now. The commit queue is backed up enough that if Dimitri has any ideas there'll be time to upload a new patch.

Comment on attachment 85334 [details] Patch Clearing flags on attachment: 85334 Committed r80820: <http://trac.webkit.org/changeset/80820>

All reviewed patches have been landed. Closing bug.

Even though my patch landed, this bug would need to stay open at the very least to track rebaselining chromium win & mac. Sadly, even chromium linux is only passing intermittently, because the controls fade-in effect is not reliably completing in under 100ms. At this point I'm inclined to simply delete the test and eat the resulting coverage loss unless we can come up with up with a way to test the total height of the video+controls or the placement of the controls in the shadow DOM. Anybody want to suggest / lobby for an alternative?

Reopening per previous comment.

(In reply to comment #14) > Even though my patch landed, this bug would need to stay open at the very least to track rebaselining chromium win & mac. > Sadly, even chromium linux is only passing intermittently, because the controls fade-in effect is not reliably completing in under 100ms. > > At this point I'm inclined to simply delete the test and eat the resulting coverage loss unless we can come up with up with a way to test the total height of the video+controls or the placement of the controls in the shadow DOM. > > Anybody want to suggest / lobby for an alternative? Being able to examine the shadow DOM from DRT seems like the best way to go.

(In reply to comment #16) > > Being able to examine the shadow DOM from DRT seems like the best way to go. This would also be useful for the (currently platform specific) tests that post mouse events to test the default media controls.

shadowRoot is now exposed to chromium DRT (in 56573). This bug (for chromium) is now blocked on media elements being converted to setShadowRoot (from setShadowHost), tracked by 53020. For non-chromium DRT, this bug is also blocked on 57415.

Created attachment 89028 [details] Test rewritten to use shadow DOM explicitly.

*** Bug 54757 has been marked as a duplicate of this bug. ***

Comment on attachment 89028 [details] Test rewritten to use shadow DOM explicitly. View in context: https://bugs.webkit.org/attachment.cgi?id=89028&action=review > LayoutTests/media/video-controls-in-media-document.html:15 > + var controls = layoutTestController.shadowRoot(video).firstChild.firstChild; > + testExpected(controls.offsetTop, 208); // == 240(video height) minus 32 (controls height). > + layoutTestController.notifyDone(); Hard coding the size of the controls doesn't seem like a great idea as ports are free to use different sizes. Because this tests is to make sure controls are rendered on top of the <video> element, why not just test that the top of the controls element is above the bottom of the video element? > LayoutTests/platform/mac/Skipped:2 > -# Copyright (C) 2008 Apple Inc. All rights reserved. > +# Copyright (C) 2011 Apple Inc. All rights reserved. An Apple notice copyright should include all years, eg. "2008, 2009, 2010, 2011".

Created attachment 89057 [details] Use shadow DOM and avoid hardcoding controls height.

(In reply to comment #21) > (From update of attachment 89028 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89028&action=review > > > LayoutTests/media/video-controls-in-media-document.html:15 > > + var controls = layoutTestController.shadowRoot(video).firstChild.firstChild; > > + testExpected(controls.offsetTop, 208); // == 240(video height) minus 32 (controls height). > > + layoutTestController.notifyDone(); > > Hard coding the size of the controls doesn't seem like a great idea as ports are free to use different sizes. Because this tests is to make sure controls are rendered on top of the <video> element, why not just test that the top of the controls element is above the bottom of the video element? Done. > > > LayoutTests/platform/mac/Skipped:2 > > -# Copyright (C) 2008 Apple Inc. All rights reserved. > > +# Copyright (C) 2011 Apple Inc. All rights reserved. > > An Apple notice copyright should include all years, eg. "2008, 2009, 2010, 2011". Left alone (2008) to avoid rocking the boat (this was an auto-edit; will config emacs to leave webkit copyrights alone).

Comment on attachment 89057 [details] Use shadow DOM and avoid hardcoding controls height. View in context: https://bugs.webkit.org/attachment.cgi?id=89057&action=review > LayoutTests/media/video-controls-in-media-document.html:15 > + var controls = layoutTestController.shadowRoot(video).firstChild.firstChild; > + test(controls.offsetTop < 240); > + layoutTestController.notifyDone(); This doesn't hard code the control height, so why use "240" for the video height? Maybe I am being pedantic because we *do* know the video dimensions, but is there any reason to not calculate the height? Also I think it is good to structure a "test(...)" so the output shows what is being tested instead of just the result. For example: testExpected("controls.offsetTop", video.offsetHeight, "<"); will generate: EXPECTED (video.offsetTop < '240') OK This can be very helpful to someone looking at a test's results later, as they don't have to view source to figure out what failed.

Created attachment 89069 [details] Use shadow DOM and avoid hardcoding controls height.

(In reply to comment #24) > This doesn't hard code the control height, so why use "240" for the video height? Maybe I am being pedantic because we *do* know the video dimensions, but is there any reason to not calculate the height? My motivation is that I was worried that video.offsetHeight might end up including the height of the controls and I wanted to avoid having to worry about finding the canvas part of the element, so chose readability. But it seems like that worry was unfounded, so changed. > Also I think it is good to structure a "test(...)" so the output shows what is being tested instead of just the result. Agreed. > For example: > testExpected("controls.offsetTop", video.offsetHeight, "<"); > will generate: > EXPECTED (video.offsetTop < '240') OK In fact it generates: ReferenceError: controls is not defined :) I didn't think about it too much before and just changed to test(), but I can regain the kind of message you want by dropping the "var" to bleed |controls| out of function scope. Done.

Comment on attachment 89069 [details] Use shadow DOM and avoid hardcoding controls height. View in context: https://bugs.webkit.org/attachment.cgi?id=89069&action=review Thanks! > LayoutTests/media/video-controls-in-media-document.html:14 > + testExpected("controls.offsetTop + controls.offsetHeight", video.offsetHeight, "<="); Much better than what I suggested!

Comment on attachment 89069 [details] Use shadow DOM and avoid hardcoding controls height. Rejecting attachment 89069 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2 Last 500 characters of output: va/lc3/JavaObject ..................................... java/lc3/StringMethods . java/lc3/forin . java/lc3/instanceof . jquery .......... loader .. mathml ........ mathml/presentation .................. media ................................................ media/video-controls-in-media-document.html -> failed Exiting early after 1 failures. 13389 tests run. 386.18s total testing time 13388 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 10 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8390101

Help? I don't see what the failure is. I suspect the test runner didn't process the expectation rm's correctly but I can't see any of the actual/expected/diff from the URL given (http://queues.webkit.org/results/8390101). Eric - do you see what went wrong?

Unfortunately the cq doesn't know how to upload failure results yet. :( I expect the test simply fails on Mac Snow Leopard.

Created attachment 89283 [details] Now passes on OS/X webkit, too.

As Eric S. guessed, CQ failure was legit. Updated patch fixes that, by removing the bogus bottom padding from mediaControls{,Efl,QuickTime}.css. I say "bogus" b/c it makes the media experience difference in media documents and in <video>-using HTML, and because it makes difficult to reason about the rendered height of the media document (which can be relevant e.g. when using iframes). This patch makes the media document case consistent with the non-media-document case. Hopefully it doesn't make a whole bunch of (unrelated) layouttests fail.

(In reply to comment #32) > As Eric S. guessed, CQ failure was legit. Updated patch fixes that, by removing the bogus bottom padding from mediaControls{,Efl,QuickTime}.css. > I say "bogus" b/c it makes the media experience difference in media documents and in <video>-using HTML, and because it makes difficult to reason about the rendered height of the media document (which can be relevant e.g. when using iframes). This patch makes the media document case consistent with the non-media-document case. > Hopefully it doesn't make a whole bunch of (unrelated) layouttests fail. Avi, there isn't a Sources/WebCore/ChangeLog entry explaining the CSS changes. Those changes are also unrelated to this bug, so if you feel they are necessary, please file a new bug to that effect, and make the CSS changes in that new bug.

(In reply to comment #33) > there isn't a Sources/WebCore/ChangeLog entry explaining the CSS changes. Those changes are also unrelated to this bug, so if you feel they are necessary, please file a new bug to that effect, and make the CSS changes in that new bug. ChangeLog was missing b/c I assumed webkit-patch would complain about needed updates (it seems it stays quiet as long as there's *some* ChangeLog involved in the diff, even if not all dirs are covered). Just-css & changelog patch uploaded to new bug 58442 which this one is now blocked on. Will upload updated test-only patch (without css changes) to this bug in a moment.

Created attachment 89379 [details] CSS changes removed.

Cleared cq+ at Ami's request, this won't work until https://bugs.webkit.org/show_bug.cgi?id=58442 has landed.

(In reply to comment #36) > Cleared cq+ at Ami's request, this won't work until https://bugs.webkit.org/show_bug.cgi?id=58442 has landed. 58442 has landed, so this patch is now open for cq+ business again.

Comment on attachment 89379 [details] CSS changes removed. Clearing flags on attachment: 89379 Committed r83947: <http://trac.webkit.org/changeset/83947>

All reviewed patches have been landed. Closing bug.

This test is timing out on Windows: http://build.webkit.org/results/Windows%20XP%20Debug%20%28Tests%29/r83947%20(27652)/media/video-controls-in-media-document-pretty-diff.html