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

[8.0.0-beta2-rev7] When we select cell 0,0 and hide the data clicking on column A with RMB allows us to add new rows #7051

Closed
AMBudnik opened this issue Jun 25, 2020 · 11 comments
Assignees
Labels
bug Context menu Plugin Core: Selection Plugin Hiding Plugin Regression Issues that were created while adding new changes to the source code Status: Merged (ready for release)

Comments

@AMBudnik
Copy link
Contributor

Description

As mentioned in this issue #7050 there is a difference between the menu list of options when cell 0,0 is being selected.

Steps to reproduce

  1. Click with RMB on the header A to open the context menu

Result 8.0.0-beta2-rev16
Zrzut ekranu 2020-06-25 o 14 30 29
https://jsfiddle.net/ph4b78uk/2/ 8.0.0-beta2-rev16
https://jsfiddle.net/ph4b78uk/4/ 8.0.0-beta2-rev 7

Result 7.4.2
742-00-none
https://jsfiddle.net/g4xyLj32/1/ 7.4.2
https://jsfiddle.net/ph4b78uk/3/ 8.0.0-beta2-rev6

Your environment

  • Handsontable version: 8.0.0-beta2-rev7
  • Browser Name and version: Chrome 83
  • Operating System: macOS Catalina
@AMBudnik AMBudnik added bug Context menu Plugin Hiding Plugin Core: Selection Plugin Regression Issues that were created while adding new changes to the source code labels Jun 25, 2020
@budnix budnix self-assigned this Jun 25, 2020
@aninde
Copy link
Contributor

aninde commented Jun 25, 2020

This issue is still broken on merge-candidate for 8.0.0-beta2 PR#7027 https://jsfiddle.net/aninde/u1zmbfkd/

@wszymanski
Copy link
Contributor

Another example, a simpler one:

data: Handsontable.helper.createSpreadsheetData(5, 5),
rowHeaders: true,
colHeaders: true,
hiddenColumns: {
  columns: [1],
  indicators: true,
},
hot.selectCell(2, 1)

Demos did by @aninde:

https://jsfiddle.net/aninde/y64gd5zk 7.4.2
https://jsfiddle.net/aninde/gf2Lctdm 8-beta2-rev16

budnix added a commit that referenced this issue Jun 26, 2020
The header selection generally contains cell selection coordinates. In a
case when all rows (or columns) are hidden that visual coordinates are
translated to renderable coordinates that do not exist. Hence no header
highlight is generated. In that case, to make a column (or a row) header
highlight, the row and column index has point to the header (the
negative value).

Issue: #7051
@budnix
Copy link
Member

budnix commented Jun 26, 2020

This PR #7053 fixes this issue.

budnix added a commit that referenced this issue Jun 29, 2020
Add ability to highlight headers (gray color) when one of the indexes
(row or column) are hidden and the second index is visible.

To support header highlight for that cases the `getCorner` method of the
Selection class is overwritten. There are two approaches to convince the
header Selection to highlight specific headers. One overwrite the method
which controls coordinates for headers (chosen approach for this commit)
and second rewrite/adjust CellCoords, CellRange, and correct all methods
that deal with numbers to support `null` values - to match changes.

Issue: #7051
budnix added a commit that referenced this issue Jun 30, 2020
budnix added a commit that referenced this issue Jun 30, 2020
Issue: #7051

Co-authored-by: Wojciech Szymański <141330+wszymanski@users.noreply.github.com>
budnix added a commit that referenced this issue Jun 30, 2020
* Revert headers selection when all indexes are trimmed

The header selection is restored by passing through the negative values
through the whole library. This braking change forced all tests to be
adjusted.

Issue: #6998

* Separate coordinates context between WoT and HoT

In this commit I separate how the coordinates are used within
Walkontable and Handsontable context. The Walkontable renderer needs to
know about negative coordinates as that module render the whole table.
In The Handsontable context, however only coordinates for cells are
important mostly. As in that context exists cell meta objects which
exists only for cells.

Issue: #6998

* Change getHight and getWidth methods

Change methods in the same manner as was done for "getTopLeftCorner",
"getOuterTopLeftCorner" and similar methods.

Issue: #6998

* Fix getHight, getWidth for selection

Issue: #6998

* Fix alignment and comments ops from context menu

While fixing the negative coordinates I've spotted that alignment and
comments command from context menu doesn't work when the selection was
made in different direction that top-left to bottom-right.

Issue: #6998, #6600

* Fix issues revealed after changing selection methods

Adjust the logic how the selection was processed by the plugins. I
removed all references to negative indexes.

Issue: #6998

* Fix Border context menu command

Add condition for retrieving only cell coordinates (ignoring header)

Issue: #6998

* Fix wrong assumption

The indexes equal to 0 are wrongly excluded from the borders logic.

Issue: #6998

* Fix undoing merge cells and alignment operations

When the header was selected and merged cells or alignment
operations were undoing, an exception was thrown. This was caused
by negative coordinates. I fixed this by excluding negative
coordinates from the process.

Issue: #6998

* Fix eslint complains

Split long line.

Issue: #6998

* Add tests for merge cells and alignment ops

Issue: #6998

* Fix unresponsive AutoFill handler

When the row or column header was selected the AutoFill handler was
unresponsive on filling action. This is caused by negative coordinates.
I fixed this by coords normalization.

Issue: #6998

* Fix selection after autofill operation

Issue: #6998

* Fix eslint complains

Issue: #6998

* Fix editor positioning

When row or column header was selected and enter was hit then the editor
position was out of sync by 1px relative to the cell. I've corrected the
condition in the editors code which includes 1px only for row and column
equals to 0.

Issue: #6998

* Make the logic for showColumn more consistent

Consistent with the counterpart file (showRow.js)

Issue: #6998

* Update src/editors/textEditor.js

Co-authored-by: Wojciech Szymański <141330+wszymanski@users.noreply.github.com>

* Update src/plugins/comments/test/comments.e2e.js

Co-authored-by: Wojciech Szymański <141330+wszymanski@users.noreply.github.com>

* Add ability to clone CellRange instance

Issue: #6998

* Fix unselectable headers when all indexes are hidden

The header selection generally contains cell selection coordinates. In a
case when all rows (or columns) are hidden that visual coordinates are
translated to renderable coordinates that do not exist. Hence no header
highlight is generated. In that case, to make a column (or a row) header
highlight, the row and column index has point to the header (the
negative value).

Issue: #7051

* Add ability to highlight headers for hidden indexes

Add ability to highlight headers (gray color) when one of the indexes
(row or column) are hidden and the second index is visible.

To support header highlight for that cases the `getCorner` method of the
Selection class is overwritten. There are two approaches to convince the
header Selection to highlight specific headers. One overwrite the method
which controls coordinates for headers (chosen approach for this commit)
and second rewrite/adjust CellCoords, CellRange, and correct all methods
that deal with numbers to support `null` values - to match changes.

Issue: #7051

* Improve code readability

Issue: #7051

* Change method name

Issue: #7051

Co-authored-by: Wojciech Szymański <141330+wszymanski@users.noreply.github.com>

Co-authored-by: Wojciech Szymański <141330+wszymanski@users.noreply.github.com>
@AMBudnik
Copy link
Contributor Author

The main issue does not differ for version 8.0.0-beta2-rev18 after merging #7053 https://jsfiddle.net/Lrtehqv9/

@wszymanski 's case solved in 8.0.0-beta2-rev18 https://jsfiddle.net/e2y0fx98/

@wszymanski
Copy link
Contributor

wszymanski commented Jun 30, 2020

I've added my example because it is the main reason why there is no header selection (It hasn't been handled). On Handsontable 7 after a click on the corner, there was a selection on the first element, even it was hidden. Now, click on the header select all cells and all column headers should be highlighted. We talked about it here and @budnix said that he will create a separate issue for it.

Probably this issue will be good for it. I don't know if we should threaten it as a regression now.

FYI: @wojciechczerniak.

The task is not about the click on a corner, just on the header.

@wszymanski
Copy link
Contributor

wszymanski commented Jun 30, 2020

I think that the current behavior of the table is regression. The selectAll method selected all headers (grey highlight) in such a case on the master.

The task is not about the click on a corner, just on the header.

@wszymanski
Copy link
Contributor

wszymanski commented Jul 1, 2020

Related to #7063.

The task is not about the click on a corner, just on the header.

@AMBudnik
Copy link
Contributor Author

