Permalink
Browse files

Enh(doco): create design folder, put some thoughts in it

  • Loading branch information...
1 parent 9be0549 commit c7a6acb1ef4324e48aa746826552134914c40bd8 @PaulL1 PaulL1 committed May 17, 2015
@@ -0,0 +1,159 @@
+# Edit and CellNav, Focus in the Grid
+
+Both edit and cellNav provide ways to focus particular cells in the grid. This, and the associated
+`editOnFocus` option have been a source of pain, but they are also (unfortunately) a very desirable
+feature that we'd like to have working smoothly.
+
+The current implementation is inspired by the google sheets implementation. A key feature is that
+the focus is held by the viewport, not by the individual cells, and focus is "faked" through use of
+css classes and directing keypresses to particular elements.
+
+<< @swalters: is this only true for cellNav, or is it also true for edit? It seems that edit must
+be receiving focus or else some of the custom editors just wouldn't work >>
+
+We continue to have difficulties with the interaction of these two modules, and with the behaviour
+upon events such as scrolling, scrolling that is triggered by typing (for a part-visible cell),
+`editOnFocus` and whether it works reliably.
+
+This overview aims to document what we think our business features are, and then describe a framework
+that we could move to that would make the logic cleaner and therefore easier to fix defects in.
+
+## CellNav
+
+The intention of cellNav is that a user can select a cell, and then use the keyboard to move around
+the grid. When the focus is given to a particular cell the grid should attempt to scroll to make that
+cell fully visible (if it's not already fully visible). If the cell is too tall or too wide to be
+fully visible, then the grid should show as much of the cell as possible whilst keeping the top left
+corner fully visible.
+
+Keyboard actions that should move the focused cell are:
+ - 4 arrow keys
+ - pg up, pg down (move a page at a time)
+ - enter (moves down)
+ - shift+enter (moves up)
+ - tab (moves right)
+ - shift+tab (moves left)
+
+Where a navigation hits the edge of the grid, the navigation should "wrap around". If we move off
+a row to the right, we should come back on 1 row down, in the leftmost focusable cell. If we move
+off a row to the left, we should come back 1 row up, in the rightmost focusable cell. Similarly for
+moving up and down - if we hit enter till we get to the bottom of the grid, we should re-enter at the
+top.
+
+There are two exceptions to this behaviour. When we exit the top left focusable cell, we should shift focus
+off the grid entirely, to the previous widget on the page. When we exit the bottom right focusable cell, we should
+shift focus to the next widget on the page.
+
+When we tab into the grid from another widget on the page, we should select the top left or bottom right
+focusable cell.
+
+When we scroll we should remember which cell had focus. If that cell remains visible then we should highlight
+it as having the focus. If that cell scrolls off the page then we should remember it, and highlight it again
+when it comes visible.
+
+A source of contention is that when we give a cell focus, we attempt to make it visible. When a user manually
+scrolls we reset the focus to the correct cell, but we don't want to automatically scroll to make it visible - this
+will give jankiness to our scrolling as the focused cell moves off the page.
+
+Another contention is that as we tab across a row we sometimes get scroll up and down for some reason to do with
+native browser behaviour.
+
+
+## Edit
+
+Edit works by noticing that a cell has entered edit mode, and replacing the cell contents with the specified editor.
+The editor will raise events for start_edit, end_edit and cancel_edit. Upon end or cancel the edit feature will
+remove the editor and replace it with the normal cell template.
+
+`editOnFocus` is a feature that works like a spreadsheet - when you enter a cell the editor is displayed and the
+cell is immediately editable.
+
+Upon a cell becoming editable we should attempt to scroll it so that it is fully visible, and as with cellNav if we
+cannot make it fully visible we should do as good a job as we can whilst keeping the top left corner visible.
+
+Editors often have default navigation actions. For example, in an input box if you use the arrow keys to go to the
+edge of the field and then off, it will shift focus to the next field. This is effectively a cellNav.
+
+Our different editors have slightly different desired default behaviours.
+
+### Input
+
+Upon an input field receiving focus all the text in that field should be automatically selected, the cell should
+be scrolled so far as possible to make the field fully visible. The arrow keys when pressed should remove the highlight
+of all text, and move around within the field. They should not exit the field when they reach the end.
+
+Enter, shift+enter, tab, shift+tab should end editing, and should trigger a cellNav. Perhaps these should still be
+trapped by cellNav itself, and not by edit? Is this possible with standard html5 widgets?
+
+## Date
+
+Ideally this would one day be a proper date picker. Anyway, up and down arrow keys should adjust the current
+date element we're on (the year, month or day), left and right arrow keys should select the next date part in that
+direction. They should not take us off the field to the left or right.
+
+Enter, shift+enter, tab, shift+tab should end editing, and should trigger a cellNav. Perhaps these should still be
+trapped by cellNav itself, and not by edit? Is this possible with standard html5 widgets?
+
+## Dropdown
+
+Starting edit should drop down the dropdown. Up and down arrows should move the selection. Left and right
+arrows should move the selection the same as up and down.
+
+Enter, shift+enter, tab, shift+tab should end editing, and should trigger a cellNav. Perhaps these should still be
+trapped by cellNav itself, and not by edit? Is this possible with standard html5 widgets?
+
+## Custom editors
+
+In general custom editors should follow the pattern of only triggering a cellNav on Enter, shift+enter, tab, shift+tab,
+this also means that custom editors should not use these keys for anything else.
+
+## Deep edit
+
+I can't remember why this is a good idea. It seems that the semantics of the input field work fine without a deep edit
+concept. I think it's more that when an editable field gets focus but hasn't yet entered edit mode, we still let the
+left/right work with cellNav. So maybe deep edit is sort of a halfway house of `editOnFocus` that we don't really need?
+
+## Edit On Focus
+
+Edit on focus should work that whenever a cell gets focus we should immediately enter edit mode. Otherwise the
+behaviour should remain the same.
+
+## Problems
+
+Edit has a number of points of contention with cellNav, and the interoperation of the two is problematic (but also
+essential). Areas of conflict are:
+
+- when you're typing in a cell you can trigger a scroll if the cell isn't fully visible. The scroll may change
+ which DOM elements are what, and therefore which DOM element it is that we're editing in. The upshot is that
+ we tend to lose focus on the editor, and then gain focus again. When we gain focus again we're sometimes not
+ where we used to be - for example on the input field we may select all the text rather than returning our cursor
+ to where it was. These are usability issues for a user who is typing in the field
+
+- some editors propagate events to the parent object, for example the standard input seems to propagate a right
+ arrow key at the end of the field to the parent, which in turn causes a cellNav. This isn't our desired
+ behaviour
+
+- if the user scrolls whilst a field is in edit mode we sometimes get unusual behaviour. Really we want it to work
+ similarly to if a scroll happened whilst editing a field - we want our position in the editor at the end of the scroll
+ to be the same as at the start, unless our field has gone off page, in which case we want to end edit mode
+
+- edit on focus is hard because when a cell gets focus we start edit. Starting edit then attempts to select all the
+ text, and scroll so we're visible, which makes a mess
+
+## Design thoughts
+
+Can we agree that tab, shift+tab, enter, shift+enter are always trapped by cellNav, and that all editors must stay
+away from them? Can we agree that other than those 4 keys, no editor will ever propagate navigation (and work out
+how to make that true, as well as document how to do it for custom editors as well)?
+
+Can we define some sort of state callback for each editor (that each editor must create and respond to)? So
+when a scroll happens we'd call getState on the editor, then when the scroll completes we'd find the new instance
+of that editor and call setState? Then each editor is responsible for maintaining it's own state, or choosing not
+to maintain it. For some editors there may be no state at all (for a dropdown, the selected item is the state), for
+others it's pretty basic (for input it's the cursor location and the selected text start and end).
+
+Can we draw a distinction between getting focus through a user action (keyboard action or mouse click or touch), and
+re-getting focus after a scroll or other grid rendering change? Then we could choose not to select all the text
+and scroll to make a cell visible if we're re-getting focus as opposed to initially getting focus?
+
+
@@ -1,7 +1,4 @@
-@ngdoc overview
-@name Rendering Cycle
-@module
-@description
+# Grid Rendering Cycle
The core grid rendering cycle is executed whenever the grid needs re-rendering. There are a number of core methods,
with some of those methods able to be called individually.
@@ -0,0 +1,133 @@
+# Scrolling and Virtualisation Design
+
+This document is a first cut at documenting how scrolling and virtualisation works, and
+providing a path to refactoring or otherwise tidying up the scrolling logic.
+
+## Overview
+
+We have various scrollable renderContainers: left, body and right. The body
+render container is always present, the left and right render containers depend
+on RTL settings and pinning settings. In a left to right (default) setup with
+grouping or selection enabled you'll have a left render container that holds the
+row header (the select buttons or the grouping expand buttons).
+
+Within each render container we have a header, a viewport and a footer.
+
+The viewport within the render containers scrolls vertically, with the scroll bar displayed
+in the rightmost render container. Irrespective of which render container receives the
+scroll event we scroll all containers that are present vertically (scroll synchronisation).
+
+The body render container scrolls horizontally, with the header, viewport and footer all scrolling.
+At the current time the scrollbar is shown at the bottom of the body viewport, consideration should
+be given as to whether it should instead be shown at the bottom of the footer render container. Irrespective
+of whether the header, viewport or footer receive the scroll event, all three are synchronised (again,
+scroll synchronisation).
+
+Virtualisation is implemented to reduce the number of DOM elements rendered. If a grid has 10,000 visible
+rows we provide the illusion that we have rendered all of them, but we actually only create DOM elements
+for those rows that are currently visible, and a reasonable buffer either side. Similarly for horizontal
+scrolling we only render the columns that are actually visible plus a buffer either side. We then apply
+padding to the viewports to provide the illusion that there are more DOM elements rendered than there are.
+
+Virtualisation applies to the body renderContainer (header, viewport, footer) for horizontal scrolling, and
+to the left, right and body viewports for vertical scrolling.
+
+
+## Design Concept
+
+### Separation
+
+We separate scrolling from virtualisation.
+- Scrolling is the process of moving the visible area of a canvas
+- Virtualisation is the process of deciding whether to change the DOM elements that are rendered, and
+ associated padding
+
+
+### Scrolling
+
+Scrolling should try to behave like native browser scrolling as far as possible - when a user triggers a
+scroll using a scrollbar, trackpad or touch event the only additional processing we should add is to
+synchronise the relevant containers, based on the scroll directions.
+
+Scrolling should provide events to virtualisation so that we know that scrolling occurred, but the act of
+scrolling should not be blocked by virtualisation - this should reduce any janking or speed issues. Ideally
+scrolling on it's own does not trigger a digest cycle - digests should only be triggered by virtualisation, if
+at all.
+
+### Virtualisation
+
+Virtualisation is the act of deciding which DOM elements should be currently rendered, and applying padding
+to make the scrollbars look as if there's the right amount of scroll distance. We render the visible rows
+plus `excessRows` additional rows to the top and bottom. We render the visible columns plus `excessColumns`
+additional columns to the left and right. When virtualisation is notified of a scroll event we take a look
+to see whether we have at least `minExcessRows` of buffer in the scroll direction, and at least `minExcessCols`
+of buffer in the scroll direction.
+
+If either minExcess is breached, then we adjust the DOM such that we have `excessRows` and `excessColumns` again,
+and adjust the scroll padding appropriately.
+
+In order to avoid browser blocking and general thrashing, we implement a throttle. Once one scroll event finishes
+we always wait at least `minScrollWait` ms before we respond to another scroll event. At the point we do respond,
+we respond based on the current scroll position, not based on any intermediate positions.
+
+
+### Variable Row and Column Size
+
+In implementing this design concept we'll also explicitly allow for variable height rows and columns. We'll build
+logic that iterates the visible column array and the visible row array, and adds to each row and column a `scrollToHere`
+value that tells us the pixel count to the leftmost edge of the column, or the topmost edge of the row.
+
+The height/width attributed to a row/column will be based on a height/width that is stored directly on the row/column,
+if that is not present then it will base off the rowHeight set in the gridOptions.
+
+The `scrollToHere` value will need to be recalculated based on triggers. The notifyDataChange architecture
+will be used to manage these triggers. Triggers will be:
+ - column resize
+ - row height change
+ - rerun of rowsProcessors or columnsProcessors (i.e. potentially change in ordering, visible rows/cols etc)
+ - change of data (perhaps picked up in the above)
+ - change of columnDefs (perhaps picked up in the above)
+
+
+### Watchers
+
+We have watchers on the row and column / on the cell that look for a change in the row/col that this particular
+DOM element is attached to.
+
+Column changes are automatically handled through use of `track by uid`, which means that any time the uid of a
+column changes new DOM elements will be rendered. Having said this, it isn't currently clear that this means
+we can avoid the watchers, but essentially this should mean that we never reuse DOM elements for columns - any
+new uid will result in new DOM elements. For rows, the trigger for reuse is virtualisation. This means that
+we can replace the current watchers with a notification architecture instead, and we can specifically notify
+when we change the row/DOM alignment. (If it turns out we also want watchers for columns, we could similarly
+notify of changes to the col).
+
+We would do this using the notifyDataChange architecture. This would remove a number of watchers.
+
+
+### Events
+
+What events do we have, and what are they used for? Need to ask @swalters or read the code.
+
+
+### Benefits
+
+The result should be that slow scrolling is very smooth - most scroll ticks will result in the canvas
+moving but no DOM re-rendering and no digest cycle. When a user scrolls faster we may result in every scroll tick
+meeting the criteria for re-rendering the DOM, but we'll only do the actual re-render at a maximum rate per second,
+so we're not driving into digest hell and thrashing our browser (particularly on lower powered devices such as
+mobile devices).
+
+
+### Things to Watch Out For
+
+The virtualisation / scrolling cycle currently calls one of the variants of grid refresh (perhaps refreshCanvas,
+can't recall off the top of my head). It's also likely that one of the grid refresh variants will need to recalculate
+virtualisation, as this would be the trigger for some things like data having changed. We need to take care that we
+don't create an endless loop.
+
+Scrolling is a bit untidy in the movement between grid triggered and native browser scrolls. I seem to recall that
+we get native scroll events triggering when we internally scroll, we need to be careful to not end up in a loop,
+and we can perhaps manage CPU use by not processing both. Perhaps we can have a way to detect that they originate
+with the same user event?
+

0 comments on commit c7a6acb

Please sign in to comment.