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

[svg] Loss of right border of rowHeader while scrolling when fixedRowsTop and fixedRowsBottom are enabled #6646

Closed
aninde opened this issue Jan 16, 2020 · 6 comments
Assignees
Labels
bug Fixing / Freezing Plugin Mobile Regression Issues that were created while adding new changes to the source code Scroll

Comments

@aninde
Copy link
Contributor

aninde commented Jan 16, 2020

Description

During the second horizontal scrolling, we lose the right border rowHeaders when fixedRowsTop and fixedRowsBottom are enabled. Only the border that is not associated with fixed is not displayed.

I scrolled it with the Apple Magic mouse, without clicking or losing focus. That can be reproduced with TrackPad or by pressing the scrolling wheel and moving the mouse sideways in a typical mouse.

Steps to reproduce

  1. Open demo https://jsfiddle.net/aninde/eq263gvh/ 8.0.0-svg-#PR6157 v.
  2. Scroll it horizontal two times

Actual result:

Right rowHeader is missing
Screenshot 2020-01-16 at 12 08 44
right-header-border-ff-6157

Expected result:

All rowHeaders should be visible while scrolling in each direction.
https://jsfiddle.net/aninde/q7k6cmz8/ 7.3.0 v. fixed
Screenshot 2020-01-16 at 12 09 35

Demo

https://jsfiddle.net/aninde/eq263gvh/ 8.0.0-svg-#PR6157 v. broken
https://jsfiddle.net/aninde/q7k6cmz8/ 7.3.0 v. fixed

Your environment

  • Handsontable version: 8.0.0-svg-PR#6157
  • Browser Name and version: Firefox, Safari, Chrome
  • Operating System: macOS
@aninde aninde added bug Fixing / Freezing Plugin Regression Issues that were created while adding new changes to the source code labels Jan 16, 2020
@aninde
Copy link
Contributor Author

aninde commented Jan 16, 2020

This error did not appear in the first stage of testing #6157. It can't be replicated on my builds from late November and early December.

@warpech warpech self-assigned this Jan 20, 2020
@warpech
Copy link
Member

warpech commented Jan 22, 2020

The problem is quite complex. I introduced this bug as part of my refactor in #6594 and it was not caught by automated tests.

Moreover, the problem seems fixed if I remove an ugly, untested 3-year old workaround (c315eaf), however the manual test proves that then the 3-year old problem #3850 returns.

The only solution I can propose is to do significant improvements to the classes innerBorderLeft, innerBorderTop. It will help if I do #6651 first, because it introduces proper path clipping using SVG.

Preliminary work was done in the branch https://github.com/handsontable/handsontable/tree/feature/issue-6064-disappearing-border (4774977)

@warpech
Copy link
Member

warpech commented Jan 23, 2020

It was agreed yesterday between @wojciechczerniak, @aninde and me, that this regression will be fixed AFTER #6157 is merged.

@aninde
Copy link
Contributor Author

aninde commented Jan 25, 2020

This bug also appears on mobile: iPads and smartphones. Tested on macOS and Android.

warpech added a commit that referenced this issue Jan 29, 2020
…f top and left overlays (#6646)

This commit removes .innerBorderLeft, .innerBorderTop classes, which were added dynamically depending on the scroll position and 
the configuration of the headers. These classes were a source of many needs for workarounds.

One such workaround was in a commit made 3 years ago: c315eaf, 
which was unfortunate in the terms of architecture. The external settings object was modified in a class constructor. 
First and foremost, Walkontable should treat that object as read-only.

The original problem #3850, which was addressed by c315eaf, is now fixed by 
the removal of .innerBorderLeft, .innerBorderTop.

After this change, the borders are rendered on the overlay, even if there are only header cells and no fixed cells. 
The line is, however, not visible, because the right padding excludes lines that are directly on the header line. 
This is a difference from what is expected in #6651 (comment). 
I will fix that in the consecutive commits.
warpech added a commit that referenced this issue Feb 18, 2020
Bugfix #6646 (Loss of right border of rowHeader while scrolling when fixedRowsTop and fixedRowsBottom
are enabled). The solution was to remove various factors that caused varied size of headers.
There are no longer classes `innerBorderTop` and `innerBorderLeft`. This change simplified the code,
but could have affected various plugins, such NestedHeaders. All plugins should be tested to be sure
that there are no regressions.

Bugfix #6673. First column is now 1px wider than before (as reported by outerWidth), to accommodate for
the fact that it draws border on the left.

Bugfix #6671 (selection flickers when autofill is on last stretched column). The cause of the problem
was that the corner position did not take into the account the scrollbar width

Feature #6651. The border is rendered on the gridline according to the new specification, and the frozen
line has a new color #5d6365.

Additional changes:

1. in `walkontable.css`, change `line-height` of `.colHeader` to a slightly smaller value that results in
the same row-height in Firefox as in Chrome on Windows

2. in `walkontable.css`, removed declaration of default <col> width 50px. Now, the same value comes from
Walkontable setting `defaultColumnWidth`. The only way to overwrite it is by providing the new value in the JS
settings object. This is more consistent.

3. make AutoColumnSize results deterministic across all tested browsers. In tests, use `toEqual` instead
of `arrayContaining` or `toBeAroundValue`.

4. in HeaderTooltips, hardcode the text width threshold for the `onlyTrimmed` to 50px. This change is
explained in a comment. Related issue: #6708

5. fix rendering of GhostTable. Before this commit, GhostTable was sometimes rendered without borders
on all sides

6. in all places where 1px is added or subtracted to accommodate for the gridline, use a constant
GRIDLINE_WIDTH defined in `utils/gridline.js`

7. in frozen line tests, change `elem.innerText` to `elem.textContent`, because the former was sometimes
empty in Safari

8. performance: `getCellFn` is now provided as configuration to `BorderRenderer`, instead of as
a parameter to `BorderRenderer.render`
@warpech
Copy link
Member

warpech commented Feb 18, 2020

Fixed in 59b71c2, as part of the SVG borders PR (#6157).

@AMBudnik AMBudnik changed the title [8.0.0-svg] Loss of right border of rowHeader while scrolling when fixedRowsTop and fixedRowsBottom are enabled [svg] Loss of right border of rowHeader while scrolling when fixedRowsTop and fixedRowsBottom are enabled May 11, 2020
@AMBudnik AMBudnik added the Scroll label Jul 6, 2020
@krzysztofspilka
Copy link
Member

This is an issue reported for the SVG borders project, which was rejected and won't be implemented in Handsontable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixing / Freezing Plugin Mobile Regression Issues that were created while adding new changes to the source code Scroll
Projects
None yet
Development

No branches or pull requests

4 participants