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

Negative values in column's width redistribution #27616

Open
lukewalczak opened this issue Dec 9, 2020 · 3 comments · May be fixed by #27681
Open

Negative values in column's width redistribution #27616

lukewalczak opened this issue Dec 9, 2020 · 3 comments · May be fixed by #27681

Comments

@lukewalczak
Copy link
Member

@lukewalczak lukewalczak commented Dec 9, 2020

Describe the bug

During the sanity testing before mobile release @chipsnyder noticed that we are able to get negative percentages in Columns and mentioned about it in the comment. Since the logic for width redistribution is shared the same occurs on web editor.

To reproduce
Steps to reproduce the behavior:

  1. Create a new Blog Post
  2. Add A Columns block
  3. Select 70/30
  4. Change column 1 to 90%
  5. Set Column 2 to 10% (or around there)
  6. Add a third column

Result:
With those numbers I got: 73.34%, -6.66% and 33.33%.

columns_web_negative

The same will happen when adding new column to the following columns: 90/5/5 and many other.

The problem is the wrong math within the function called getRedistributedColumnWidths located in columns utils.

export function getRedistributedColumnWidths(
	blocks,
	availableWidth,
	totalBlockCount = blocks.length
) {
	const totalWidth = getTotalColumnsWidth( blocks, totalBlockCount );
	const difference = availableWidth - totalWidth;
	const adjustment = difference / blocks.length;

	return mapValues( getColumnWidths( blocks, totalBlockCount ), ( width ) =>
		toWidthPrecision( width + adjustment )
	);

Mentioned method is assuming that difference (difference between available width and new columns width sum) is divided equally between columns and subtracted from each of them. As a result we can get negative values, since adjustment has higher value than width.

Let's consider it on the example from reproduction steps:

  • Two columns: 90% and 10%
  • New column will have width equal 33% (100 / 3)
  • availableWidth refers to the sum of first two columns after adding the third one: 66.67 (100 - 33)
  • total width refer to the sum of all columns: 100
  • difference is -33.33 (66.67 - 100)
  • adjustment is -16.665 (-33.33 / 2)

Finally we have

  • first column: 90 - 16.665
  • second column: 10 - 16.665
  • third column: 33.33

Possible problem solution

Imo the mechanism should be a bit different and width should be calculated proportionally to the totalWidth:

export function getRedistributedColumnWidths(
	blocks,
	availableWidth,
	totalBlockCount = blocks.length
) {
	const totalWidth = getTotalColumnsWidth( blocks, totalBlockCount );

	return mapValues( getColumnWidths( blocks, totalBlockCount ), ( width ) => {
		const newWidth = ( availableWidth * width ) / totalWidth;
		return toWidthPrecision( newWidth );
	} );
}

As a result we get positive values, divided proportionally:

  • first column: 60
  • second column: 6.67
  • third column: 33.33

Expected behavior

Columns shouldn't have negative width values

Screenshots

Attached above.
If applicable, add screenshots to help explain your problem.

Editor version (please complete the following information):

  • WordPress version: [e.g: 5.3.2]
  • Does the website has Gutenberg plugin installed, or is it using the block editor that comes by default? [e.g: "gutenberg plugin", "default"]
  • If the Gutenberg plugin is installed, which version is it? [e.g., 7.6]

Desktop (please complete the following information):

  • OS: [e.g. iOS]
  • Browser [e.g. chrome, safari]
  • Version [e.g. 22]

Smartphone (please complete the following information):

  • Device: [e.g. iPhone6]
  • OS: [e.g. iOS8.1]
  • Browser [e.g. stock browser, safari]
  • Version [e.g. 22]

Additional context

@chipsnyder
Copy link
Contributor

@chipsnyder chipsnyder commented Dec 9, 2020

Hey @tellthemachines and @gziolo, I saw that you two had worked in this area somewhat recently with the PR for fixing some unit tests. I was wondering if you two had enough context here to review @lukewalczak's plan.

@gziolo
Copy link
Member

@gziolo gziolo commented Dec 10, 2020

I don’t know how getRedistributedColumnWidths should work but what you shared makes a lot of sense. It definitely seems buggy to calculate negative values. I checked unit tests and they cover only 2 columns. I’m sure adding test cases that verify use cases you identified for 3 (or even more) columns as needing changes would be a good start here.

@lukewalczak
Copy link
Member Author

@lukewalczak lukewalczak commented Dec 11, 2020

@gziolo thanks for your answer, I'm planning to prepare the PR with the fix along with additional test to cover edgy cases

@lukewalczak lukewalczak linked a pull request that will close this issue Dec 11, 2020
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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