Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#60263 closed defect (bug) (fixed)

Fluid typography: PHP division by zero error when theme.json min and max viewport widths are equal

Reported by: ramonopoly's profile ramonopoly Owned by: isabel_brison's profile isabel_brison
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.5
Component: Editor Keywords: has-patch has-unit-tests gutenberg-merge
Focuses: Cc:

Description (last modified by ramonopoly)

When the same value is provided for min and max viewport widths in theme.json fluid typography config, PHP will throw a DivisionByZeroError.

This is because fluid typography calculates the linear scale denominator using minViewportWidth - maxViewportWidth.

See the two 1640px values:

{
	"$schema": "https://schemas.wp.org/trunk/theme.json",
	"version": 2,
	"settings": {

		"typography": {
			"fluid": {
				"minFontSize": "16px",
				"maxViewportWidth": "1640px",
				"minViewportWidth": "1640px"
			}
		}
	}
}

To avoid dividing by zero values, I propose returning null when minViewportWidth - maxViewportWidth is zero or some other falsey.

The consequence is that fluid font sizes will not be calculated.

The fluid typography clamp rule needs valid max and min viewport constraints.

Alternatives to this approach:

  1. We could provide a fallback from the default values in gutenberg_get_typography_font_size_value(), e.g., 1600 - 320.

Change History (10)

This ticket was mentioned in PR #5875 on WordPress/wordpress-develop by @ramonopoly.


3 months ago
#1

Syncs:

When the same value is provided for min and max viewport widths in the fluid typography config, set the linear scale factor to default 1.

To avoid dividing by zero values. PHP will throw an error and, besides, the fluid typography clamp rule needs valid max and min viewport constraints.

Trac ticket: https://core.trac.wordpress.org/ticket/60263

@ramonopoly commented on PR #5875:


3 months ago
#2

Those failing PHP 8 multisite tests look unrelated to me.

@andrewserong commented on PR #5875:


3 months ago
#3

Just adding a similar comment as to the one I left on https://github.com/WordPress/gutenberg/pull/57866#pullrequestreview-1822541005, but I'm wondering if it'd be better to switch off fluid typography / return early if the min and max viewport sizes are the same?

#4 @ramonopoly
3 months ago

  • Description modified (diff)

@ramonopoly commented on PR #5875:


3 months ago
#5

but I'm wondering if it'd be better to switch off fluid typography / return early if the min and max viewport sizes are the same?

Thanks @andrewserong

I agree and I've updated accordingly.

The fluid typography clamp rule needs valid max and min viewport constraints to be "fluid" anyway, so returning early makes total sense.

🍺

@audrasjb commented on PR #5875:


3 months ago
#6

This looks good to me. I restarted the failed phpunit test.

#7 @audrasjb
3 months ago

  • Milestone changed from Awaiting Review to 6.5

Moving to milestone 6.5.

@audrasjb commented on PR #5875:


3 months ago
#8

Unit tests are passing now.

#9 @isabel_brison
3 months ago

  • Owner set to isabel_brison
  • Resolution set to fixed
  • Status changed from new to closed

In 57329:

Editor: fix fluid font division by zero error when min and max viewport widths are equal.

Fixes a division error by returning null when minViewportWidth - maxViewportWidth is zero in wp_get_computed_fluid_typography_value.

Props ramonopoly, mukesh27, andrewserong, audrasjb.
Fixes #60263.

@isabel_brison commented on PR #5875:


3 months ago
#10

Committed in r57329.

Note: See TracTickets for help on using tickets.