Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trac 35947 : Customizer panel fails to fully expand leaving extra margin #145

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@kienstra
Copy link

commented Mar 4, 2016

@westonruter,
This pull request for trac-35497 removes extra space at the bottom of an active Customizer panel. As the ticket notes, there's "unnecessary spacing when you go inside a panel."

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.

I tested this with the test code on the ticket on the following themes. There was no longer a space at the bottom of the panels. I couldn't find any side-effects.
Graphene
Heuman
Customizr
Twenty Fifteen
Twenty Sixteen

kienstra added some commits Mar 4, 2016

Customize : Remove extra space at bottom of Customizer panel.
Before, 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.
So set the height of the non-active sections to 0.
And don't add a margin-top value to the active panel's content.

Fixes #35947.
Customize : Remove variables that were deleted in commit 8894171.
These are no longer used in the function in which they're defined.
They were deleted as part of a commit to remove the 'margin-top' value.

Fixes #35947.
@westonruter

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2016

Woah. Hey, @delawski, thoughts?

@delawski

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2016

Hey, @westonruter. I took a look at the code. I'm a bit concerned about removing the code responsible for repositioning panels with margin-top. It might not be very safe in current panel/section implementation, so I have to check it locally. I'll checkout the branch tomorrow and do few tests. I'll be able to tell something more then.

kienstra and others added some commits Mar 5, 2016

Fix issue with widget's contents being too high on clicking in preview.
When clicking a widget in the preview, the 'onChangeExpanded' method is called.
This included a jQuery method that adds a 'margin-top' value.
Remove this method call, as it caused a spacing issue.
The inactive panels no longer occupy space, from commit 8894171.
So there's no adjustment needed for the controls to appear properly.

Fixes #34747.
Piotr Delawski
Piotr Delawski
Merge branch 'master' into trac-35947
* master:
  Posts, Post Types: Ensure that non-ASCII characters in attachment slugs aren't shown in urlencoded form in the sample permalink UI.
  Menus: Ensure theme location setting data is saved with a large menu.
  Customize: Eliminate unnecessary `WP_Customize_Site_Logo_Control` in favor of re-using `WP_Customize_Image_Control`.
  I18N: Fix an invalid placeholder added in [36844].
  TinyMCE, inline link: add styling for the dialog and UI Autocomplete to Press This.
  Pres This: - Change the newly added `press_this_save_post_content` filter to `press_this_save_post` and pass the $post_data array to it. - Remove the newly added `press_this_useful_html_elements`. It only runs in compatibility mode when a URL is typed by the user. - Remove the `press_this_suggested_content` filter. It is redundant as the suggested HTML for the editor is already filtered by `press_this_suggested_html`. - Add some more inline docs and rename couple of vars to make the code more readable.
  Docs: Improve DocBlock syntax and add a missing `@return` notation for `WP_Image_Editor_Imagick::strip_meta()`, introduced in [36700].
  Docs: Improve the hook doc summary for the `image_strip_meta` filter, introduced in [36700].
  I18N: Move the `aria-label` text in `get_theme_update_available()` to a separate string for easier translation.
  I18N: Move the `aria-label` text in `wp_plugin_update_row()` and `wp_theme_update_row()` to a separate string for easier translation.
  Comment out some CSS files in `$_old_files` that were added back as a result of [36341].
  Docs: Improve syntax and correct documentation throughout a variety of methods in `WP_Customize_Widgets`.
  Docs: Improve documentation for `WP_Customize_Widgets::customize_dynamic_partial_args()`, introduced in [36586].
@delawski

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2016

Different approach proposal

I have pushed my proposal for the fix to the issue. Instead of removing _recalculateTopMargin() which is risky in current sliding panels implementation and may cause new issues, I have delayed focus event on back button.

While working on #34391, I've noticed that focusing on element during CSS transition may cause unexpected results, very similar to the one presented. In case of #34391 I have implemented full solution taking advantage of transitionEnd event listening. In case of this ticket, I have used simpler solution - focus() is delayed by 180 ms which is the time needed for the transition to end.

