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

Interface Skeleton: Limit the Editor Interface to max-width: 100% #26552

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Oct 28, 2020

Description

Some blocks are somehow able to exceed the editor boundaries and, when set to full width, gain a width of literally millions of pixels, making the editor unusable.

I'm not sure how to justify this solution/workaround, but it works, and it doesn't seem to cause regressions.

See: Automattic/jetpack#17616

How has this been tested?

Tested in both post and site editor, trying a wide array of different blocks, set at wide and full width.

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@Copons Copons added [Type] Bug An existing feature does not function as intended General Interface Parts of the UI which don't fall neatly under other labels. [Package] Interface /packages/interface labels Oct 28, 2020
@Copons Copons self-assigned this Oct 28, 2020
@github-actions
Copy link

Size Change: +18 B (0%)

Total Size: 1.21 MB

Filename Size Change
build/edit-post/style-rtl.css 6.39 kB +5 B (0%)
build/edit-post/style.css 6.38 kB +5 B (0%)
build/edit-site/style-rtl.css 3.86 kB +3 B (0%)
build/edit-widgets/style-rtl.css 3.1 kB +2 B (0%)
build/edit-widgets/style.css 3.11 kB +3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.78 kB 0 B
build/api-fetch/index.js 3.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 130 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.94 kB 0 B
build/block-library/editor.css 8.94 kB 0 B
build/block-library/index.js 146 kB 0 B
build/block-library/style-rtl.css 7.82 kB 0 B
build/block-library/style.css 7.82 kB 0 B
build/block-library/theme-rtl.css 837 B 0 B
build/block-library/theme.css 838 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.2 kB 0 B
build/components/style.css 15.2 kB 0 B
build/compose/index.js 9.81 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 772 B 0 B
build/data/index.js 8.77 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.46 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-site/index.js 22.1 kB 0 B
build/edit-site/style.css 3.85 kB 0 B
build/edit-widgets/index.js 26.4 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 43.1 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.7 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.2 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@jeyip
Copy link
Contributor

jeyip commented Oct 28, 2020

Tests

Functionality

  • The slideshow block does not disappear when selecting the full width setting
  • The slideshow block full width setting behaves as expected
  • Full width frontend block behaves as expected

Browsers

  • Chrome
  • Firefox
  • Edge
  • Safari

In IE11, I'm seeing a Your site doesn't include support for the jetpack/[relevant-block] error when testing jetpack specific blocks. This is, however, happening on the master branch as well.

I'll be double-checking the full screen mode for other blocks and update this comment as I progress.

  • Gif
  • Image
  • OpenTable
  • Eventbrite checkout
  • Latest Posts

Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to justify this solution/workaround, but it works, and it doesn't seem to cause regressions.

I'm not certain if this is the best solution either, but I can confirm that it fixes the issue and doesn't appear to cause regressions in the interface layout or block width settings. Approving this for now, but I'll continue testing other blocks and thinking about why the width: 100% style addresses the issue.

@oandregal
Copy link
Member

For context, this regression was introduced at #25884 which made a few changes to the grid model and positioning elements of the interface package. The interface package has a wide reach as it's used by all editors, so I'd welcome that we gain some more understanding of what happened in 25884 and why this fixes it before landing this PR. cc @jorgefilipecosta @jasmussen for thoughts.

@Copons
Copy link
Contributor Author

Copons commented Oct 29, 2020

The interface package has a wide reach as it's used by all editors, so I'd welcome that we gain some more understanding of what happened in 25884 and why this fixes it before landing this PR.

@nosolosw Totally agree with this. I'm keeping this PR un-merged until we have more clues on what's happening.

Unfortunately, debugging this issue is complicated.
As far as we know, it only happens with the Slideshow block from Jetpack (hence requiring installing Jetpack), which uses the external library Swiper, which, when the block is set at full width, incorrectly calculates its width to 33554400px (written in scientific notation as 3.35544e+07px) instead of the available editor width.

For context, the issue causes the whole editor to be unusuable, as its width too becomes extremely large, moving all centered elements (the title and all the correctly sized blocks) millions of pixel to the right, making the editor look completely blank.
On top of this, calculating such large number seems to affect the editor performances, but I don't have any hard data to back this assumption.

When I investigated the issue, I stumbled upon a Stack Overflow thread mentioning that 33554400 is the maximum possible width value of some browsers.
This made me wonder that maybe Swiper was somehow failing to infer the slideshow's container width, and accidentally using the maximum browser's value.
Therefore my proposed solution workaround is to define a width to a slideshow parent element, which luckily enough seems to have hit the right spot.

Curiously enough, I had to set the width to .interface-interface-skeleton__editor because setting it to interface skeleton elements (or any others, for that matter) closer to the block didn't seem to work.

