Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#33220 closed defect (bug) (fixed)

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

Reported by: mattwiebe Owned by: valendesigns
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: ui, javascript Cc:


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]

Attachments (10)

33220.diff (3.2 KB) - added by mattwiebe 6 years ago.
33220.2.diff (2.8 KB) - added by westonruter 6 years ago.
Additional change: https://github.com/xwp/wordpress-develop/commit/3eceff0472f6e1855208e308374fd0c4b0fbc31c
33220.3.diff (2.9 KB) - added by valendesigns 6 years ago.
33220.4.diff (3.3 KB) - added by westonruter 6 years ago.
Additional change: https://github.com/xwp/wordpress-develop/commit/9cf9c18df728f306a7a65e03068279276b508a57
33220.5.diff (3.6 KB) - added by westonruter 6 years ago.
Additional change: https://github.com/xwp/wordpress-develop/commit/e0e82b48ae3cb30714ffd11c60b64dae30172943
33220.6.diff (3.7 KB) - added by valendesigns 6 years ago.
33220.7.diff (3.7 KB) - added by mattwiebe 6 years ago.
33220.8.diff (5.6 KB) - added by valendesigns 6 years ago.
33220.9.diff (5.3 KB) - added by westonruter 6 years ago.
Additional change: https://github.com/xwp/wordpress-develop/commit/069c72cd919eb6fe29524095e805725bf189907e
33220.10.diff (5.9 KB) - added by ocean90 6 years ago.

Download all attachments as: .zip

Change History (47)

#1 @samuelsidler
6 years ago

  • Milestone changed from Awaiting Review to 4.3

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

#2 @valendesigns
6 years ago

  • Keywords needs-patch added

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.

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

6 years ago

#4 @obenland
6 years ago

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

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

6 years ago

#5 @mattwiebe
6 years ago

  • Keywords has-patch added; needs-patch removed

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 :)

#6 @westonruter
6 years ago

@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.

#7 @westonruter
6 years ago

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

@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?

#8 @valendesigns
6 years ago

Reviewing it now.

#9 @valendesigns
6 years ago

  • Keywords needs-patch added; has-patch removed

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.

#10 @valendesigns
6 years ago

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?

6 years ago

#11 @valendesigns
6 years ago

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?

#12 @westonruter
6 years ago

@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:


#13 @westonruter
6 years ago

  • Keywords has-patch added; needs-patch removed

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 );

#14 @westonruter
6 years ago

  • Keywords needs-patch added; has-patch removed

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:


#15 @westonruter
6 years ago

  • Keywords has-patch added; needs-patch removed

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?

#16 @valendesigns
6 years ago

  • Keywords needs-patch added; has-patch removed

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.

6 years ago

#17 @valendesigns
6 years ago

  • Keywords has-patch added; needs-patch removed

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.

6 years ago

#18 @mattwiebe
6 years ago

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.

#19 @westonruter
6 years ago

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

#20 @valendesigns
6 years ago

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

#21 @valendesigns
6 years ago

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.

#22 @valendesigns
6 years ago

  • Keywords needs-testing added

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.

Last edited 6 years ago by valendesigns (previous) (diff)

6 years ago

#23 @valendesigns
6 years ago

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

#24 @mattwiebe
6 years ago

Yep, .8 looks good here.

#25 @valendesigns
6 years ago

  • Keywords needs-testing removed

Thanks @mattwiebe for testing.

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

#26 @westonruter
6 years ago

  • Keywords commit added

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.

#27 @valendesigns
6 years ago

Thank you! Good naming update, as well.

#28 @obenland
6 years ago

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

6 years ago

#29 @ocean90
6 years ago

  • Focuses javascript added

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.

#30 @ocean90
6 years ago

#32662 was marked as a duplicate.

#31 @azaozz
6 years ago

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.

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

6 years ago

#33 @westonruter
6 years ago

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

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.

#34 @westonruter
6 years ago

Regression: #33428

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

6 years ago

#36 @samuelsidler
6 years ago

#33339 was marked as a duplicate.

#37 @westonruter
6 years ago

Another scenario needing a solution: #33567

Note: See TracTickets for help on using tickets.