WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 12 months ago

#32577 reviewing defect (bug)

Customizer QUnit tests not cleaning up

Reported by: iseulde Owned by: dlh
Milestone: Future Release Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch
Focuses: javascript Cc:

Description

Ideally everything should be removed from the DOM after the test or module is done.
For now we can hide it to the left instead of top, it just moves down again as more tests are added.

Attachments (5)

Screen Shot 2015-06-06 at 16.17.31.png (229.6 KB) - added by iseulde 2 years ago.
32577.patch (2.3 KB) - added by dlh 18 months ago.
32577.2.patch (2.3 KB) - added by dlh 18 months ago.
WordPress_QUnit_Test_Suite.png (450.2 KB) - added by westonruter 13 months ago.
32577.3.patch (2.0 KB) - added by dlh 12 months ago.

Download all attachments as: .zip

Change History (17)

#1 @iseulde
2 years ago

In 32701:

Customizer: hide controls on test page

Introduced in [30919].
See #32577 and #30701.

#2 @valendesigns
2 years ago

  • Owner set to valendesigns
  • Status changed from new to assigned

I can take a look tomorrow. I need to get more familiar with the QUnit tests anyhow.

#3 @valendesigns
2 years ago

  • Milestone changed from Future Release to 4.3
  • Version set to trunk

#4 @valendesigns
2 years ago

  • Milestone changed from 4.3 to Future Release

#5 @westonruter
23 months ago

  • Owner changed from valendesigns to dlh
  • Summary changed from Customizer tests not cleaning up to Customizer QUnit tests not cleaning up

@dlh
18 months ago

#6 @dlh
18 months ago

  • Keywords has-patch added; needs-patch removed

32577.patch attempts to remove the problematic controls after their respective modules complete.

I would appreciate feedback on what the desired visual experience is when viewing the test suite. The patch reverts to the positioning that was there before [32701]. In my testing, this results in the controls being briefly visible when the suite loads. top: -10000 removes the flash; removing the .css() line altogether causes the controls to move down as @iseulde described, but eventually they disappear.

@dlh
18 months ago

#7 @dlh
18 months ago

Sorry: 32577.2.patch contains the pre-[32701] positioning. I left a 0 in the first one.

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


13 months ago

#9 @celloexpressions
13 months ago

  • Owner changed from dlh to westonruter
  • Status changed from assigned to reviewing

#10 @westonruter
13 months ago

  • Keywords needs-patch added; has-patch removed
  • Owner changed from westonruter to dlh

@dlh I tried 32577.2.patch and with the removal of left I see the fixture UI showing up still (see screenshot above). Restoring the negative left causes it to go away. Idea: what if the #customize-theme-controls element had a fixed 1px square viewport with the overflow hidden? Then we wouldn't have to worry about the size of the elements in the DOM. So something like:

    position: absolute;
    top: -10px;
    left: -10px;
    width: 1px;
    height: 1px;
    overflow: hidden;

#11 @dlh
13 months ago

@westonruter Thanks for reviewing the patch. I see the same issue with it as in your screenshot. I'll try to figure out what happened and test your CSS idea, too.

@dlh
12 months ago

#12 @dlh
12 months ago

  • Keywords has-patch added; needs-patch removed

32577.3.patch implements the CSS approach from 32577#comment:10, which seems pretty straightforward and works as expected in my testing.

The patch also attempts to supplement the CSS with a different method of cleaning up the DOM: Rather than attempting to pick off individual elements -- some of which appear to be created separately from of the tests themselves, if I followed correctly? -- it removes all of them at the end of the test suite, with one exception.

Of course, this approach still doesn't meet the goal of removing fixtures after each module or test. Plus, once you get to QUnit.done(), using $( '#customize-theme-controls' ).remove() would accomplish the same thing.

Note: See TracTickets for help on using tickets.