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

Fixes layout calculation problems for Firefox and Safari #580

Closed
wants to merge 2 commits into from
Closed

Fixes layout calculation problems for Firefox and Safari #580

wants to merge 2 commits into from

Conversation

johannheyne
Copy link

In Firefox the this.gutter value is sometimes a bit different. That causes in some use cases a this.cols calculation result lower than the expected. Using Math.round instead of Math.floor fixes this problem here and outputs the correct this.cols value.

There was a problem in calculation of the colSpan:
Firefox: operator % produces in some cases results > "1"
Safari: operator % produces in some cases "1" as result

In Firefox the gutter value is sometimes a bit different. That causes in some use cases a cols calculation result lower than the expected cols result. Using Math.round instead of Math.floor fixes this problem here.
Firefox: operator % produces in some cases results > "1"
Safari: operator % produces in some cases "1" as result
@@ -79,7 +79,8 @@ function masonryDefinition( Outlayer, getSize ) {
Masonry.prototype._getItemLayoutPosition = function( item ) {
item.getSize();
// how many columns does this brick span
var remainder = item.size.outerWidth % this.columnWidth;
var factor = item.size.outerWidth / this.columnWidth,
remainder = factor - Math.floor( factor );
Copy link
Owner

Choose a reason for hiding this comment

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

This calculation represents the fractional difference between item.size.outerWidth and columnWidth, not the pixel measurement.

@desandro
Copy link
Owner

I appreciate you taking a swing at this, but I don't feel that these edits will work to be merged into master.

@johannheyne
Copy link
Author

OK, but here is a fact in my use case and I´m going to proof it:
masonry_bug_columnwidth

@johannheyne
Copy link
Author

OK, I´ve got it. The thing is, that Math.floor keeps the total width of all cols and gutter within the parent container width. This is a feature so you cannot change this. Thats why Math.round should be used only if there is a miscalculation, may because getting the width from the computed styles. But how to solve this?

@johannheyne
Copy link
Author

Firefox Jumping Cols Bug

The width specified in pixels in the computed styles of the first item element or sizer element with percentage width definition like 33.33333333% or calc( 100% / 3 ) within a fluid element has at certain window widths a greater fractional pixel value than actually physically. In such a case, one column is less calculated than actually would be possible. The effect is, that the amount of calculated cols changes while sizing the layout.

Test: http://codepen.io/johannheyne/pen/lCctx

A fix could be to floor the columnWidth before it is used.
100.667 should be 100.66

this.columnWidth = Math.floor( this.columnWidth * 100 ) / 100;

@johannheyne
Copy link
Author

Safari Jumping Cols Bug

In Safari there is the same cols jumping effect in fluid layouts with elements in percentage width definition but with another reason. As far as I understand it, this bug is caused on how masonry.js determines to round the calculation of the cols by division of the this.columnWidth and the item.size.outerWidth.

The script assumes in var mathMethod = remainder && remainder < 1 ? 'round' : 'ceil';, that in the modulo operation var remainder = item.size.outerWidth % this.columnWidth; the result is whether 0 or an fractional number below 1. This is true as long as an integer value of this.columnWidth is an integer divider of item.size.outerWidth which is true as long the browser gives fractional values.

But Safari just delivers rounded integers for this.columnWidth and item.size.outerWidth. And thats why the result of calculation of var remainder is an integer of 0 or greater. Is remainder === 1 or greater, the var mathMethod gets the wrong rounding method which results in less cols than possible in this case.

A fix should be consider that in static layouts with static this.columnWidth setting the var remainder can be >= 1. In layouts with percentage this.columnWidth setting, the var mathMethod for calculating var colSpan = Math[ mathMethod ]( item.size.outerWidth / this.columnWidth ); should be always round.

var mathMethod = remainder && ( remainder < 1 ) ? 'round' : 'ceil';

// FIX: Safari Jumping Cols Bug
// if var this.columnWidth is a result of an element with percentage width, 
// var mathMethod should be set to 'round'

var colSpan = Math[ mathMethod ]( item.size.outerWidth / this.columnWidth );

desandro added a commit that referenced this pull request Jun 27, 2014
@NicolasGraph
Copy link

Will this issue be fixed in the future to easily play with frameworks?

@johannheyne
Copy link
Author

desandro fixed it in an separate branch
https://github.com/desandro/masonry/tree/overshoot

@dipson
Copy link

dipson commented Mar 20, 2015

@desandro Any reason why your overshoot branch has not been merged into master yet?

@desandro
Copy link
Owner

desandro commented Apr 1, 2015

I've merged the overshoot fix into master. Masonry v3.2.3 has the pixel rounding fix. Give it a shot!

Thanks again for this contribution. Moving on! 🐎

@desandro desandro closed this Apr 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants