Layout seems to stack up?

[pyrossh]

Sometimes the masonry layout seems to work when my data is loaded from the server. But sometimes it's just messed. Tried to debug it and couldn't find it. Can someone help me with this? I don't know if this is related to this https://github.com/eiriklv/react-masonry-component/issues/54

Its live here http://bindi.surge.sh/

[yoadsn]

Have been having this problem as well. For me it happens when I change the array of children cards in the Masonry component without unmounting/mounting the component itself. This bug was introduced in 5.0.0 - reverting back to 4.4.0 and below resolves the problem.

This makes some sense since the change introduced in 5.0.0 is:

• Not done by the original maintainer
• Removed a check which is related to removal of elements from the dom.
• Not covered by any tests as far as I can see

As far as I can tell the "stacking problem" is actually a crash somewhere in 'componentDidUpdate' of the Masonry component.

Hopefully I can find the exact place.

[yoadsn]

Ok, Here is a repro of this problem: http://www.webpackbin.com/EkbX-8h4G Clicking the "switch" button would change the children (entirely) and would cause an exception as well as the "stacking up" behavior.

Uncaught TypeError: Cannot read property 'removeChild' of null
    at SubClass.proto.removeElem (http://sandbox.webpackbin.com/api/sandbox/vendors/118193884/bundle.js:39:13930)
    at SubClass.proto.remove (http://sandbox.webpackbin.com/api/sandbox/vendors/118193884/bundle.js:39:14201)
    at SubClass.<anonymous> (http://sandbox.webpackbin.com/api/sandbox/vendors/118193884/bundle.js:34:9883)
    at Array.forEach (native)
    at SubClass.proto.remove (http://sandbox.webpackbin.com/api/sandbox/vendors/118193884/bundle.js:34:9855)
    at Constructor.performLayout (http://sandbox.webpackbin.com/api/sandbox/vendors/118193884/bundle.js:22:14199)
    at Constructor.componentDidUpdate (http://sandbox.webpackbin.com/api/sandbox/vendors/118193884/bundle.js:22:15490)
    at CallbackQueue.notifyAll (http://sandbox.webpackbin.com/api/sandbox/vendors/118193884/bundle.js:16:20402)
    at ReactReconcileTransaction.close (http://sandbox.webpackbin.com/api/sandbox/vendors/118193884/bundle.js:20:14015)
    at ReactReconcileTransaction.closeAll (http://sandbox.webpackbin.com/api/sandbox/vendors/118193884/bundle.js:16:23992)

[yoadsn]

Some more info on the crash:

react-masonry: performLayout calls var diff = this.diffDomChildren(); and get list of dom elements. react-masonry: performLayout calls this.masonry.remove(diff.removed); since there are "removed" elements. masonry: delegates the remove call to outlayer. outlayer: finds the outlayer items for each element and calls remove on each item. outlayer item: calls removeElem on the item which does this.element.parentNode.removeChild( this.element ); this last call find that parentNode is null hence the exception: Uncaught TypeError: Cannot read property 'removeChild' of null.

The cause must be the first call to this.diffDomChildren(); which now returns an element which has a corrresponding Outlayer item but has no parent to remove from. Trying to remove this item does not guard against an element which is no longer part of the dom.

Who needs to fix this? Outlayer (by guarding) or masonry-react (by making sure there is a parent). I think the fact that Outlayer does not allow removal of items which are no longer in the DOM is a problem...

A mismatch between how react works with the dom tree and Outlayer. React expects to be able to remove and add elements on the tree without anything standing in the way.

If we keep the item "keys" and just replace the content it would probably not cause the crash. But this relies on some internal optimization of react which could change any day. see: http://www.webpackbin.com/NJuA5IhNz

On the other hand, the way it was before had a memory leak of items in Outlayer which in turn leaked detached dom elements. What a mess.

[mikelambert]

Hey there, thanks for debugging, and sorry for the breakage. Totally agree it's probably related to my change, for all the reasons you mention. ;)

Nice discovery on the Outlayer memory leak in the old code, regardless!

In my case, with the original code react-masonry-component code, I found that the old elements I had deleted from my array-of-masonry-children were being skipped from getDiffDomChildren() because they had no parent. This screwed up the removed field, which was always empty for me (as I discussed in my other message)...and resulted in me seeing rendering issues when removing elements from the first half of the array (as the elements in the second half of the array were then laid out incorrectly, all with left: 0).

I'm sorry it's late and I don't have a great answer (I have never dug into the masonry layout code or outlayer itself), but wanted to give you a response to the breakage I likely caused.

Interestingly enough, I do not have any rendering issues, and do not get any JS exceptions from my use of these changes, like you do. You can see this in action at http://www.dancedeets.com/tutorials/ (though I apologize that it's all minified/compressed, but you can use the React debugger at least).

In @pyros2097 case, it looks like the individual components use animations and transition properties (which I don't use). In the case of the repro, it uses other properties. However, I agree that the repro case is still a simple enough breakage, and still hints at something fishy going on...

But I'm trying to understand what differentiates your breakages from my also even-more-basic usage of (without any parameters whatsoever), where my children have a fixed with and variable height (set by the auto-layout of the children themselves, as opposed to fixed heights set statically in the child itself.)

[yoadsn]

Thanks for the live example - it helped. The difference, is in the option transitionDuration - when it's 0 the crash occurs - otherwise it's not. See the repro here - http://www.webpackbin.com/4Jev5F2NM Why would that matter?

Checkout the code in Outlayer for removing an item:

proto.remove = function() {
  // just remove element if no transition support or no transition
  if ( !transitionProperty || !parseFloat( this.layout.options.transitionDuration ) ) {
    this.removeElem();
    return;
  }

  // start transition
  this.once( 'transitionEnd', function() {
    this.removeElem();
  });
  this.hide();
};

So clearly the removal process considers the transition duration. When a transition is required it would wait for a 'transitionEnd' event to remove the actual DOM element from it's parent. This event would fire after the hide() call here initiates the transition. I am not clear about this mechanism, it's quite convoluted, but I can see this in the ontransitionend handler:

if ( event.target !== this.element ) {
    return;
  }

Which could well mean that detached elements would not be considered for transition and so the event would never fire and the the remove code would not run for them, hence no crash.

Someone with more knowledge about Outlayer may be able to clear this up.

What do we do now? We cannot force everybody to set a transition duration. I disabled mine since it caused other problems with rendering.

[yoadsn]

Ok, after some more debugging the transitionend event indeed does not fire for the elements which are not in the DOM - the crashing code does not run. It has nothing to do with the lines I suspected. I think it's due to the fact the elements outside of the DOM are not getting transition events (and transitions at all for that matter).

So, it's only by chance that the crash did not occur in your scenario. Nice! ;)

[afram]

I've created 2 new test cases - one with default transition and one with explicit transitionDuration set to zero.

The test case you would expect to fail now fails. :-)

Hopefully it will aid in not regressing this in future.

[yoadsn]

Awesome. Thanks. I will wait on @mikelambert for a repro on his original problem before attempting to suggest a fix. For anyone reading this at the moment including the OP - three workarounds are possible:

• revert to 4.4.0
• use "stable keys" - so that the index of the children will not change and so react would not remove the children from the dom. (would only take you so far, since at some point the number of children would change)
• set a transitionDuration - even a "1" would do (a 1 ms duration, unnoticeable)

[abelsoares]

@yoadsn , @mikelambert , any update on this?

[mikelambert]

@yoadsn's change https://github.com/eiriklv/react-masonry-component/commit/c844c9c83c632b1f5ec6b9ca261cf48f278cfa2d fixed my issue (and reverted my broken change), and was released as 5.0.1.

[afram]

You'll want to use 5.0.3 as there were bugs in 5.0.1

[pyrossh]

Its fixed for me. I just had to add the masonry options disableImages and stuff...

[kidequinox]

having stacking issue on 5.0.3. tried transitionDuration and disableImages. Because the images have not loaded yet, the layout gets created with out calculating the sizes.

[afram]

Hi @kidequinox

Have you tried this in Firefox/Chrome/Edge/Safari browsers with consistent results?

Can you please paste a code sample, or better yet, can you please create an example which demonstrates what you are experiencing on http://www.webpackbin.com/

[pyrossh]

These are the options i use,

options={{transitionDuration: 0 }}
disableImagesLoaded={false}
updateOnEachImageLoad={true}>

I don't think this a masonry bug, it seems to me like your height for the container is fixed or something.

[kidequinox]

I'm loading the image in as the background of the div, and using it's :after to make the div scale responsively in proportion.

.audioImg:after { padding-top: 100%; display: block; content: ''; }

@pyros2097 I have the same options settings.

@afram It's Stacking in chrome/ff/safari.

[yoadsn]

@kidequinox Perhaps I'm misreading your approach. But since you are using background-image of the divs - they must have the size defined for them (width and height). They will not grow/shrink to fit the loaded image.

The styling you shared with us shows you are trying to use the intrinsic sizing approach to make sure your container divs take up all the space before the image is loaded. "after" would not do - what makes your divs have height is the "padding-top" which is relative to the width of the container. since you set it on 100% - that means your divs are always squares, (ratio of 1:1 between width and height). Consequently, If your images have an aspsect ratio other then 1:1 they would not fit. Depending on the background-position style property - they could behave in a number of ways.

This lib (masonry) can predetermine the width for you without the images being loaded (relative, of fixed) But it cannot know the height - this is something you need to get somehow to the client in the form of aspect ratio along with the image src.

I hope that clarifies the approach a bit - let me know if you need more details.

p.s. if you can create a minimal example in webpackbin with your problem - we can be more helpful. p.s.s I think this could well be in a new issue unrelated to the original bug discussed here.

[kidequinox]

@yoadsn Your right, this is possibly another issue.

I was using the intrinsic sizing approach, so there is no set height on that div. So will setting the height and width style of the

work ? I can do this in JS on the div's style and scale the values proportionally on window resize.

I'm using the image as a background so I can crop the image to a square via css.

p.s. I'm in a time crunch so I can't do a webpackbin at the moment. I appreciate your feedback!!

[yoadsn]

@kidequinox Oh shoot - sorry dude I did not see your comment yet and did not know you are in a hurry. Using padding to set the height of the div (relative to the width of the container) and then containing another div within it with left: 0; top: 0; width: 100%; height: 100%; Will get you a div (the inner one) in the right size (correct aspect ratio). Then, you can either set an image to fill up that div with the background css prop or img tag. it should work.

Setting the padding percentage using JS is the correct approach unless you know in advanced the aspect ratio of all images and it's the same one. (not likely).

To summarize - I created this codepen that might help: http://codepen.io/yoadsn/pen/ZLVRyd

• A container - responds to your page and has some width.. If you want it to be relative to screen size (for example 30%) you can set a relative width. you can also use media queries to change the width according to screen sizes.
• An outer div, will fill up the container - same width as the container.
• The Outer div will use padding percentage and a height of 0 to set a content height relative to the width of container. This would make sure the content height relative to content width reflects the required apsect-ratio of the image to present.
• Create an inner div to fill up all the parent div (position: absolute, top,left 0.. width/height 100%)
• Fill inner div with an img (width: 100%) or set it's background property (background-position = cover) to the image you want.

HTH

[kidequinox]

Thank you @yoadsn this works for the intrinsic sizing approach!!! Thank you much!!