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

CellMeasurer notes: causes of height measurement errors, and how to fix them. #352

Closed
estaub opened this issue Aug 15, 2016 · 18 comments
Closed

Comments

@estaub
Copy link

estaub commented Aug 15, 2016

CellMeasurer renders each cell outside of the table context, and hence does not apply any inherited styling from outer elements. This kind of issue can sometimes be hard to debug. An approach I've used is to dump Window.getComputedStyle() on both the CellMeasurer and the visible version of cells at issue, and diff them. This requires using a breakpoint in CellMeasurer's "measureCell", since the hidden renderings are transient. [If other users need to do this also, an enhancement that makes the hidden renderings accessible to a handler might be useful.]

Another source of errors is vertical alignment. This may only occur with table layout. If different cells have different alignment, the vertical space required for a row can be larger than that measured for any single cell. Similarly, a baseline-aligned large image in one cell can require space above the first row of text in other cells, and thus forces padding that is not counted other cells' sizes. If one of these other cells is the tallest cell in the row (e.g. multiple lines), it will not count the image size above the top line's lineHeight. I've been able to work around this by using consistent top alignment. There might be a way for CellMeasurer to measure this vertical-alignment-forced padding and correct for it, but I haven't dug into it.

@bvaughn
Copy link
Owner

bvaughn commented Aug 15, 2016

CellMeasurer renders each cell outside of the table context, and hence does not apply any inherited styling from outer elements.

Hm. Ok. I can try to make this more explicit in the docs. I can see how it might be unexpected.

This requires using a breakpoint in CellMeasurer's "measureCell", since the hidden renderings are transient. [If other users need to do this also, an enhancement that makes the hidden renderings accessible to a handler might be useful.]

I don't think I want this transient rendered state to be exposed. It shouldn't be used for anything, and may change with future versions of CellMeasurer if/when I find more performant ways to do this measuring. (It's already changed once.)

Another source of errors is vertical alignment. This may only occur with table layout. If different cells have different alignment, the vertical space required for a row can be larger than that measured for any single cell.

I don't understand. CellMeasurer measures all columns in a row, and then uses the tallest one. If you're concerned about vertical alignment within a shorter column, just set the height of that column to fill 100% of its container cell and vertically align (using either flexbox, line-height, or position+top).

Similarly, a baseline-aligned large image in one cell can require space above the first row of text in other cells, and thus forces padding that is not counted other cells' sizes. If one of these other cells is the tallest cell in the row (e.g. multiple lines), it will not count the image size above the top line's lineHeight. I've been able to work around this by using consistent top alignment. There might be a way for CellMeasurer to measure this vertical-alignment-forced padding and correct for it, but I haven't dug into it.

Sounds like what you're describing is too complicated for CellMeasurer to handle. It isn't meant to be a general layout algorithm (like flow or flexbox). It just measurers heights and/or widths. My human brain doesn't even fully understand the layout rule you're describing to be honest. (I don't understand how an image alignment in one column would require top-margin in another.) But you should be able to account for this in user-land, I think. Just add paddings or margins inside of your rendered content and CellMeasurer will account for them when measuring the client rect.

@bvaughn
Copy link
Owner

bvaughn commented Aug 15, 2016

@bvaughn
Copy link
Owner

bvaughn commented Aug 15, 2016

I don't think there's anything else actionable on this ticket (at least within a scope I think I can take on for react-virtualized) so I'm going to close the issue for now. We can continue discussion here though if you have additional input. :)

@bvaughn bvaughn closed this as completed Aug 15, 2016
@estaub
Copy link
Author

estaub commented Aug 16, 2016

For reference, here's a simple example where the row height is larger than that of any single column, using consistent "baseline" alignment:

<table>
    <tbody>
    <tr>
        <td style='vertical-align: baseline'><img
            src='https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png'>
        </td>
        <td style='width:1em; vertical-align:baseline'>a b c d</td>
    </tr>
    </tbody>
</table>

or alternatively, using flex:

<div style='display:flex; align-items:baseline'>
        <div><img
            src='https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png'>
        </div>
        <div style='width:1em'>a b c d</div>
</div>

I believe the fix for this, should it be called for, is to measure the row in toto, not cell by cell.

@bvaughn
Copy link
Owner

bvaughn commented Aug 16, 2016

I don't think CellMeasurer can handle this use-case.

@estaub
Copy link
Author

estaub commented Aug 17, 2016

I figured out how, in using CellMeasurer with VirtualScroll, to avoid the vertical-alignment issue, and for the cells to inherit style specified on the row. Simply invoke CellMeasurer with just one column, which contains the entire row, e.g.:

                    <CellMeasurer
                      cellRenderer={({columnIndex,rowIndex})=>rowRenderer({index:rowIndex})}
                      columnCount={1}

You might consider changing the guidance on using CellMeasurer with VirtualScroll to this approach. I must confess that, despite multiple readings,I don't understand the current guidance; it may achieve the same effect, if renderCell is intended to render the entire row, ignoring the columnIndex parameter.

In addition to being accurate in more contexts, this speeds up the measurement a great deal - in my case, better than 2x. Window resizing isn't snappy, but it's a lot better.

@bvaughn
Copy link
Owner

bvaughn commented Aug 18, 2016

You might consider changing the guidance on using CellMeasurer with VirtualScroll to this approach.

I'm afraid I don't understand. VirtualScroll has only 1 column per row. (There is no rowRenderer.) I'm also not sure how this is related to any speed increases to be honest. If there are multiple columns, Grid is going to ask for their size one at a time. So rendering the entire row each time (assuming you're using Grid) would be slower, I'd expect. If you're using VirtualScroll then there's only 1 column to begin with- so I really don't understand.

@estaub
Copy link
Author

estaub commented Aug 18, 2016

You're confused, or more likely miswrote; see https://github.com/bvaughn/react-virtualized/blob/master/docs/VirtualScroll.md for doc on rowRenderer.

Re VirtualScroll and one column per row: that's not the outside-world view of the most likely content - a table.

I was originally sizing each of my columns individually, since that's what seemed natural with a "ColumnMeasurer". There are multiple columns in a row. As you say, each row is in a single Grid, so from VirtualScroll's, the table columns are under the radar - the row has one column.

I'm guessing this must be getting more annoying than helpful, so I'll stop now. I mainly wanted to get some notes in here for other users who run into the same befuddlements.

@nsuthar0914
Copy link

@bvaughn was having an issue with rowheights while using cellmeasurer with grid and landed here. using flex was the issue. could you please give a hint as how to proceed if i want to use display:flex in cell renderer.

@estaub
Copy link
Author

estaub commented Jan 20, 2017

@nsuthar0914 If you're using any styling outside of the cell seen by cellmeasurer that will affect the sizing of the cell contents, it will not use that styling and therefore be incorrect. In my case, the cell widths were the issue, so I used cellmeasurer just once on each entire row, making sure to include all size-relevant css on the measured row. YMMD, you don't say anything about how you're using flex.

@nsuthar0914
Copy link

@estaub my cells are actually containing another component which uses div's with display:flex, particularly, react-mdl's grid. I did see that if I move styles like fixed height inline, they are being used by cellmeasurer, but adding flex to styles was having no effect. I checked the component being used by "unstable_renderSubtreeIntoContainer" in cellmeasurer, it did have the classes and styles intact, yet the clientHeight there didn't match the computed height in browser.

@nsuthar0914
Copy link

@bvaughn thanks for making an awesome library btw.

@estaub
Copy link
Author

estaub commented Jan 20, 2017

@nsuthar0914 I found it really easy to overlook some style from a parent. Also, cell-measurer itself sets a few relevant parent styles that you may have different. I ended up using Chrome DevTools to compare styles to be sure; see my original post above for details.

@bvaughn
Copy link
Owner

bvaughn commented Jan 20, 2017

Thanks, @nsuthar0914!

And thanks for adding info @estaub.

@misha-panyushkin
Copy link

misha-panyushkin commented Jan 21, 2017

@estaub, @nsuthar0914 you should better use container option and pass it into cell-measurer. I run into a problem of this kind and found easy to pass appropriate container node to handle css styles inheritance for correct height calculations. In my case it was List element itself:

<CellMeasurer
    {...cellMeasurerProps}
    container={() => this.listInstanceRef}
>
    {({ getRowHeight }) => {
        return (
            <List
                ref={ref => this.listInstanceRef = ref}
                rowHeight={getRowHeight}
                {...listProps}
            />
        )
    }}
</CellMeasurer>

@estaub
Copy link
Author

estaub commented Jan 21, 2017

@misha-panyushkin Yes, that seems useful.

@nsuthar0914
Copy link

@misha-panyushkin this.listInstanceRef is undefined in _getContainerNode as it probably runs before the child is actually rendered.

@nsuthar0914
Copy link

@estaub @bvaughn @misha-panyushkin i couldn't get cellmeasurer to give the heights that i wanted the first time, so i simply added another list to get and set heights on componentdidmount and onScroll --

  componentDidMount() {
    this.handleResizeWidth();
  }

  handleResizeWidth() {
    let that = this;
    setTimeout(function() {
      that.itemHeights = that.list.map((listItem, index) => {
        let item = document.getElementById(`item-${index}`);
        if (item) {
          return item.clientHeight;
        } else {
          return null;
        }
      });
      that.cellmeasurer && that.cellmeasurer.resetMeasurements();
      that.grid && that.grid.recomputeGridSize();
    }, 10);
  }

  handleScroll() {
    let heightsToBeUpdated = false;
    this.itemHeights && this.itemHeights.forEach((listItem, index) => {
      let item = document.getElementById(`item-${index}`);
      if (!listItem && !!item) {
        this.itemHeights[index] = item.clientHeight;
        heightsToBeUpdated = true;
      }
    });
    if (heightsToBeUpdated) {
      this.cellmeasurer && this.cellmeasurer.resetMeasurements();
      this.grid && this.grid.recomputeGridSize();
    }
  }

It isn't elegant at all and if i have like 100 of my heavy list item components in the list and scroll too fast, there is flickering. But this will have to do for now, i guess.

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

No branches or pull requests

4 participants