Make WordPress Core

Opened 9 years ago

Last modified 2 years ago

#32577 reviewing defect (bug)

Customizer QUnit tests not cleaning up

Reported by: iseulde's profile iseulde Owned by: dlh's profile dlh
Milestone: Future Release Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch needs-refresh
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 9 years ago.
32577.patch (2.3 KB) - added by dlh 9 years ago.
32577.2.patch (2.3 KB) - added by dlh 9 years ago.
WordPress_QUnit_Test_Suite.png (450.2 KB) - added by westonruter 8 years ago.
32577.3.patch (2.0 KB) - added by dlh 8 years ago.

Download all attachments as: .zip

Change History (18)

#1 @iseulde
9 years ago

In 32701:

Customizer: hide controls on test page

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

#2 @valendesigns
9 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
9 years ago

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

#4 @valendesigns
9 years ago

  • Milestone changed from 4.3 to Future Release

#5 @westonruter
9 years ago

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

@dlh
9 years ago

#6 @dlh
9 years 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
9 years ago

#7 @dlh
9 years 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.


8 years ago

#9 @celloexpressions
8 years ago

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

#10 @westonruter
8 years 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
8 years 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
8 years ago

#12 @dlh
8 years 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.

#13 @desrosj
2 years ago

  • Keywords needs-refresh added
  • Milestone set to Future Release

@dlh is this still occurring today? 32577.3.patch is also stale and needs a refresh if the answer is yes.

Note: See TracTickets for help on using tickets.