cc @fullofcaffeine @ockham who were involved with the issue and the temporary fix on Jetpack, and might have some additional insights, or ideas about this.

@jeyip
Copy link
Contributor

jeyip commented Oct 30, 2020

After lots of investigation, here's what I've uncovered so far:

The more I look into the cause, the more it seems like the issue lies with the Swiper JS library. In a demo with Swiper, I can reproduce an error similar to what we saw in the Jetpack slideshow block. That is to say, I can recreate a problem where swiper slides are set to a width of 33554400px.

  1. Add an opening and closing div around all of the HTML code in this Codepen demo
  2. Resize the iframe window with the drag bar that sits between the HTML, CSS, and JS code editors and the browser view.
  3. Using the dev console, verify the width of the swiper-slides as 33554400px
  4. Add a classname attribute to the new div we just added
  5. In the CSS code editor, select the classname and provide an explicit width of 100%
  6. Try resizing the browser again
  7. Verify that the swiper slides do not have a width of 33554400px

One thing to note about the demo is that it uses version 4.3.3 of swiper. I plan to recreate another demo with an updated version tomorrow.

@jeyip
Copy link
Contributor

jeyip commented Oct 30, 2020

It's late here and I think this writeup might need more clarity. I'll return to it in the morning to continue editing, but hopefully the gist of it is understandable.

There's still more investigation to be done, but digging into swiper js source code, I see a few things that should be noted:

My best guess as to what's happening is that some calculation in updateSlides increases slide widths beyond the swiper container. This subsequently increases the swiper container clientWidth. When the update method is called again, it walks through another round of updateSize and updateSlides, which then further increases slide widths beyond the swiper container, which eventually reaches 33554400px.

I'm still uncertain as to why update is being called repeatedly 🤔

@sirreal
Copy link
Member

sirreal commented Oct 30, 2020

See: Automattic/jetpack#17616

The same issue appears to affect the CoBlocks Post Carousel block:

post-carousel

(reported by @mrfoxtalbot)

@ockham
Copy link
Contributor

ockham commented Oct 30, 2020

Thanks a lot for your research, @jeyip ❤️

@Copons
Copy link
Contributor Author

Copons commented Oct 30, 2020

I've started looking into the issue with the CoBlocks Post Carousel block, and it seems it's exactly the same problem, down to the same 33554400px width, and it would be fixed by this PR as well.

Both Jetpack Slideshow and CoBlocks Post Carousel are "slider" blocks, but they use different external libraries: the former uses Swiper, the latter Slick.
So it's just fair to say that it's our fault, rather than theirs (unless we manage to demonstrate they are broken in exactly the same way).

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can not say for sure that this PR did not cause any regression. But in the tests, I did in edit-widgets, edit-site, and edit-post both in desktop and mobile views things worked well and the PR fixes the issues identified so I guess it could be merged.

@sirreal sirreal added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 2, 2020
@youknowriad
Copy link
Contributor

We could use some help figuring out the intermittent failures here. The reusable blocks one for instance.

