dojo / dojox

Dojo 1 - extras library. Please submit bugs to https://bugs.dojotoolkit.org/
https://dojotoolkit.org/
Other
152 stars 229 forks source link

_FocusManager#setFocusCell scroll error #305

Open edhager opened 5 years ago

edhager commented 5 years ago

If the cells in a DojoX grid throw an error on focus, clicking on a cell can cause the browser to scroll the grid out of view.

Steps to reproduce:

  1. Create a new HTML file in your DojoX grid/tests directory and copy the "Test HTML File" content below into the new file. This is the test_edit.html file with the following modifications:
    • A tall <div> was added to the top of the body for padding.
    • The first cell in the ID column will throw an error when focus() is called.
  2. Open the new test file in Chrome.
  3. Click on the first cell in the "Id" column. It contains the number zero.

The browser will scroll up into the padding <div> and most likely scroll the grid out of view. You can click on other cells in the grid to see how it normally behaves.

When you click on a cell, _FocusManager@setFocusCell is called. That method first sets focus on a zero height/width input in a _View object. The CSS absolutely positions that input 1000 pixels above the _View domNode so focusing on it causes the browser to scroll up.

Next, setFocusCell calls _FocusManager@_focusifyCellNode(true) to focus on the cell that received the click event. _focusifyCellNode calls util.fire(n, "focus"); to set the focus on the table cell node.

    _focusifyCellNode: function(inBork){
        var n = this.cell && this.cell.getNode(this.rowIndex);
        if(n){
            html.toggleClass(n, this.focusClass, inBork);
            if(inBork){
                var sl = this.scrollIntoView();
                try{
                    if(has("webkit") || !this.grid.edit.isEditing()){
                        util.fire(n, "focus");
                        if(sl){ this.cell.view.scrollboxNode.scrollLeft = sl; }
                    }
                }catch(e){}
            }
        }
    },

Notice the call to util.fire is wrapped in a try-catch block. This suggests that errors may occur but the errors are swallowed by the do-nothing catch block. If the call to focus fails, the browser focus remains on the zero height/width input.

Two suggestions:

  1. If the call to focus fails in _focusifyCellNode, make the code attempt to focus on some other part of the grid.
  2. Add a call to console.error or console.warn in the catch block of _focusifyCellNode so developers are aware of any focus errors? There are other methods in _FocusManager that have smiliar empty catch blocks.

Test HTML File

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
<title>Test dojox.grid.DataGrid Editing</title>
<style>
    @import "../resources/Grid.css";
    body {
        font-family: Tahoma, Arial, Helvetica, sans-serif;
        font-size: 11px;
    }
    .dojoxGridRowEditing td {
        background-color: #F4FFF4;
    }
    .dojoxGrid input, .dojoxGrid select, .dojoxGrid textarea {
        margin: 0;
        padding: 0;
        border-style: none;
        width: 100%;
        font-size: 100%;
        font-family: inherit;
    }
    .dojoxGrid input {
    }
    .dojoxGrid select {
    }
    .dojoxGrid textarea {
    }
    #controls {
        padding: 6px 0;
    }
    #grid {
        width: 850px;
        height: 350px;
        border: 1px solid silver;
    }
</style>
    <script type="text/javascript" src="../../../dojo/dojo.js" data-dojo-config="isDebug:false, parseOnLoad: true"></script>
    <script type="text/javascript">
        dojo.require("dojox.grid.DataGrid");
        dojo.require("dojo.data.ItemFileWriteStore");
        dojo.require("dojo.parser");
    </script>
    <script type="text/javascript" src="support/test_data.js"></script>
    <script type="text/javascript">
    // ==========================================================================
    // Custom formatter
    // ==========================================================================
    formatMoney = function(inDatum) {
        return isNaN(inDatum) ? '...' : '$' + parseFloat(inDatum).toFixed(2);
    }
    // ==========================================================================
    // Grid structure
    // ==========================================================================
    statusCell = { field: 'col3', name: 'Status', styles: 'text-align: center;', type: dojox.grid.cells.Select, options: [ "new", "read", "replied" ] };
    gridLayout = [{
        defaultCell: { width: 8, editable: true, styles: 'text-align: right;'  },
        cells: [
            { name: 'Id', field: 'id', width: 3 },
            { name: 'Priority', field: 'col1', styles: 'text-align: center;', type: dojox.grid.cells.Select, options: ["normal", "note", "important"] },
            { name: 'Mark', width: 3, field: 'col2', styles: 'text-align: center;', type: dojox.grid.cells.Bool },
            statusCell,
            { name: 'Message', field: 'col4', styles: '', width: '100%' },
            { name: 'Amount', field: 'col5', formatter: formatMoney },
            { name: 'Amount', field: 'col6', formatter: formatMoney }
        ]
    },{
        defaultCell: { width: 4, editable: true, styles: 'text-align: right;' },
        rows: [
            { name: 'Mark', width: 3, field: 'col2', styles: 'text-align: center;', type: dojox.grid.cells.Bool}, 
            statusCell,
            { name: 'Amount', field: 'col5', formatter: formatMoney},
            { name: 'Detail', value: 'Detail'}
        ]
    }];
    // ==========================================================================
    // UI Action
    // ==========================================================================
    addRow = function(){
        test_store.newItem({
            id: grid.rowCount,
            col1: 'normal',
            col2: false,
            col3: 'new',
            col4: 'Now is the time for all good men to come to the aid of their party.',
            col5: 99.99,
            col6: 9.99,
            col7: false
        });
    }
</script>
</head>
<body>
<div style="height: 1500px">Padding.  Scroll down to see the grid.</div>
<h2>
    dojox.grid.DataGrid Basic Editing test
</h2>
<div id="controls">
    <button onclick="grid.render()">Refresh</button>&nbsp;&nbsp;&nbsp;
    <button onclick="grid.edit.focusEditor()">Focus Editor</button>
    <button onclick="grid.focus.next()">Next Focus</button>&nbsp;&nbsp;&nbsp;
    <button onclick="addRow()">Add Row</button>
    <button onclick="grid.removeSelectedRows()">Remove</button>&nbsp;&nbsp;&nbsp;
    <button onclick="grid.edit.apply()">Apply</button>
    <button onclick="grid.edit.cancel()">Cancel</button>&nbsp;&nbsp;&nbsp;
    <button onclick="grid.singleClickEdit = !grid.singleClickEdit">Toggle singleClickEdit</button>&nbsp;
</div>
<br />
<div id="grid" dojoType="dojox.grid.DataGrid" 
    data-dojo-id="grid"
    rowSelector="20px"
    store="test_store" structure="gridLayout"></div>
<br />
<div id="rowCount"></div>
<script>
    require(["dojo/ready"], function (ready) {
        ready(function () {
            var view = document.getElementById("dojox_grid__View_2");
            var cell = view.querySelectorAll(".dojoxGridCell")[0];
            cell.focus = function () {throw new Error("It failed")};
        });
    });
</script>
</body>
</html>
dylans commented 5 years ago

I'd probably suggest doing both... focus somewhere else AND add a console.warn statement when doing so.