Incorrect cell selection if a frozen exists and the grid is scrolled down

[mpegithub]

Hello,
In this example example-frozen-columns-and-rows-spreadsheet.html,
and in the bottom grid, if the grid is scrolled down and a selection is made, this one is incorrect: it looks like the number of scrolled lines are added to the selection...

[mpegithub]

Note: it's ok here http://jlynch7.github.io/SlickGrid/examples/example-frozen-columns-and-rows-spreadsheet.html
(v2.2)

[mpegithub]

I found a patch doing the job for me : in slick.cellrangeselector.js, function handleDrag

If not defined _gridOptions.frozenColumn = _gridOptions.frozenRow = -1 ...

Replace :

     if ( (!_grid.canCellBeSelected( end.row, end.cell ) ) 
          || ( !_isRightCanvas && ( end.cell > _gridOptions.frozenColumn ) )
          || ( _isRightCanvas && ( end.cell <= _gridOptions.frozenColumn ) )
          || ( !_isBottomCanvas && ( end.row >= _gridOptions.frozenRow ) )
          || ( _isBottomCanvas && ( end.row < _gridOptions.frozenRow ) )
        ) {
         return;
        }

By :

        if (!_grid.canCellBeSelected(end.row, end.cell)) {
          return;
        }

        // when having frozen column(s), we need to do extra checks
        if (_gridOptions.frozenColumn >= 0) {
          if ((!_isRightCanvas && (end.cell > _gridOptions.frozenColumn)) ||
            (_isRightCanvas && (end.cell <= _gridOptions.frozenColumn))
          ) {
            return;
          }
        }

        // when having frozen row(s), we need to do extra checks 
        if (_gridOptions.frozenRow >= 0) {
          if ((!_isBottomCanvas && (end.row >= _gridOptions.frozenRow)) ||
            (_isBottomCanvas && (end.row < _gridOptions.frozenRow))
          ) {
            return;
          }
        }

[ghiscoding]

I guess it could be simplified as

if (!_grid.canCellBeSelected(end.row, end.cell)) {
   return;
}

// when having frozen column(s), we need to do extra checks
if (_gridOptions.frozenColumn >= 0) {
  if (
   ((!_isRightCanvas && (end.cell > _gridOptions.frozenColumn)) || (_isRightCanvas && (end.cell <= _gridOptions.frozenColumn)))
   || ((!_isBottomCanvas && (end.row >= _gridOptions.frozenRow)) || (_isBottomCanvas && (end.row < _gridOptions.frozenRow)))
  ) {
   return;
 }
}

[mpegithub]

Not sure, we need to test frozenColumn AND frozenRow, what about if _gridOptions.frozenRow is not defined (= -1) ?

[ghiscoding]

ahh I see, you have separated the 2 types of frozen (columns/rows), I didn't realize that since both comments were very similar. Anyhow in both cases you want to return ASAP, so I don't think there's any performance difference in returning early (all in 1 big if). The way I wrote is 1 or the other, if both your if into 1 bigger if, though I didn't test anything.

Ah wait I see what you mean, I thought the if (_gridOptions.frozenColumn >= 0) was used 2x times but the next one is actually different with if (_gridOptions.frozenRow >= 0). Then I guess your code is ok but maybe make the comments a bit more obvious because I didn't see the difference while doing a quick look at it lol

[mpegithub]

Yes I agree, maybe it can be useful to change the comment..

A proposal:

if (!_grid.canCellBeSelected(end.row, end.cell)) {
    return;
}

// we need to do extra checks when having...

// ... frozen column(s), 
if (_gridOptions.frozenColumn >= 0) {
    if ((!_isRightCanvas && (end.cell > _gridOptions.frozenColumn)) ||
        (_isRightCanvas && (end.cell <= _gridOptions.frozenColumn))
    ) {
        return;
    }
}

// ... or frozen row(s)
if (_gridOptions.frozenRow >= 0) {
    if ((!_isBottomCanvas && (end.row >= _gridOptions.frozenRow)) ||
        (_isBottomCanvas && (end.row < _gridOptions.frozenRow))
    ) {
        return;
    }
}

[ghiscoding]

yes this looks much better after a quick view at it. Have you tried your code with regular grid? I assume it's ok but we'll have to test that too. Can you make a PR for this and take the credits or you want us to do it?

[mpegithub]

What do you mean by regular grid ? Without frozen items ?
No matter of who take the credits.

[ghiscoding]

Yes regular grid without any frozen options set. I think it's fine but we need to test a few options to make sure nothing breaks.

If you could make the PR that would be great, but if don't know how or you can't do it then let us know and we'll take it

[mpegithub]

I understand, it seems to be ok for regular grids.
Sorry for the PR but i am a newbie here, thank you to do the job for me ;)

[ghiscoding]

ok might take a few days to look into this and testing it

[ghiscoding]

@mpegithub I tried your changes and that didn't work, however I went back to the source of what I copied it from X-Slickgrid (which was my source to bring Frozen feature). Can you please go over PR #331 and test it out, you can continue conversion there. Thanks for feedback

[mpegithub]

Not working for me, sorry.

In my case I have no frozen row so :

1. _gridOptions.frozenRow = -1 => end.row is always >= _gridOptions.frozenRow
2. active canvas is Not bottom canvas => !_isBottomCanvas is always true

=> row 114 is always true : ( !_isBottomCanvas && ( end.row >= _gridOptions.frozenRow ) )
=> always return

Or maybe I miss something !

[ghiscoding]

@mpegithub
I pushed a fix, could you test the new fix and continue the conversation in the PR #331
Thanks for all the feedback

[ghiscoding]

Closed by PR #331