Customizer: layout issues with autofocus'd sections on small screens

[mattwiebe]

The short version: the autofocus-ed section starts its layout too high. Interestingly, the margin-top and height values are the same when opened later without autofocus (which works fine) as compared with autofocus. Fun times!

iOS SImulator, 4s:

Chrome emulator, 420x320:

This can be an enormous problem since the back button is hidden in some cases.

My hunch is that the layout should be defined in CSS rather than calculated in JS since we're not actually dealing with a lot of variables here, and we know these variables.

Tested against trunk @ [33493]

[samuelsidler]

We should investigate this for 4.3 and possibly pick it up before final.

[valendesigns]

I don't believe this is device or autofocus specific. The issue, as far as I can tell, is that there's code which calculates the distance of the heading from the top and due to a race condition or some other unknown issue the top is calculated incorrectly. I've experience it on multiple devices and screens resolutions by navigating to a section in the Customizer before it has fully loaded. Once it fully loads the position becomes incorrect and hidden behind the Customizer actions.

[obenland]

@celloexpressions, @westonruter could you see if you can come up with a patch here?

[mattwiebe]

33220.diff fixes both the autofocus issue and also adds a window resize handler, since the explicit height setting needs to respond to the different layout contexts it may find itself in.

This all feels a bit inelegant, but it works :)

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/3eceff0472f6e1855208e308374fd0c4b0fbc31c

[westonruter]

@mattwiebe: Thanks! I can confirm this is working. In 33220.2.diff​ I removed the newly-introduced noScroll param to focus() in favor of just fixing the behavior of focusing on an element, which was discovered in #33258 (:focusable seems to not be working right in jQuery UI). With a fixed focus() method this also means we can get rid of scrollIntoView() because focusing on an element will bring it into view automatically.

[westonruter]

@valendesigns, @celloexpressions: Thoughts on the patch given you both were were heavily involved with #31336 which changed the behavior of the sections to act like panels?

[valendesigns]

Reviewing it now.

[valendesigns]

I can verify it fixes autoload, but the issue when you navigate into the Customizer panels and sections before the frame is loaded still exists.

I tried creating a patch but I can't seem to figure it out. The issue is that everything works when you are navigating around but the second the frame loads the open section jumps down 42px.

This part of the issue existed before the patch. Basically, it feels like there are two events that set the height, one on click and one after the frame loads. Unfortunately, the math is wrong on the second event because of a missing section in the scroll height or something to that effect. I'm having a hard time pinpointing exactly where the bug is coming from.

I can look at this again tonight when I wake up but I have to get some sleep. If someone wants to jump in feel free or leave feedback and I'll take another legit stab at it in 10 hours or so with a fresh set of eyes.

[valendesigns]

So I've got a bit more information to share. It seems this issue only happen to panels or sections that come after a panel or section which is created dynamically through context and is not visible in time for the calculation so the parent UL has an incorrect negative margin by a factor of 42px for each hidden panel or section that came above it in the UI.

I believe we need to create a callback that updates the parent UL's negative top margin right after the context inserts the dynamic items into the UI. Anyone have suggestions where to add that?

Also, it would make sense for consistency to dynamically get the header actions height like we do in other place, instead of hardcoding 45. So this:

content.css( 'margin-top', ( 45 - position - scroll ) );

Becomes this:

content.css( 'margin-top', ( $( '#customize-header-actions' ).height() - position - scroll ) );

As well, it seems like we could move that code into the sizing function instead of in the expand function. Thoughts?

[valendesigns]

I've uploaded a patch that changes the items I mentioned above. However, I'm not sure how to fix the contextual items becoming visible too late. Does anyone have suggestions on how to fix this knowing what is actually happening now?

[westonruter]

@valendesigns: If we make the contextually-active panels/sections expand with a duration of 0 as opposed to having a slideDown animation, this would probably do it.

Side tip: I've found the easiest way to reproduce the issue by autofocusing on a section that appears after the Widgets panel, e.g. the Static Front Page section: /wp-admin/customize.php?autofocus[section]=static_front_page:

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/9cf9c18df728f306a7a65e03068279276b508a57

