Rewrite custom borders to use SVGs insead of DIVs #6467
Comments
|
I would also add
and remember to check all the browsers and go locally ;) |
|
@wojciechczerniak @AMBudnik thank you very much for preparing the checklist. Also, I would like to check custom borders with:
|
|
Due to better readability I will report bugs related to Case 1Jumping selection #6157 (comment) Case 2Leaking fill handle in overlay Chrome, Safari, Firefox / MacOs Pro, Regression. Steps to reproduction
Result Notice that the blue square of fileHandle is rectangle, not square and is cutted while scrolling Case 3Overlaying right border on frozen columns while scrolling. SVG Branch - Firefox 70, locally 7.2.2 Firefox, jsfiddle Steps to reproduction
Result: Pink, the right border is overlaying fixed columns. Selection is doubled and moved. Case 4Overlaying top border on frozen rows while scrolling. It works worst in the Firefox, but even in Chrome and Safari doesn't look good.
Firefox SVG Branch Chrome SVG Branch EDIT: All cases described in this comment also occurs on IE/Real Edge/Firefox on Windows 10. |
Case 5Border colors with spaces
|
Case 6Border color order #6453 (comment) the color order is not preserved. |
|
I'm not sure about the assumption with zooming and blurriness. On PR description was: I wear glasses so I need someone with healthy eyes to look at it too. @jansiegel @budnix @AMBudnik can you double-check how Below, I paste print screens on the 150% zoom in and zoom into those print to see details.
|
To me they look sharp at multiplies of 100, and a little blurry on other levels, just as @warpech stated in #6157, at least on Chrome, Firefox and Safari. |
…lling fixes Case 1, Case 3 and Case 4 from #6467 (comment)
Summary of testingFound:
Fixed issues that they need to add tests to:
|
…lling fixes Case 1, Case 3 and Case 4 from #6467 (comment)
|
@krzysztofspilka asked me for estimate how much time is needed to finish. Here are my pessimistic estimates (total: up to 9 days).
These are the occurences of the same problem: There is a race condition between border rendering and overlay positioning when main window serves as a scroll container. Additional problem, that seems to be related: in some Walkontable tests, I must call
This might take 1 day.
This might take 1 day.
This might take 1 day.
Can we please move this out of scope of the PR #6157. My rough estimate is that it might take 1 day of work for each test. |
… spaces fixes the problem described in #6467 (comment): customBorders are not rendering in any browser when there is space in color name jsfiddle.net/aninde/bfckronw 7.2.2 with svg borders
|
As said in point 2 of #6157 OP, the I am not sure of the severity of this. Was such possibility ever advertised in the docs? Are our customers using it? If yes, then I think we need to add a proper API for selection customization and explain the migration. |
|
@warpech the demo in your comment is in the stable version 7.2.2 This is how selection looks on your PR with changed CSS. Only square is changed. Is this in line with the assumptions? |
It is in line with my assumptions, yes. I changed the CSS API of the "borders" but did not change the CSS API of the "corners". |
It was: https://handsontable.com/docs/1.17.0/tutorial-styling.html and was removed. I don't have evidence for this, but it might be an important part of the customization process. Everything that is a part of the UI should be customizable in terms of color at least. In #6538 @AMBudnik proposes a feature to keep this feature. I've found one question about it #2487 |
…rolling reproduces the Case 1, Case 3 and Case 4 from #6467 when the following lines are commented out: ``` wt.draw(); // TODO as it turns out, the desired rendering is only visible after the second draw. A problem that does not appear in HOT but appears in raw WOT. Why? ```
because I think that the key in solving the "Case 1", "Case 3" and "Case 4" problems in #6467 (comment) lies in the fact that adjustElementsPosition was done irrespective of redrewClone. the next step will be to move "adjustElementsPosition" before "redrawClone" Note that in this change, I needed to change one test (in "selectAll.spec.js"). I believe that the previous test was actually wrong and the new test fixture is the correct one, because there are less rows drawn.
I made a small PR that implements this: #6593. However there is one pitfall (see in the PR's first comment). |
|
@warpech you are correct. "Case 2" it is not regression, it can be reproduced on 7+ version, my mistake. |
|
New issue explaining a problem discovered in the PR: #6626 (Selection on printed Handsontable) |
without fixed rows this bug do not occur. Lack of the bottom border of colHeader is also in version 7.3.0, which is why I don't report it here. |
+1 on that. We should hide the rest of the selection and white rectangle that covers the grid. But it's out of the scope so up to you. Edit: not only custom border is doubled at the bottom:
|
* refactor: move resetFixedPosition calls to a new method in to the Overlays class because it unnecessarily obfuscates the code in the Table class * refactor: simplify lengthy stack traces by using a separate method to draw Walkontable clones because previously the stack traces appeared to be recursive when Walkontable.draw (for overlay) was called inside Walkontable.draw (for master). from now on, Walkontable.drawClone (for overlay) is called inside Walkontable.draw (for master). this allows me to do refactor further in the following commits * refactor: split the Walkontable class (core.js) into a real Core class and child classes Master, Clone because it allows to further simplify the code of Walkontable.Table * refactor: remove properties `drawn` and `drawInterrupted` from Clone class because it seems that they only have use in Master * refactor: limit the amount of information passed from Overlay to Clone because most of it was not used * refactor: getOverlayName is only needed in Clone, not in Master because in Master it only returns one value deterministically * refactor: the `hasOversizedColumnHeadersMarked` object is only ever populated in case of Master as it was revealed by the previous commit. Hence, it makes sense to flatten the object into a boolean * code style: use the name "overlayRoot" consistently because the same variable is called "overlayRoot" in many other contexts * refactor: merge the methods "adjustRootElementSize" and "adjustRootChildrenSize" because having them separately only obfuscates the code in my opinion, making it hard for me to debug other problems this commit also improves the JSDoc for two functions * change: have just a single instance of "rowUtils" and "columnUtils" for master and all clones because so far every clone had it's own instances of "rowUtils" and "columnUtils", which was bad for memory usage and debugging * refactor: move "rowUtils" and "columnUtils" from Table to Master class because it is more used from other classes (and externally) than from within the Table class * test fix: change the expected path depending on the scrollbar width because the coordinates on Windows are different due to to the different scrollbar width * test: check for the draw position of borders in overlays when fast scrolling reproduces the Case 1, Case 3 and Case 4 from #6467 when the following lines are commented out: ``` wt.draw(); // TODO as it turns out, the desired rendering is only visible after the second draw. A problem that does not appear in HOT but appears in raw WOT. Why? ``` * refactor: rearrange some if(isMaster) conditions because they could be simpler and/or more readable * refactor: use unpacked values of wtViewport and wtOverlays since they already exist in this function, use them consistently * refactor: don't use own instance through an external reference because that's just a waste of CPU cycles * step1 * step 2 - move refreshSelections as the last thing in draw * step 3 - calling syncScrollWithMaster at the end of draw() seems redundant at least no tests fail * refactor: remove "this.instance" aliases of "this.wot" because they are marked as legacy and deprecated since long time * refactor: rename "wtOverlay.wot" to "wtOverlays.master" because it is not obvious what instance does "wot" refer to. There already is one instance referred to as "clone", so it makes sense to refer to the original counterpart as "master" * refactor: rename "clone.cloneSource" to "clone.overlay.master" for the most consistent addressing of "master" WOT instance I can think of * refactor: previous refactor exposed quite redundant way of finding the "master" instance since we are already in an "overlay" class, we know the "master" class is available always as "this.master" * refactor: more consistent naming of a variable because it is already called "master" in many other places * refactor: split out the draw() function used by the master table because it worked way too different compared to overlay tables. Also, in OOP it is a anti-pattern for the parent class to know about the child classes. * refactor: remove the property "tableOffset" because it was not used anywhere * refactor: use an else clause to call "setHeaderContentRenderers" only one per draw because one additional call in case of bottom/bottomLeftCorner clones was redundant * refactor: do not return a value from table draw() because the result was not used anywhere * refactor: do not return a value from overlay.updateStateOfRendering() and overlays.prepareOverlays() because the results were not used anywhere * refactor: move "this.holderOffset" definition to master table constructor because this property is only used in the master table * refactor: reduce the number of variables used * refactor: remove table.isTableVisible because it was not used anywhere * refactor: use table.wtRootElement consistently because in many places an unnecessary DOM traversal was used * refactor: remove redundant checks for "this.clone" because it is always defined after the constructor is finished * change: move the calls of "wtOverlays" methods closer to each other because it will allow me to combine these methods. The method "resetFixedPositions", in case of a full render, is now only called if the rendering was not skipped by the "beforeDraw" hook * change: move the call of "prepareOverlays" down to make it closer to calls of other methods of "wtOverlays". This allows me to bulk these methods into one in the next commits * change: don't prepare overlays in fast draw because they are always already prepared * refactor: move "prepareOverlays", "applyToDom", "resetFixedPositions" into "refresh" because as single API entry point ("wtOverlays.refresh") it allows to better understand the context in which it is used * change: don't call "refreshSelections" in full render if rendering is skipped don't call "refreshSelections" in full render if rendering is skipped in the "beforeDraw" hook. I don't have a particular problem with it, but I am changing it because it seems redundant and I want to clean up the code from unnecessary calls. * refactor: reduce the ambiguity in overlay method names overlay.refresh -> redrawClone overlay.reset -> resetElementsSize (analogous to overlay.adjustElementsSize) overlay.resetFixedPosition -> adjustElementsPosition (analogous to overlay.adjustElementsSize) overlay.applyToDOM -> workaroundsForPositionAndSize (really, this function needs like it needs to be removed) overlay.adjustRootElementSize -> _adjustElementsSize overlays.refresh -> refreshClones overlays.refreshAll -> refreshMasterAndClones overlays.resetFixedPositions -> adjustElementsPositions overlays.adjustElementsSize -> adjustElementsSizes (plural, analogous to adjustElementsPositions) overlays.applyToDOM -> workaroundsForPositionsAndSizes (plural, analogous to workaroundsForPositionAndSize) * refactor: remove the method "syncOverlayOffset" because it wasn't clear why this particular code is not inlined inside "workaroundsForPositionAndSize" just like everything else * change: merge methods "syncScrollPositions", "syncScrollWithMaster" into one "propagateMasterScrollPositionsToClones" because these two methods were almost identical * refactor: remove the master element's properties in the overlay classes because it was very confusing that they refer to the master instance. Better to not have such properties at all, to avoid ambiguity Two were not used at all: TABLE, wtRootElement. Three were used: hider, holder, spreader * change: don't check if the master table is visible because at the point overlays.workaroundsForPositionsAndSizes is called, it should always be visible * refactor: don't use destructuring assignment on master instance in _adjustElementsSize because it is less confusing if I can see which lines of code use the master and which use the clone * change: don't call adjustElementsSize in workaroundsForPositionAndSize because there is no clear reason why this is called. All tests pass without it * refactor: use less variables because the logic was confusing * change: don't overwrite the flag "needFullRender" in "redrawClone" because it was already done in "updateStateOfRendering" and I don't understand why it is needed again. No tests fail without it. * refactor: change the way the destructuring assignment is used to prevent confusion when we are using the master and when the clone instance this commit also changes the constant names to be consistent: masterParent -> masterRootElement overlayRoot -> overlayRootElement overlayRootStyle -> overlayRootElementStyle * refactor: DRY code to set trimmingContainer because it was using unneccessary repetitions * change: remove some calls to "adjustElementsSize" and "adjustElementsSizes" because they seem redundant. It was impossible to remove the line in "table.js" without removing the lines in "top.js", "left.js" and "bottom.js", because otherwise tests failed * refactor: flatten the method "adjustElementsSize" and inline the implementation method "_adjustElementsSize" inside "adjustElementsSize" to make reasoning about the code simpler * change: cluster lines that call "adjustElementsSizes" closer together as a step in unifying as much code as possible * refactor: simplify if/else clause because the nested ifs were redundant * change: remove calls to "adjustElementsSizes" that seem redundant because no tests fail when they are skipped * refactor: rename "workaroundsForPosition(s)AndSize(s)" to "workaroundsForPosition(s)" because it no longer contains code related to sizing * change: do not run "adjustElementsPositions" in fastDraw I took a hint from "workaroundsForPositions"... if "workaroundsForPositions" are run only in fastDraw, why should it be otherwise for "adjustElementsPositions"? * change: incorporate "workaroundsForPositons" into "adjustElementsPositions" because these methods are only called together, there is a big chance they can be combined with a slightly different order of function calls * refactor: merge methods "workaroundsForPosition", "repositionOverlay" into "adjustElementsPosition" because these methods were always called together and dealt with a similar topic * refactor: remove the constant that holds the value of "getScrollbarWidth" because that constant was only used in one place. Better to call "getScrollbarWidth" there directly * refactor: use "overlayRootElementStyle" consistently since it was already defined in this method, use it in all relevant lines * change: inline "adjustElementsPositions" in "redrawClones" because I think that the key in solving the "Case 1", "Case 3" and "Case 4" problems in #6467 (comment) lies in the fact that adjustElementsPosition was done irrespective of redrewClone. the next step will be to move "adjustElementsPosition" before "redrawClone" Note that in this change, I needed to change one test (in "selectAll.spec.js"). I believe that the previous test was actually wrong and the new test fixture is the correct one, because there are less rows drawn. * change: calculate trimmingContainer once for all overlays because calculating it for each and every overlay was redundant, since it always has the same result * change: determine the master table's trimming container once per draw because calculating it for each and every overlay was redundant, since it always has the same result * refactor: remove the need to set "trimmingContainer" property on an overlay because the master table's "trimmingContainer" property can be used directly, since it is always equal * refactor: move element resizing of topLeft and bottomLeft overlays to a separate method because that is analogous to top, bottom and left overlays. This allows to update all overlays in a consistent way. * refactor: move workaround that fixes CSS created in "clone.draw" directly after "clone.draw" because the method, where this code was previously, failed if it was called before "clone.draw" * change: call `adjustElementsPosition` before `redrawClone` this allows to remove double `wt.draw();` workarounds used in `border.spec.js` and `selection.spec.js` * change: remove dead code about debug overlay Debug overlay was removed in 9e8b832. Some leftover CSS rules were cleaned up in e2ee03d. This commit removes the remaining code related to the debug overlay. * change: call "adjustElementsPosition" unconditionally (reverts d2363d2) because overlays must be repositioned also in the case of fast draw. Such use is not tested, but becomes apparent immediately in manual tests Putting this method inside "redrawClone" in more DRY * refactor: cleanup temporary comments because they were only used while work was in progress * refactor: move code related only to "master" overlay to the constructor of that overlay * change: calculate hypothetical position of the neighboring cell instead of measuring TD in another overlay because measuring in another overlay will not always is possible if the table is scrolled far from the overlay edge. The new solution calculated the hypothetical position of the neighboring cell by assuming the position and the dimensions of the last cell rendered in the current overlay. This changes the fix for the problem "the overlays overlaps custom borders". The original problem was explained in #6157 (review). The fix was introduced in d62d215 and later modified in d831e90 and 1c6fc14. * code cleanup: remove redundant comments because one of them was relevant to the behavior changed in f8c226b. The other comment was added unintentionally in that commit * fix tests in Windows verified in Firefox and Chrome. Using the scrollbar size in the equation was redundant after change f8c226b
|
Keep in mind that the problem still exists that we cannot set custom styles for area selection, until the area selection was used. It is explained in more details in #6593 (comment) (OP and the first comment by @aninde). Edit: There's a new issue for that: #6656 Edit 2: #6656 is done. |
warpech
changed the title
|
I closed the SVG borders PR #6157, becuase it got largely outdated. At some point, we will reimplement this on a fresh branch. |
|
Inform ZD |
Description
Refactor DIV based border renderer to SVG one. The main reason behind this change is the performance of Handsontable instance with multiple cell borders. As a side effect, we've fixed a few long-standing issues with custom borders.
Oh, and they are now beautiful:
This was huge and the results are amazing. Multiple tests were added. Tested on all popular web browsers.
Epic. Work was done within smaller issues.
To Do
Related issues
We should check those
Merged Cells + selection causes improper merge cell highlight. #4859 Merged Cells + selection causes improper merge cell highlight(related to mergeCells)More custom border styles #5245 More custom border styles(feature request, not included)Borders after insert row are not movedown #6063 Borders after insert row are not movedown(related to cellMeta CellMeta Refactor #6274)Checklist
updateSettings?autoColumnSizeThe text was updated successfully, but these errors were encountered: