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.

Sign up for GitHub

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

Jump to bottom

_FocusManager#setFocusCell scroll error #305

Open
edhager opened this issue Jan 26, 2019 · 1 comment
Open

_FocusManager#setFocusCell scroll error #305

edhager opened this issue Jan 26, 2019 · 1 comment
Labels
bug

Comments

@edhager
Copy link

edhager commented Jan 26, 2019

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>
@edhager edhager added the bug label Jan 26, 2019
@dylans
Copy link
Member

dylans commented Jan 26, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dylans @edhager