AMBudnik commented Jul 1, 2020

I'm sharing additional details as this issue might have caused some misunderstanding.

First of all, as @budnix solved the selection of a header we will compare version 8.0.0-beta2-rev18 https://jsfiddle.net/emja9h6s/ with 7.4.2 https://jsfiddle.net/dnypv40j/

Settings

In this case, we load 4 rows of data and hide them all programmatically via hiddenRows settings.
At the same time, we add selectCell() method on cell 0, 0 (which is hidden).

User action

The only use action is to open a context menu using RMB on header A. And here we can see that before rev7 of 8.0.0-beta2 when we did that actions

  • Insert row above
  • Insert row below

are disabled, in 8.0.0-beta2-rev7 and newer those options are active.

Screenshots

Here's 8.0.0-beta2-rev18
Zrzut ekranu 2020-07-1 o 14 44 23

Here's 7.4.2
Zrzut ekranu 2020-07-1 o 14 43 45

Debugging

afterOnCellMouseDown logs. In both cases when we click the header we get

  • 2nd parameter {row: -1, col: 0}
  • 3rd paramteter <th class=​"ht__highlight">​…​</th>​

Let me know if you need a further investigation on this case.

@aninde
Copy link
Contributor

aninde commented Jul 7, 2020

Case 4

We can not add anything from the corner (top-left element)

https://jsfiddle.net/aninde/48z0jkt3/ 8.0.0-beta2-rev19
https://jsfiddle.net/aninde/o5y19s6m/ v7.4.
Screenshot 2020-07-07 at 13 35 06
Screenshot 2020-07-07 at 13 34 54

budnix added a commit that referenced this issue Jul 9, 2020
This PR fixes a bug that made it impossible to select row or column
header using RMB. This happens when under the clicked row or column
header there was selected cell.

The commit fixes the bug by removing code residue after introducing
support for negative coordinates (true headers selection).

Additionally, I found that our test helper which emulates mouse events
contains wrong logic. The helper supports "which" property which is
deprecated. I've replaced to "button" prop.

Issue: #7051
budnix added a commit that referenced this issue Jul 9, 2020
Issue: #7051
budnix added a commit that referenced this issue Jul 10, 2020
This PR fixes a bug that made it impossible to select a row or column 
header using RMB. This happens when under the clicked row or column
header there was selected cell.

The PR fixes the bug by removing unnecessary code that was needed
before the negative coordinates feature (#7027).

Additionally, I found that our test helper which emulates mouse events
contains the wrong logic. The helper supports "which" property which
is deprecated. I've replaced to "button" prop.

Issue: #7051
@aninde
Copy link
Contributor

aninde commented Jul 10, 2020

Case 1

solved #7051 (comment)

The original demo had incorrect classes. https://jsfiddle.net/aninde/c5m21px0/ - broken 8.0.0-rev16

This case is solved by 8.0.0-beta2-rev22 https://jsfiddle.net/aninde/09kg8o6q/.
The header is selected. We can't add any row from the header. Lack of bottom border is reported as a
regression.

Screenshot 2020-07-10 at 16 22 32

Case 2

still works well #7051 (comment)
@wszymanski case is still fixed like it was with rev18 https://jsfiddle.net/aninde/Lasu23rg/ in rev22

Case 3

still works well #7051 (comment)

Still works well with rev22 https://jsfiddle.net/aninde/g7bpy2uj/
afterOnCellMouseDown logs look similar after clicking A header.

  • 2nd parameter {row: -1, col: 0}
  • 3rd paramteter <th class=​"ht__highlight ht__active_highlight">​…​​

Case 4

I need to check assumptions
#7051 (comment)
https://jsfiddle.net/aninde/yz2r49en/ - rev22
Screenshot 2020-07-10 at 16 40 24

@aninde
Copy link
Contributor

aninde commented Jul 10, 2020

Ok, Case 4 is correct with assumptions described #7082 (comment) so I'm closing this regression.
Screenshot 2020-07-10 at 16 53 08

@aninde aninde closed this as completed Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Context menu Plugin Core: Selection Plugin Hiding Plugin Regression Issues that were created while adding new changes to the source code Status: Merged (ready for release)
Projects
None yet
Development

No branches or pull requests

4 participants