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

Block Library: Flatten width calculation of gallery columns #20690

Merged
merged 2 commits into from Mar 9, 2020

Conversation

@aduth
Copy link
Member

aduth commented Mar 6, 2020

Fixes #20652

This pull request seeks to update the width calculation applied to the gallery block columns. These changes are intended both as a simplification/unification of the width calculation, and as a resolution to a crash which occurs in Microsoft Edge.

As observed at #20652 (comment), the crash appears to occur as a result of the Edge-specific @supports (-ms-ime-align:auto) { block implemented in the gallery block styles.

As noted in #13326, the original issue #13270 is a result of how Edge interprets the width style. Compare how the style width: calc((100% - 16px * 2) / 3); is interpreted between Edge and Chrome:

Edge Chrome
Screen_Shot_2020-03-06_at_1_15_46_PM Screen_Shot_2020-03-06_at_1_16_03_PM

The proposed changes should apply effectively the same width, but simplify the calculation at compile time, resulting in the style calc(33.33333% - 10.66667px). It appears that this is handled better in Edge, avoiding the early wrapping, rounding down to 10.66px per column.

image

Aside: There is overflow which occurs in Edge for the gallery block, with or without these changes. This should probably be investigated separately.

Testing Instructions:

Repeat steps to reproduce from #20652, verifying that no crash occurs.

Repeat steps to reproduce from #13270 / testing instructions from #13326, verifying that gallery columns wrap as expected both in Edge and in your preferred browser.

Follow-up tasks:

There is another style which uses the problematic @supports(-ms-ime-align:auto) block:

// Rules inside this query are only run by Microsoft Edge.
// Edge has bugs around position: sticky;, so it needs a separate top rule.
// See also https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/17555420/.
@supports (-ms-ime-align:auto) {
position: fixed;
width: 100%;
}

However, there is no immediate crash which occurs as a result of this style. The problem may only manifest as a combination of @supports within a @media media query. In any case, over time we should consider removing these Edge-specific styles, which are no longer valid in recent Chromium-based versions of Edge.

@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Mar 6, 2020

Size Change: -101 B (0%)

Total Size: 864 kB

Filename Size Change
build/block-library/style-rtl.css 7.45 kB -51 B (0%)
build/block-library/style.css 7.46 kB -50 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 104 kB 0 B
build/block-editor/style-rtl.css 10.6 kB 0 B
build/block-editor/style.css 10.6 kB 0 B
build/block-library/editor-rtl.css 7.36 kB 0 B
build/block-library/editor.css 7.36 kB 0 B
build/block-library/index.js 115 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.75 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.3 kB 0 B
build/edit-post/style-rtl.css 8.64 kB 0 B
build/edit-post/style.css 8.64 kB 0 B
build/edit-site/index.js 4.64 kB 0 B
build/edit-site/style-rtl.css 2.48 kB 0 B
build/edit-site/style.css 2.48 kB 0 B
build/edit-widgets/index.js 4.44 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/index.js 43.8 kB 0 B
build/editor/style-rtl.css 3.98 kB 0 B
build/editor/style.css 3.98 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.11 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

Copy link
Contributor

jasmussen left a comment

Much simpler code, nice.

Happy to see the before and after identical in editor and preview, at arbitrary column widths.

I can also confirm that Edge loads, and seems to work well, for me:

Screenshot 2020-03-09 at 09 23 59

@aduth aduth merged commit 1612deb into master Mar 9, 2020
4 checks passed
4 checks passed
build
Details
pull-request-automation
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@aduth aduth deleted the fix/20652-edge-gallery-column-calc branch Mar 9, 2020
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.