I've tested the fix on Chrome/OS X, Firefox/OS X, Firefox/Windows 8.1, Firefox/Windows 7, IE11/Windows 7 (Twenty Sixteen in each case).

@kienstra, @westonruter: it'd be great if you could double check the fix works for you as expected. As a side note: I was able to reproduce the original issue only in Firefox.

@delawski

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2016

I have created new branch and PR #146 so that the patch file doesn't contain previous commits and their reverts.

@westonruter

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2016

@delawski It is fine, even preferred, to have a PR include all of the activity and all commits here because the PR isn't actually merged. Having all of the commits shows all of the authors and makes it easier to give props. So I closed #146 in favor of keeping this PR.

@kienstra

This comment has been minimized.

Copy link
Author

commented Mar 7, 2016

Tested Piotr's Commit
Works As Expected

As noted on trac-35947, @delawski's latest commit fixes the vertical spacing issue (screenshot). This issue was originally shown in these screenshots.

Customize : Set height to 0 for all sections in root panel that aren'…
…t open.

Before, non-open sections still took up space in the DOM.
And they caused problems with the calculation of the margin-top value.
So set height to 0 for all sections, and auto for open section.

Fixes #34344.

Piotr Delawski added some commits Mar 10, 2016

Piotr Delawski
Fix `focus()` inside widget customize control (try to focus on form e…
…lements first, then on anchors and other elements).
Piotr Delawski
Try to refocus with delay on nav control in case no element was :visi…
…ble initially (edge-case scenario; see #35947).
Piotr Delawski
Merge branch 'master' into trac-35947
* master: (184 commits)
  Dashicons: Fix incorrect ID in SVG version of font.
  Dashicons: Update to the latest files.
  Media: fix erroneously inserting a rel attribute in `get_image_send_to_editor()`. Reverts most of [34259] and [34260] and adds a unit test.
  Responsive Images: the `src` of the image has to be first in the `srcset`, because of a bug in iOS8. Update the unit tests to reflect the changes.
  Twenty Thirteen, Twenty Fourteen, Twenty Fifteen: Update screenshots to 1200 x 900 Fixes #34806 Props: iamtakashi
  Docs: Document default `WP_Ajax_Response::add()` arguments as a hash notation.
  REST API: Add `home_url` to API index to avoid confusion with `site_url`.
  Docs: Improve 4.5 changelog entries introduced in [36992] for `wp_authenticate()`, and the `authenticate` and `wp_login_failed` hooks.
  Emoji: The `everythingExceptFlag` browser support flag, introduced in [36816], wasn't being initialised correctly.
  Emoji: Fix the diversity emoji check in Safari.
  Post 4.5-beta4 version bump.
  4.5-beta4
  Docs: Clarify documentation for the `xmlrpc_enabled` filter to better explain that its scope only extends to methods requiring authentication.
  TinyMCE: Adjust textpattern tests for the changes in [37023].
  TinyMCE: after discussion in Slack https://wordpress.slack.com/archives/core/p1458164584000700 - Remove `***` and `___` text pattern and support for spaces in `---`. The only `<hr>` text pattern is 3 or more dashes, no spaces. - Remove the `*`, `**`, `_`, and `__` text patterns for bold and italic.
  Media: When generating the base URL to be used in the `srcset` attribute, use an `https` scheme when the image base URL's host matches that of the current host, and the request is being served over HTTPS. This prevents mixed content warnings caused by `http` embedded media.
  REST API: Remove unused variable `$api_root` from WP_Rest_Server->embed_links() method.
  Bump grunt-contrib-qunit ~0.7.0 → ~1.1.0
  Bump grunt-contrib-watch ~0.6.1 → ~1.0.0
  Responsive images: Skip images with a missing `$image_meta['file']` value.
  ...
@delawski

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2016

I have pushed a fix proposal for the last issue. Please, take a look at my comment on trac.wordpress.org:

https://core.trac.wordpress.org/ticket/35947#comment:28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.