Add a test to ensure transition events work also in subdocuments

NEW
Unassigned

Status

()

Core
CSS Parsing and Computation
2 years ago
2 years ago

People

(Reporter: smaug, Unassigned)

Tracking

36 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Created attachment 8713740 [details] [diff] [review]
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.
Attachment #8713740 - Flags: review?(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 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.]
Attachment #8713740 - Flags: review?(dholbert) → review-
(Reporter)

Updated

2 years ago
Assignee: bugs → nobody
(Reporter)

Comment 3

2 years ago
(and I assigned to nobody since I don't think I will have time to spend on making testing setup better.)
(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.)
You need to log in before you can comment on or make changes to this bug.