Make WordPress Core

Opened 9 years ago

Closed 8 years ago

#34344 closed defect (bug) (fixed)

Expanded section margin-top glitches when other section is deactivated

Reported by: westonruter's profile westonruter Owned by: ryankienstra's profile ryankienstra
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.2
Component: Customize Keywords: has-patch
Focuses: javascript Cc:

Description

After having expanded a section, if you then deactivate another section, the margin-top for the expanded section causes the contents to go out of view. Additionally, deactivated section is incorrectly animated when it should just collapse instantly since it is not in view.

To reproduce the problem in Twenty Fifteen, execute the following from the console:

wp.customize.section('header_image').expand( { completeCallback: function() { 
        _.delay( function(){ wp.customize.section('colors').active(false); }, 1000 ); 
} } );

Video depicting the above behavior: https://cloudup.com/cP5JgMgkqFu

Relates to #33567.

Attachments (4)

34344.diff (2.0 KB) - added by westonruter 9 years ago.
section-margin-top-problem.png (77.5 KB) - added by westonruter 9 years ago.
bug.png (125.6 KB) - added by megathemes 9 years ago.
overflow
800bf8.diff (616 bytes) - added by ryankienstra 9 years ago.

Download all attachments as: .zip

Change History (24)

@westonruter
9 years ago

#1 @westonruter
9 years ago

[attachent:34344.diff] is a work-in-progress. It eliminates the deactivation animation of the non-visible section, but there is still an issue (shown in section-margin-top-problem.png) where the margin-top of the section's contents still gets erroneously shifted up ~45px. Help wanted.

@megathemes
9 years ago

overflow

#2 @megathemes
9 years ago

Hey,

I found another issue with margin-top overflow,

  1. Drop a widget into an area
  2. Quickly go up level before widget populates into preview window (before ajax has finished i guess?)
  3. Come back into the widgets area
  4. margin-top starts from the top, as if it were top: 0 and ignores the saving box area

attached image above

Last edited 9 years ago by megathemes (previous) (diff)

#3 @westonruter
9 years ago

See also #34391 for a potential definitive solution to all of the margin-top glitches.

#4 @westonruter
9 years ago

  • Milestone changed from Future Release to 4.5

#5 @kirasong
9 years ago

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

Assigning to @westonruter for 4.5 tracking, since we're almost halfway through beta.

#6 @westonruter
9 years ago

  • Owner changed from westonruter to ryankienstra

@ryankienstra @delawski This is closely related to what you've been working on for #35947.

In fact, I would have thought that @ryankienstra's PR would have fixed the issue as it describes:

This occurs when there are many panels, using the code provided in the ticket to produce the panels. The non-active sections and panels still occupied space in the DOM. So the active panel had a margin-top property to move it to the top. This PR sets the height of the non-active sections to 0. And it doesn't add a margin-top value to the active panel's content.

Nevertheless, the issue also happens with 334473c checked out.

Your experience here is appreciated. If it's not something that can be figured out over the next several days, this is probably something that we'll need to punt for the next release.

#7 @westonruter
9 years ago

@ryankienstra @delawski I suppose the setting the height of inactive sections to zero is irrelevant because what this involves is an active section. So then the solution it seems would be, still, to either eliminate the use of margin-top to position the panels (#34391)?

@ryankienstra
9 years ago

#8 @ryankienstra
9 years ago

  • Keywords has-patch added; needs-patch removed

Possible Solution

Hi @westonruter,
Could you please see my pull request and identical patch?

They set the height to 0 for all sections in the root panel that aren't open.

Reason
Like we saw in trac-35947, non-open sections still took up space in the DOM. And they caused problems with the calculation of the margin-top value.

For example, this ticket gives code to reproduce the bug:

wp.customize.section('header_image').expand( { completeCallback: function() { 
         _.delay( function(){ wp.customize.section('colors').active(false); }, 1000 ); 
} } );

This opens the header_image section, and de-activates the colors section. But the colors section had a height of 43px. So when it's de-activated, it is given display: none. And it's taken out of the DOM.

But the margin-top of the header_image content area was already set at -259px. So when the colors section moves out of the DOM, the header_image section moves too high.

This is similar to @delawski's commit for trac-35947.

#9 @westonruter
9 years ago

@ryankienstra I've cherry-picked your patch to amend it onto the patch for #35947 so that they can more easily be tested together.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


9 years ago

#11 @ryankienstra
9 years ago

Status Update
Now Attached To trac-35947

As @westonruter suggested, this defect should be grouped with a related defect:

trac-35947 : Customizer panel fails to fully expand leaving extra margin

@westonruter cherry-picked this ticket's commit into that ticket's pull request. It looks like trac-35947 is almost ready to be merged into Beta, pending a minor issue.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


9 years ago

#13 @chriscct7
9 years ago

@delawski + @westonruter is this going to be ready in time or should the ticket be punted out?

#14 @westonruter
9 years ago

  • Keywords 4.6-early added
  • Milestone changed from 4.5 to Future Release

As with #35947, it looks like we'll need to re-visit this early 4.6.

#15 @westonruter
9 years ago

  • Milestone changed from Future Release to 4.6

This ticket was mentioned in Slack in #core by westonruter. View the logs.


9 years ago

#17 @celloexpressions
9 years ago

  • Keywords 4.6-early removed
  • Milestone changed from 4.6 to Future Release

Punting with #34391. These bugs could still get a fix during beta, but I'd prefer for efforts to focus on a broader rethinking via #34391 and getting that ready for early 4.7.

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


8 years ago

#19 @westonruter
8 years ago

  • Milestone changed from Future Release to 4.7

#20 @westonruter
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 38648:

Customize: Re-architect and harden panel/section UI logic.

Removes contents for sections and panels from being logically nested (in the DOM) in order to eliminate many issues related to using margin-top hacks. The element containing the link to expand the content element for panels and sections is now a sibling element to its content element: the content is removed from being nested at initialization. The content element is now available in a contentContainer property whereas the head element (containing the link to open the construct) is in a headContainer property. The existing container property is now a jQuery collection that contains both of these elements. Since the head element is no longer in an ancestor element to the content element, the aria-owns property is now used to maintain the relationship between the headContainer and the contentContainer. These changes are also accompanied by an improvement to the animation performance for the sliding panes.

Props delawski, celloexpressions.
Fixes #34391.
Fixes #34344.
Fixes #35947.

Note: See TracTickets for help on using tickets.