[westonruter]

OK, 33220.4.diff​ implements a change which ensures that setting the active state for a panel/section/control when the preview iframe is loading will result in a visibility toggle with a 0 duration. This ensures that the height can be calculated properly when the autofocusing kicks in. I'm not sure why autofocusing wasn't using this activate() method to begin with, as I recall this was the very scenario for why onChangeActive was calculating the duration as such in the first place:

duration = ( 'resolved' === api.previewer.deferred.active.state() ? args.duration : 0 );

[westonruter]

Shoot. The latest patch works for me on desktop but then on mobile it looks worse. The top negative margin is too great: it is -429px when it should be -329px:

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/e0e82b48ae3cb30714ffd11c60b64dae30172943

[westonruter]

There, I think I got it: 33220.5.diff​. Now that sections act like panels, the problem was when autofocusing on a section it would erroneously try to focus on the link in the overall li for the section, as opposed to looking for the first focusable element in the contents of the section (e.g. the ul).

@mattwiebe: Does the latest patch still fix the issues you originally identified and patched for.
@valendesigns: Anything else we're missing?

[valendesigns]

Patch 5 works for autofocus but does not for navigating with the mouse. It is not reliable to test with just autofocus because it happens after the Customizer loads and therefore will have the correct heights to calculate as the contextual items are already loaded in the UI. This is not the case when you use the mouse.

[valendesigns]

This latest patch binds to pane-contents-reflowed to execute sizing when the frame is done loading, which fixes the issue in both autofocus and when you're navigating really quickly before the contextual items are added to the UI. Please test and verify.

[mattwiebe]

33220.6 produced erratic results on resize, with the section often being pushed way down. This happens because margin-top should not be in the sizing() function, it only gets good results the first time. Moving the setting ofmargin-top into expand() so it only happens once produces good results.

33220.7.diff attached to do that.

[westonruter]

@valendesigns: Does patch 7 improve the jumping behavior you noticed? Does it still pass your tests?

[valendesigns]

I'll test it in around 20 minutes, just starting my day.

[valendesigns]

Unfortunately, the last patch does not work. I'll see if I can fix the jumpy behavior but margin-top needs to be in the sizing function for this to solve both scenarios.

[valendesigns]

I'm happy to say that I believe it's finally fixed. Patch 33220.8.diff​ has consistency for both autofocus and early loading without the jumpy behavior during resize. Please test.

[valendesigns]

@mattwiebe Could you please verify that the latest patch is working for you?

[mattwiebe]

Yep, .8 looks good here.

[valendesigns]

Thanks @mattwiebe for testing.

@westonruter Could you look at the latest patch and verify it's ready for commit?

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/069c72cd919eb6fe29524095e805725bf189907e

[westonruter]

Looks and feels good. In 33220.9.diff​ I just refreshed the batch by merging in some code that already landed into trunk, and I improved some variable naming.

[valendesigns]

Thank you! Good naming update, as well.

[obenland]

I know @ocean90 is reviewing this - @azaozz or @helen, could one of you do a lead review?

[ocean90]

33220.9.diff​ looks good. Tested in Chrome, Firefox, and Mobile Safari on an iPhone 5.

33220.10.diff​ includes the revert of [33157] which now no longer needed.

[ocean90]

#32662 was marked as a duplicate.

[azaozz]

33220.10.diff seems to fix the reported problem.

Testing on Android 5.1 phone was quite hard. The Static Front Page section worked well however both Widgets and Menus had glitches and were hard to use as drag & drop works on Android. If Menus were used first (with drag & drop), "Static Front Page" will still be missing the top part and have a lot of empty space at the bottom. Disabling jquery.ui.touch-punch.js makes all work quite better.

[westonruter]

In 33610:

Customize: Fix layout issues in panels and sections when resizing and autofocusing.

Also reverts [33157] which is no longer needed.

Props valendesigns, mattwiebe, westonruter, ocean90.
Fixes #33220.

[westonruter]

Regression: #33428

[samuelsidler]

#33339 was marked as a duplicate.

[westonruter]

Another scenario needing a solution: #33567