fix(selector): cell range selector was not working when at bottom #331

ghiscoding · 2019-02-02T23:07:57Z

the issue was that if user open the example "example-frozen-columns-and-rows-spreadsheet.html" and he scrolls completely at the bottom of the frozen grid (bottom right) and try to select a cell range, it was not selecting properly

reference issue #329

This is a Work in Progress (DO NOT MERGE YET)
We need to do some more test before merging

- the issue was that if user open the example "example-frozen-columns-and-rows-spreadsheet.html" and he scrolls completely at the bottom of the frozen grid (bottom right) and try to select a cell range, it was not selecting properly

ghiscoding · 2019-02-02T23:14:12Z

@mpegithub could you please test this change and see if that works. I tried locally and it works. I need to do some more tests with few more examples to make sure it doesn't break anything (I need to test with and without frozen grids). The source of the code change is from X-Slickgrid - slick.cellrangeselector.js

@6pac
Please don't merge yet, I need to do more testing and I will probably merge it myself if that is ok with you. Thanks

ghiscoding · 2019-02-05T15:45:42Z

@mpegithub the code change was to remove the if but I think it should stay. However I can see that the previous if was only looking for frozen column, we should probably put back the if but also check for frozen row and/or frozen column. I will take a look at it later.

ghiscoding · 2019-02-06T00:19:46Z

@mpegithub
Could you please try again, I tested on my side with both regular grid and frozen grid, it seems to be all good. Also note that I copied your code suggestion and that worked, however the only different is that the regular grid check should be at the end (after the frozen checks) not at the beginning and that works. Please confirm so that I can merge it soon. Thanks.

Tested with example-excel-compatible-spreadsheet and example-frozen-columns-and-rows-spreadsheet.html

mpegithub · 2019-02-06T10:18:19Z

Hi, I test with some frozen rows and columns then scroll down and/or right => it is not good for me, always the same issue.
Finally I think the problem is maybe not here but in the calculation of thestart point of the selected range dd.range.start, look in handleDragStart function, this part gives a bad start point

      var start = _grid.getCellFromPoint(
        dd.startX - $(_canvas).offset().left,
        dd.startY - $(_canvas).offset().top);

I think these computes ignore the scrolls offset.

mpegithub · 2019-02-06T13:59:28Z

I used your correction but removed else statements in handleDrag function because it is possible to have frozen columns AND frozen rows.

Next I add an event manager for scroll to get scollLeft and scrollTop values. Then, in handleDragStart function, I added these values to the start point coordinates only if frozen columns or frozen rows exist. This works for me with frozenColumn: 0 and frozenRow: 5 for example.

Here is my code :

