1244235 - Add a test to ensure transition events work also in subdocuments

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: transitionend_in_iframe_test.diff

I was about to regress this behavior since there wasn't a testcase to check that
if we have display: none iframe, other iframes should be still processed normally.

If the test doesn't pass, it will timeout.

Comment 1

[Daniel Holbert [:dholbert]]

Comment on attachment 8713740 [details] [diff] [review]
transitionend_in_iframe_test.diff

Review of attachment 8713740 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_transitions_events.html
@@ +60,5 @@
>  SimpleTest.waitForExplicitFinish();
>  SimpleTest.requestFlakyTimeout("untriaged");
>  var gTestCount = 0;
>  function started_test() { ++gTestCount; }
> +function finished_test() { if (--gTestCount == 0) { runIframeTests(); } }

Could you just create a new dedicated mochitest for the stuff you're testing here?  The way you're kicking it off here (invoking your new test code instead of finish()) feels a bit shoehorned in.  And your new test code doesn't share any content/utility-code with the rest of this file.

(This test is already hard to follow & kinda inelegant -- due largely to all the async stuff it's needing to do.  But at least its cleanup/completion process is pretty clean -- when all tests are finished, we call finish.  But this patch muddies that currently-clean part, and makes the test as a whole even harder to follow than it already is.)

@@ +348,5 @@
>  is(e.isTrusted, false)
>  
>  document.body.dispatchEvent(e);
>  
> +function runIframeTests() {

Please add a comment somewhere explaining what this test code is actually testing.  e.g.:

  // Ensure that a 'display:none' iframe doesn't prevent us from sending
  // transitionend events from a (different) displayed iframe.

@@ +361,5 @@
> +
> +  visibleIframe.onload = function() {
> +    visibleIframe.contentDocument.body.firstElementChild.addEventListener("transitionend",
> +      function() { SimpleTest.finish(); });
> +    visibleIframe.contentDocument.body.firstElementChild.style.backgroundColor = "red";

Two things (both optional):

 (1) consider declaring a convenience variable e.g.
 var firstElem = visibleIframe.contentDocument.body.firstElementChild;
    to avoid repeating yourself and avoid making these lines super-long.

 (2) There might be a possibility for a race condition here, along the lines of bug 1034220. I can't remember if we're guaranteed that styles will have been flushed by the time 'onload' fires. But if it turns out they haven't been flushed, then this color-tweak won't trigger a transition.  If you like, you can *ensure* we've flushed style by querying getComputedStyle for firstElementChild.  But it doesn't look like we do this elsewhere in this file, so maybe it's not necessary... (hence, optional)

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8713740 [details] [diff] [review]
transitionend_in_iframe_test.diff

Review of attachment 8713740 [details] [diff] [review]:
-----------------------------------------------------------------

[Marking "r-" to clear this out of my review queue, because I think this would be best separated into its own file (per beginning of comment 1) and that merits another quick round of review when that's done.]

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(and I assigned to nobody since I don't think I will have time to spend on making testing setup better.)

Comment 4

[Daniel Holbert [:dholbert]]

(I was just imagining making a new file with e.g.
 perl testing/mochitest/gen_template.pl -type plain 
...and simply copypasting the exact same new code into that new file, instead of into an existing mochitest.

I don't think that should be too involved, but I'm sympathetic to time pressure. If you don't get to this, maybe I'll do so after getting through my current review backlog.)