Add z-index to fixedRowsBottom #6269

pnowak · 2019-09-17T09:40:40Z

Context

After added z-indexes to selection borders (class .current, .area and .fill), and after scroll over fixedRowsBottom selection borders overlap to fixed area.
~~Honestly, z-index: 12 is enough to .ht_clone_bottom class. But I decided to use the existing place (clones class) and values to keep it easier to manage.~~

How has this been tested?

I tested with different cell types.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature or improvement (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Additional language file or change to the existing one (translations)

Related issue(s):

Checklist:

My code follows the code style of this project,
My change requires a change to the documentation.

wojciechczerniak · 2019-09-18T07:13:54Z

I know this is out of the scope for the linked issue, but while we're near z-indexes for overlays there is one more issue when there is more fixed bottom rows:

https://jsfiddle.net/tohnkvsd/

It's because .ht_clone_bottom_left_corner and .ht_clone_top_left_corner have the same z-index but bottom one is later in DOM structure. We should split those two and set z-index: 108 for the top header.

And BTW. if I remember correctly @budnix removed debug overlay. Can we clean up .ht_clone_debug class from CSS @budnix ?

pnowak · 2019-09-18T10:19:17Z

Do we want this appearance?

wojciechczerniak · 2019-09-18T10:21:32Z

No. Header should be always at the top.

…clone debug #5947

warpech · 2019-09-20T09:15:03Z

I think that this PR is a good occasion to change src/editors/_baseEditor.js to set zIndex using a CSS class instead of inline style. This will help to cement the behavior and make it clearer where does it come from.

For example, there could be classes:

.displayLayer--low {
 z-index: 101;
}

.displayLayer--medium {
 z-index: 102;
}

.displayLayer--high {
 z-index: 103;
}

And then just add these classes dynamically on .ht_clone_* and .handsontableInputHolder. WDYT?

…e editor #5947

wojciechczerniak · 2019-09-26T15:35:59Z

@warpech There's a lot more z-indexes that we have to cover.

z-index	feature
999	editor ui
1x4	editor input
1x3	floating objects
1x2	selection
1x1	custom borders
1x0	overlay

Where x is an overlay

z-index	overlay
180	top left corner
170	top right corner
160	top
150	bottom left corner
140	bottom right corner
130	bottom
120	left
110	right
100	master

Features like borders, selection, floating object and editors should hide under the fixed overlays that are above the layer the feature is located on.

warpech · 2019-09-26T20:09:58Z

I am glad to hear that. This means that there should never be two elements with the same zIndex anymore, right?

Anyway, my point was that it would improve the code quality to cover all these combinations using CSS class names, and never set style.zIndex directly in code.

It seems to me that this style was implemented in dd7c131, so I am happy now 😊

wojciechczerniak · 2019-09-27T06:14:04Z

src/editors/_baseEditor.js

@@ -371,20 +371,28 @@ class BaseEditor {
   *
   * @returns {string}
   */
-  getEditedCellsZIndex() {
+  getEditedCellsZIndexClass() {


This method is not used anywhere? Then we should remove unused code.

I'm not sure if the classes here are correct. They would give the editor same z-index as overlay.

For now, this method isn't used because we decided to permanent z-index for editor in every overlay.
We will use it in the next step when we implement separate editor UI for editor input. This method has only one goal - return appropriate overlay sections to the editor. And then we are going to add 4 to base z-index.

…tedCellsZIndexClass method #5947

wszymanski

Please take a look at how the editor looks like.

edit: I'm sorry, it works. The first click shouldn't open the editor.

wszymanski · 2019-09-30T11:00:00Z

src/css/handsontable.css

+  z-index: -1;
+}
+
+.ht_editor_show {


I would call this class as ht_editor_shown.

Maybe "visible" would be better?

wszymanski · 2019-09-30T11:00:10Z

src/css/handsontable.css

@@ -585,21 +585,48 @@ CheckboxRenderer
  background: #eee;
 }

-.ht_clone_top {
-  z-index: 101;
+.ht_editor_hide {


I would call this class as ht_editor_hidden.

wszymanski · 2019-09-30T11:19:55Z

src/editors/textEditor.js

+    arrayFilter(childNodes, (childNode) => {
+      if (hasClass(childNode, 'handsontableEditor')) {
+        hasClassHandsontableEditor = true;
+      }


Maybe use:

Array.find or

rangeEach with returning false when the class will be found (It would break the loop.).

handsontable/src/helpers/number.js

Lines 35 to 37 in 98878a6

if (iteratee(index) === false) {

break;

}

For me, arrayFilter isn't the best choice.

wszymanski · 2019-09-30T11:21:06Z

src/editors/selectEditor.js

@@ -66,6 +68,10 @@ class SelectEditor extends BaseEditor {
  close() {
    this._opened = false;
    this.select.style.display = 'none';
+
+    if (hasClass(this.select, 'ht_editor_show')) {


Maybe all those used classed could be moved to constants on the top of the file or event to the helper?

wszymanski · 2019-09-30T11:23:59Z

src/editors/textEditor.js

+     * @private
+     * @type {string}
+     */
+    this.zIndexClass = void 0;


I would call this variable as layerClass or similarly. For me, it would better call the described being, not just it's feature.

Add z-index to ht_clone_bottom class #5947

4260ede

pnowak requested a review from wszymanski September 17, 2019 09:40

pnowak assigned wszymanski Sep 17, 2019

pnowak assigned pnowak and unassigned wszymanski Sep 18, 2019

wojciechczerniak mentioned this pull request Sep 18, 2019

fixedRowsBottom throwing errors when height is too short #5907

Closed

Increased z-index in clone top and clone top left corner and removed …

e2ee03d

…clone debug #5947

pnowak assigned wszymanski and pnowak and unassigned pnowak and wszymanski Sep 18, 2019

Fixed z-indexes to all combinations fixed area #5947

7e2971e

pnowak assigned wszymanski and unassigned pnowak Sep 18, 2019

warpech mentioned this pull request Sep 21, 2019

Improve custom borders performance (SVG) #6157

Closed

6 tasks

wszymanski assigned pnowak and unassigned wszymanski Sep 24, 2019

pnowak added 2 commits September 26, 2019 14:23

Changed z-index for overlay, and used permanent z-index for the activ…

dd7c131

…e editor #5947

Removed unused zIndex #5947

1a59c97

pnowak assigned wszymanski and unassigned pnowak Sep 26, 2019

wojciechczerniak reviewed Sep 27, 2019

View reviewed changes

pnowak added 2 commits September 27, 2019 10:47

Only handsontableEditor use ht_editor_show class otherwise use getEdi…

5d4f0ac

…tedCellsZIndexClass method #5947

Fixed jsdoc for zIndexClass #5947

2a3b8eb

wszymanski suggested changes Sep 30, 2019

View reviewed changes

wszymanski assigned pnowak and unassigned wszymanski Sep 30, 2019

After CR: rename, use constants and rangEach #5947

fe1ceeb

pnowak assigned wszymanski and unassigned pnowak Oct 1, 2019

wszymanski assigned pnowak and unassigned wszymanski Oct 3, 2019

wszymanski approved these changes Oct 3, 2019

View reviewed changes

pnowak merged commit 69410a7 into develop Oct 3, 2019

pnowak deleted the feature/issue-5947 branch October 3, 2019 14:10

pnowak mentioned this pull request Oct 15, 2019

Increase z-index in resize and move area #6342

Merged

6 tasks

pnowak mentioned this pull request Nov 4, 2019

Remove z-index from handsontableInputHolder and add it to handsontableEditor #5375

Closed

6 tasks

pnowak mentioned this pull request Jan 9, 2020

[2.0.0+] Open editor overlaps elements in clone top element #5105

Closed

AMBudnik added the Status: Released label Aug 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add z-index to fixedRowsBottom #6269

Add z-index to fixedRowsBottom #6269

pnowak commented Sep 17, 2019 •

edited

wojciechczerniak commented Sep 18, 2019

pnowak commented Sep 18, 2019

wojciechczerniak commented Sep 18, 2019

warpech commented Sep 20, 2019

wojciechczerniak commented Sep 26, 2019

warpech commented Sep 26, 2019 •

edited

wojciechczerniak Sep 27, 2019

pnowak Sep 27, 2019

wszymanski left a comment •

edited

wszymanski Sep 30, 2019

wojciechczerniak Sep 30, 2019

wszymanski Sep 30, 2019

wszymanski Sep 30, 2019

wszymanski Sep 30, 2019

wszymanski Sep 30, 2019 •

edited

Add z-index to fixedRowsBottom #6269

Add z-index to fixedRowsBottom #6269

Conversation

pnowak commented Sep 17, 2019 • edited

Context

How has this been tested?

Types of changes

Related issue(s):

Checklist:

wojciechczerniak commented Sep 18, 2019

pnowak commented Sep 18, 2019

wojciechczerniak commented Sep 18, 2019

warpech commented Sep 20, 2019

wojciechczerniak commented Sep 26, 2019

warpech commented Sep 26, 2019 • edited

wojciechczerniak Sep 27, 2019

Choose a reason for hiding this comment

pnowak Sep 27, 2019

Choose a reason for hiding this comment

wszymanski left a comment • edited

Choose a reason for hiding this comment

wszymanski Sep 30, 2019

Choose a reason for hiding this comment

wojciechczerniak Sep 30, 2019

Choose a reason for hiding this comment

wszymanski Sep 30, 2019

Choose a reason for hiding this comment

wszymanski Sep 30, 2019

Choose a reason for hiding this comment

wszymanski Sep 30, 2019

Choose a reason for hiding this comment

wszymanski Sep 30, 2019 • edited

Choose a reason for hiding this comment

pnowak commented Sep 17, 2019 •

edited

warpech commented Sep 26, 2019 •

edited

wszymanski left a comment •

edited

wszymanski Sep 30, 2019 •

edited