(function ($) {
  // register namespace
  $.extend(true, window, {
    "Slick": {
      "CellRangeSelector": CellRangeSelector
    }
  });

  function CellRangeSelector(options) {
    var _grid;
    var _currentlySelectedRange;
    var _canvas;
    var _gridOptions;
    var _$activeCanvas;
    var _dragging;
    var _decorator;
    var _self = this;
    var _handler = new Slick.EventHandler();
    var _defaults = {
      selectionCss: {
        "border": "2px dashed blue"
      }
    };

    // Frozen row & column variables
    var _rowOffset;
    var _columnOffset;
    var _isRightCanvas;
    var _isBottomCanvas;

    // Scrollings
    var _scrollTop;
    var _scrollLeft;

    function init(grid) {
      options = $.extend(true, {}, _defaults, options);
      _decorator = options.cellDecorator || new Slick.CellRangeDecorator(grid, options);
      _grid = grid;
      _canvas = _grid.getCanvasNode();
      _gridOptions = _grid.getOptions();
      _handler
        .subscribe(_grid.onScroll, handleScroll)
        .subscribe(_grid.onDragInit, handleDragInit)
        .subscribe(_grid.onDragStart, handleDragStart)
        .subscribe(_grid.onDrag, handleDrag)
        .subscribe(_grid.onDragEnd, handleDragEnd);
    }

    function destroy() {
      _handler.unsubscribeAll();
    }

    function getCellDecorator() {
      return _decorator;
    }

    function handleScroll(e, args) {
      _scrollTop = args.scrollTop;
      _scrollLeft = args.scrollLeft;
    }

    function handleDragInit(e, dd) {
      // Set the active canvas node because the decorator needs to append its
      // box to the correct canvas
      _$activeCanvas = $(_grid.getActiveCanvasNode(e));

      var c = _$activeCanvas.offset();

      _rowOffset = 0;
      _columnOffset = 0;
      _isBottomCanvas = _$activeCanvas.hasClass('grid-canvas-bottom');

      if (_gridOptions.frozenRow > -1 && _isBottomCanvas) {
        _rowOffset = (_gridOptions.frozenBottom) ? $('.grid-canvas-bottom').height() : $('.grid-canvas-top').height();
      }

      _isRightCanvas = _$activeCanvas.hasClass('grid-canvas-right');

      if (_gridOptions.frozenColumn > -1 && _isRightCanvas) {
        _columnOffset = $('.grid-canvas-left').width();
      }

      // prevent the grid from cancelling drag'n'drop by default
      e.stopImmediatePropagation();
    }

    function handleDragStart(e, dd) {
      var cell = _grid.getCellFromEvent(e);
      if (_self.onBeforeCellRangeSelected.notify(cell) !== false) {
        if (_grid.canCellBeSelected(cell.row, cell.cell)) {
          _dragging = true;
          e.stopImmediatePropagation();
        }
      }
      if (!_dragging) {
        return;
      }

      _grid.focus();

      var startX = dd.startX - $(_canvas).offset().left;
      if (_gridOptions.frozenColumn >= 0) {
        startX += _scrollLeft;
      }

      var startY = dd.startY - $(_canvas).offset().top;
      if (_gridOptions.frozenRow >= 0) {
        startY += _scrollTop;
      }

      var start = _grid.getCellFromPoint(
        // dd.startX - $(_canvas).offset().left,
        // dd.startY - $(_canvas).offset().top);
        startX, startY);

      dd.range = { start: start, end: {} };
      _currentlySelectedRange = dd.range;
      return _decorator.show(new Slick.Range(start.row, start.cell));
    }

    function handleDrag(e, dd) {
      if (!_dragging) {
        return;
      }
      e.stopImmediatePropagation();

      var end = _grid.getCellFromPoint(
        e.pageX - _$activeCanvas.offset().left + _columnOffset,
        e.pageY - _$activeCanvas.offset().top + _rowOffset
      );

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

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

      // ... or regular grid (without any frozen options)
      if (!_grid.canCellBeSelected(end.row, end.cell)) {
        console.log("1");
        return;
      }

      dd.range.end = end;

      _decorator.show(new Slick.Range(dd.range.start.row, dd.range.start.cell, end.row, end.cell));
    }

    function handleDragEnd(e, dd) {
      if (!_dragging) {
        return;
      }

      _dragging = false;
      e.stopImmediatePropagation();

      _decorator.hide();
      _self.onCellRangeSelected.notify({
        range: new Slick.Range(
          dd.range.start.row,
          dd.range.start.cell,
          dd.range.end.row,
          dd.range.end.cell
        )
      });
    }

ghiscoding · 2019-02-06T14:24:02Z

@mpegithub
Ah right, good catch, I did add the else in front of each because I thought it was sufficient for the return. Anyhow, I removed the else, can you re-test again? The 2 examples I tested still works correctly on my side. Thanks

mpegithub · 2019-02-06T14:55:15Z

Which examples ? Do you test with frozen column AND frozen rows then scroll to right and down in the right bottom part of the grid ?

ghiscoding · 2019-02-06T14:58:40Z

Ah sorry I wrote in earlier post

Tested with example-excel-compatible-spreadsheet and example-frozen-columns-and-rows-spreadsheet.html

So 1 has frozen rows/columns and the other is a regular grid. Those are mainly the only few examples which have the cell range enabled.

mpegithub · 2019-02-06T15:17:41Z

I don't understand, I tried with example-frozen-columns-and-rows-spreadsheet.html example and it's not good for me :

in the right bottom part of the grid
scroll to the right
scroll down
make a selection
=> start point of selection is bad

ghiscoding · 2019-02-06T15:28:54Z

damn you are right again, I forgot to scroll. The scrolling does screw it up. Then I don't know what else to try and I'm at work so my time is a bit limited.

If I look at the 2 repos, Jlynh7 and X-Slickgrid (which is based of JLynch) have different approach but trying both and the issue still persist. All the code that I use to implement the frozen feature in this lib comes from the X-Slickgrid repo since it was the most used and last updated.

mpegithub · 2019-02-06T15:48:45Z

Have you look at my last proposition with an handle on scroll event in earlier post ?

ghiscoding · 2019-02-06T15:52:43Z

You mean your post here? I thought you only removed the else, now I just noticed you changed a few things after trying to copy paste that code. However, after copying the code I don't see any selection happening anymore, ~~so that doesn't seem to help~~ .,.. ahh wait, your code is not complete, there was a few lines that are missing, basically the last 10-15 lines. It does seem to work now with that. I will have to test it out more but later today.

mpegithub · 2019-02-06T16:03:03Z

The following modifications are working for me :

add two vriables

    // Scrollings
    var _scrollTop;
    var _scrollLeft;

handle scroll event in init function :

    function init(grid) {
      options = $.extend(true, {}, _defaults, options);
      _decorator = options.cellDecorator || new Slick.CellRangeDecorator(grid, options);
      _grid = grid;
      _canvas = _grid.getCanvasNode();
      _gridOptions = _grid.getOptions();
      _handler
        .subscribe(_grid.onScroll, handleScroll)    // <<<<<
        .subscribe(_grid.onDragInit, handleDragInit)
        .subscribe(_grid.onDragStart, handleDragStart)
        .subscribe(_grid.onDrag, handleDrag)
        .subscribe(_grid.onDragEnd, handleDragEnd);
    }

create an handle function for scroll event to get scroll offsets :

   function handleScroll(e, args) {
      _scrollTop = args.scrollTop;
      _scrollLeft = args.scrollLeft;
    }

modification of the start point of the selection in handleDragStart function :

    function handleDragStart(e, dd) {
      var cell = _grid.getCellFromEvent(e);
      if (_self.onBeforeCellRangeSelected.notify(cell) !== false) {
        if (_grid.canCellBeSelected(cell.row, cell.cell)) {
          _dragging = true;
          e.stopImmediatePropagation();
        }
      }
      if (!_dragging) {
        return;
      }

      _grid.focus();

      // >>>>>
      var startX = dd.startX - $(_canvas).offset().left;
      if (_gridOptions.frozenColumn >= 0) {
        startX += _scrollLeft;
      }

      var startY = dd.startY - $(_canvas).offset().top;
      if (_gridOptions.frozenRow >= 0) {
        startY += _scrollTop;
      }

      var start = _grid.getCellFromPoint(startX, startY);
      // <<<<<

      dd.range = { start: start, end: {} };
      _currentlySelectedRange = dd.range;
      return _decorator.show(new Slick.Range(start.row, start.cell));
    }

keep code for handleDrag function :

    function handleDrag(e, dd) {
      if (!_dragging) {
        return;
      }
      e.stopImmediatePropagation();

      var end = _grid.getCellFromPoint(
        e.pageX - _$activeCanvas.offset().left + _columnOffset,
        e.pageY - _$activeCanvas.offset().top + _rowOffset
      );

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

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

      // ... or regular grid (without any frozen options)
      if (!_grid.canCellBeSelected(end.row, end.cell)) {
        console.log("1");
        return;
      }

      dd.range.end = end;

      _decorator.show(new Slick.Range(dd.range.start.row, dd.range.start.cell, end.row, end.cell));
    }

ghiscoding · 2019-02-06T16:06:27Z

If that the same code as your previous post?

mpegithub · 2019-02-06T16:11:52Z

It seems that the end of code is missing in my previous post..

See below the full file :

slick.cellrangeselector.js.txt

ghiscoding · 2019-02-06T16:13:20Z

I pushed the changes from that earlier post you had. I tried both examples and also scroll in both, it seems to work correctly now. Can you test on your side.

mpegithub · 2019-02-06T16:15:21Z

I tested, it's ok for me.

ghiscoding · 2019-02-06T16:17:14Z

Great, thanks for all your investigation, so are you ok if I merge the code now?

mpegithub · 2019-02-06T16:17:57Z

Yes.

mpegithub · 2019-02-06T16:19:51Z

Just a question, do you know if it is possible to make a selection using the shift key ? Do I miss an option of the plugin ?

ghiscoding · 2019-02-06T16:21:26Z

Hmm I don't think using shift would make any difference, this is a range selector so what are you expecting the shift to do?

mpegithub · 2019-02-06T16:24:48Z

Click on top left cell, press and maintain shift key, then click on another cell to make the selection.
It can be very useful if you want to make a big selection after using the scrollbar for example...
Not sure I am very clear sorry !

ghiscoding · 2019-02-06T16:25:21Z

The range selector uses the jquery drag, so I don't see the shift working with this, I assume it will be disregarded

mpegithub · 2019-02-06T16:26:15Z

Ok, thanks for all !

ghiscoding · 2019-02-06T16:34:17Z

I should say thanks to you since you're the one who provided the fix 😉

ghiscoding · 2019-02-07T17:04:09Z

@mpegithub
I think we forgot to test with only 1 frozen option (row or column) enabled and it's causing the selection to not work. See new opened issue #336

hmmm this is strange, I get errors thrown up in the console which were not showing before merging the PR. I don't think other PRs caused this issue though since I tried reverting manually some of the change and that has no effect.

Found the issue, the 2 new variables scrollTop and scrollLeft were not initialized. Our test was always to scroll at the bottom to test your reported error but we didn't try just not scrolling. It's fixed now in PR #337

ghiscoding · 2019-02-07T20:13:34Z

@mpegithub
Found a second small issue, see PR #338 with animated gif.
Basically we should add to startX or startY only when we are actually in the frozen area.
I'd like to merge this by tomorrow as I would like to have a new NPM release this week.

ghiscoding mentioned this pull request Feb 2, 2019

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

Closed

Ghislain Beaulac added 2 commits February 5, 2019 19:13

fix(selector): cell range selector work with frozen and regular grid

edd52cf

refactor(selector): simplify the code and fix indentation and format

e2d2be9

refactor(selector): remove else condition but keep regular if

71ece1d

ghiscoding changed the title ~~WIP - fix(selector): cell range selector was not working when at bottom~~ fix(selector): cell range selector was not working when at bottom Feb 6, 2019

refactor(selector): change code as per comment in PR

512f704

ghiscoding merged commit dcf8194 into master Feb 6, 2019

ghiscoding deleted the bugfix/frozen-cell-range-selector branch February 6, 2019 21:33

ghiscoding mentioned this pull request Feb 7, 2019

Enalbling frozenColumn disables cell range selection #336

Closed

ghiscoding mentioned this pull request Feb 7, 2019

fix(selector): cell selector throws error in console when no scroll #338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(selector): cell range selector was not working when at bottom #331

fix(selector): cell range selector was not working when at bottom #331

ghiscoding commented Feb 2, 2019

ghiscoding commented Feb 2, 2019 •

edited

Loading

ghiscoding commented Feb 5, 2019

ghiscoding commented Feb 6, 2019 •

edited

Loading

mpegithub commented Feb 6, 2019 •

edited

Loading

mpegithub commented Feb 6, 2019 •

edited by ghiscoding

Loading

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019 •

edited

Loading

mpegithub commented Feb 6, 2019 •

edited

Loading

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019 •

edited

Loading

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019 •

edited

Loading

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019 •

edited

Loading

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019

ghiscoding commented Feb 7, 2019 •

edited

Loading

ghiscoding commented Feb 7, 2019

fix(selector): cell range selector was not working when at bottom #331

fix(selector): cell range selector was not working when at bottom #331

Conversation

ghiscoding commented Feb 2, 2019

ghiscoding commented Feb 2, 2019 • edited Loading

ghiscoding commented Feb 5, 2019

ghiscoding commented Feb 6, 2019 • edited Loading

mpegithub commented Feb 6, 2019 • edited Loading

mpegithub commented Feb 6, 2019 • edited by ghiscoding Loading

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019 • edited Loading

mpegithub commented Feb 6, 2019 • edited Loading

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019 • edited Loading

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019 • edited Loading

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019 • edited Loading

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019

mpegithub commented Feb 6, 2019

ghiscoding commented Feb 6, 2019

ghiscoding commented Feb 7, 2019 • edited Loading

ghiscoding commented Feb 7, 2019

ghiscoding commented Feb 2, 2019 •

edited

Loading

ghiscoding commented Feb 6, 2019 •

edited

Loading

mpegithub commented Feb 6, 2019 •

edited

Loading

mpegithub commented Feb 6, 2019 •

edited by ghiscoding

Loading

ghiscoding commented Feb 6, 2019 •

edited

Loading

mpegithub commented Feb 6, 2019 •

edited

Loading

ghiscoding commented Feb 6, 2019 •

edited

Loading

mpegithub commented Feb 6, 2019 •

edited

Loading

ghiscoding commented Feb 6, 2019 •

edited

Loading

ghiscoding commented Feb 7, 2019 •

edited

Loading