@youknowriad youknowriad merged commit 41e09f3 into master Nov 2, 2020
@youknowriad youknowriad deleted the fix/interface-skeleton-editor-width branch November 2, 2020 10:36
@github-actions github-actions bot added this to the Gutenberg 9.3 milestone Nov 2, 2020
tellthemachines pushed a commit that referenced this pull request Nov 11, 2020
tellthemachines added a commit that referenced this pull request Nov 12, 2020
* Cover block: Restore default overlay background (#26569)

* Restore default Cover overlay background

The default Cover block overlay background was removed. This changed the
style of existing blocks on existing sites.

Restore the default background in such a way that it does not conflict
with theme-provided background-color styles and there is no need to
artificially add extra specificity.

Fix regression: #26545

* Limit the interface skeleton to a max width of 100% to prevent some blocks managing to exceed the width (#26552)

* Cover Block: Restore opacity controls (#26625)

* Fix image block centering when resizing to a smaller size (#26376)

* Fix image block centering when resizing to a smaller size

* Revert previous 100% width fix

* Remove display: table when image block is resized to avoid centering of block

* Match frontend classes for alignment in editor

* Gallery: Remove caption fallback for alt attribute (#26676)

* Fix quote block default alignment (#26680)

* Gallery: Remove obsolete deprecation entry (#26736)

* Do not apply text color if it is being overriden (#24626)

* Fix: Preset colors don't work on button block style outline (#26707)

* Tests: Add fixture for Column deprecation (#26774)

* Fix/column width units (#26757)

* Fix issues with different units in column widths

* Columns with fixed width should keep width on recalculation

* Address review feedback

* fix undefined index notice in Social Link Block (#25663)

* fix undefined index notice

* use isset instead of array_key_exists check

* Update packages/block-library/src/social-link/index.php

Co-authored-by: Greg Ziółkowski <[email protected]>

Co-authored-by: Greg Ziółkowski <[email protected]>

* Adds in missing styling for toolbar when using text-only setting (#26769)

* Adds in missing styling for toolbar when using text-only setting

* Increases margin

* Moves style to correct file

* Inserter: Fix 'Browse All' in Quick Inserter (#26443)

* Inserter: Fix 'Browse All' in Quick Inserter

Maintain the insertion point in @wordpress/block-editor state when
moving from the Quick Inserter to the Inserter.

This fixes a bug where the insertion point is wrong after clicking a
block appender and selecting 'Browse All'.

Care is taken to not regress a previous bug where the insertion point is
wrong after clicking an inline insertion button and selecting 'Browse
ALl'.

* Inserter: Fix typo

Co-authored-by: Daniel Richards <[email protected]>

* Call getBlockInsertionPoint once

* Add useSelect dependencies

* Make setInsertionPoint unstable

* Avoid setting `isVisible` state when `SET_INSERTION_POINT` is triggered

* Make index required and clarify rootClientId usage

* Split insertionPoint into two reducers

* Fix getInsertionPoint selector that expects default state of reducer to be null

* Avoid resetting insertionPoint state on HIDE_INSERTION_POINT

Co-authored-by: Daniel Richards <[email protected]>

Co-authored-by: Jon Surrell <[email protected]>
Co-authored-by: Jacopo Tomasone <[email protected]>
Co-authored-by: Daniel Richards <[email protected]>
Co-authored-by: Bernie Reiter <[email protected]>
Co-authored-by: Greg Ziółkowski <[email protected]>
Co-authored-by: Oliver Juhas <[email protected]>
Co-authored-by: Jorge Costa <[email protected]>
Co-authored-by: Fabian Kägy <[email protected]>
Co-authored-by: Tammie Lister <[email protected]>
Co-authored-by: Robert Anderson <[email protected]>
@tellthemachines tellthemachines removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 16, 2020
Copons referenced this pull request Dec 8, 2020
)

* Set toggle and nav panel as first element in interface skeleton

* Position sidebar toggle alongside header topbar

* Add site and content wrappers to interface skeleton

Previously, the navigation panel and navigation toggle lived within skeleton__body and skeleton__header, respectively. In order to address unexpected keyboard focus order between the nav toggle and nav panel, we create a new element inside of interface-skeleton called skeleton__navigation-sidebar.

To account for the layout changes of introducing a new element, we add two wrapper classes: skeleton__site and skeleton__content. Both ensure that the skeleton__header and skeleton__body shrink their children correctly when the navigation sidebar is open.

* Add and remove padding to ensure document settings title is centered

* Remove toggle from flow of page when sidebar is closed

The navigation sidebar lives alongside the skeleton header and skeleton body. This is to ensure that the children of the body and header shrink when the navigation sidebar is open.

The problem is that when the navigation sidebar is closed, the skeleton body (i.e. the block editor) no longer spans the entire width of the viewport. The sidebar (and toggle) occupy space at the edge of the screen. To address this, we position the navigation toggle absolutely when the sidebar is closed.

* Refactor navigation sidebar directory structure

Because navigation panel and navigation toggle live outside the header and body, we place both components in their own navigation sidebar directory

* Update scss references

* Update navbar-toggle references

* Modify z index of footer

* Remove unnecessary comment

* Remove interface-interface-skeleton__layout

* Use interface-skeleton as container

* Rename skeleton__site to skeleton_editor

* Rename skeleton__navigation sidebar to drawer

* Remove comment

* Use z-index mixin for interface-skeleton__footer

* Use z-index mixin for navigation toggle

* Add missing comma in z-index file

* Update interface drawer label for navigation sidebar

* Recenter document actions

* Create variable for header toolbar min width

* Use navigation sidebar redux actions and selectors in refactored nav sidebar

Navigation sidebar and inserter sidebar state were both moved into the 'core/edit-site' data store. We incorporate any relevant actions and selectors into our source code updates.

* Remove unnecessary isNavigationOpened selector

* Return isNavigationOpened directly from selector

Co-authored-by: Copons <[email protected]>
@oxyc
Copy link
Member

oxyc commented Dec 9, 2020

Seems this did not get backported to WP 5.6 after all.

Screen Shot 2020-12-09 at 14 11 14

Edit: For reference, this was broken in #26944

@Copons
Copy link
Contributor Author

Copons commented Dec 9, 2020

For context, the max-width was removed in #26944 because not compatible with IE11.

It was totally my bad for not testing the change on IE11. I'll open a new issue and try to solve it ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels. [Package] Interface /packages/interface [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants