Changing the behavior of CellBrowser so that the first child node of the next level in the tree is not immediately selected in the SelectionModel when its parent is selected. In general, we no longer select a value in a CellList or CellTable until the user interacts with the widget. Also, we do not deselect a selected value until a new value is available for selection. This is important for asynchronous lists, which clear the current data then load the new data asynchronously. We do not want to trigger a SelectionEvent between these states where we have nothing selected.
Review at http://gwt-code-reviews.appspot.com/1198801
Review by: sbrubaker@google.com
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@9403 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/ShowMorePagerPanel.java b/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/ShowMorePagerPanel.java
index 6d97341..dedeed3 100644
--- a/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/ShowMorePagerPanel.java
+++ b/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/ShowMorePagerPanel.java
@@ -54,6 +54,9 @@
public ShowMorePagerPanel() {
initWidget(scrollable);
+ // Do not let the scrollable take tab focus.
+ scrollable.getElement().setTabIndex(-1);
+
// Handle scroll events.
scrollable.addScrollHandler(new ScrollHandler() {
public void onScroll(ScrollEvent event) {
diff --git a/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java b/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
index 7a79fcd..1ecbfba 100644
--- a/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
+++ b/user/src/com/google/gwt/user/cellview/client/HasDataPresenter.java
@@ -181,6 +181,8 @@
boolean rowCountIsExact = false;
final List<T> rowData = new ArrayList<T>();
final Set<Integer> selectedRows = new HashSet<Integer>();
+ T selectedValue = null;
+ boolean viewTouched;
public DefaultState(int pageSize) {
this.pageSize = pageSize;
@@ -218,6 +220,10 @@
return Collections.unmodifiableList(rowData);
}
+ public T getSelectedValue() {
+ return selectedValue;
+ }
+
public boolean isRowCountExact() {
return rowCountIsExact;
}
@@ -233,6 +239,10 @@
public boolean isRowSelected(int index) {
return selectedRows.contains(index);
}
+
+ public boolean isViewTouched() {
+ return viewTouched;
+ }
}
/**
@@ -271,6 +281,8 @@
this.pageStart = state.getPageStart();
this.rowCount = state.getRowCount();
this.rowCountIsExact = state.isRowCountExact();
+ this.selectedValue = state.getSelectedValue();
+ this.viewTouched = state.isViewTouched();
// Copy the row data.
int rowDataSize = state.getRowDataSize();
@@ -343,6 +355,11 @@
List<T> getRowDataValues();
/**
+ * Get the value that is selected in the {@link SelectionModel}.
+ */
+ T getSelectedValue();
+
+ /**
* Get a boolean indicating whether the row count is exact or an estimate.
*/
boolean isRowCountExact();
@@ -354,6 +371,13 @@
* @return true if selected, false if not
*/
boolean isRowSelected(int index);
+
+ /**
+ * Check if the user interacted with the view at some point. Selection is
+ * not bound to the keyboard selected row until the view is touched. Once
+ * touched, selection is bound from then on.
+ */
+ boolean isViewTouched();
}
/**
@@ -728,9 +752,12 @@
return;
}
+ // The user touched the view.
+ ensurePendingState().viewTouched = true;
+
/*
* Early exit if the keyboard selected row has not changed and the keyboard
- * selected value is already set.
+ * selected value is already set.
*/
if (!forceUpdate && getKeyboardSelectedRow() == index
&& getKeyboardSelectedRowValue() != null) {
@@ -1162,24 +1189,38 @@
/*
* Update the SelectionModel based on the keyboard selected value. This must
- * happen before we read the selection state.
+ * happen before we read the selection state. We only bind to selection
+ * after the user has interacted with the widget at least once. This
+ * prevents values from being selected by default.
*/
if (KeyboardSelectionPolicy.BOUND_TO_SELECTION == keyboardSelectionPolicy
- && selectionModel != null) {
- T oldValue = oldState.getRowDataSize() > 0
- ? oldState.getRowDataValue(oldState.getKeyboardSelectedRow()) : null;
+ && selectionModel != null && pending.viewTouched) {
+ T oldValue = oldState.getSelectedValue();
Object oldKey = getRowValueKey(oldValue);
T newValue = rowDataCount > 0
? pending.getRowDataValue(pending.getKeyboardSelectedRow()) : null;
Object newKey = getRowValueKey(newValue);
- if ((oldKey == null) ? newKey != null : !oldKey.equals(newKey)) {
+ /*
+ * Do not deselect the old value unless we have a new value to select, or
+ * we will have a null selection event while we wait for asynchronous data
+ * to load.
+ */
+ if (newKey != null && !newKey.equals(oldKey)) {
+ // Check both values for selection before setting selection, or the
+ // selection model may resolve state early.
+ boolean oldValueWasSelected = (oldValue == null) ? false
+ : selectionModel.isSelected(oldValue);
+ boolean newValueWasSelected = (newValue == null) ? false
+ : selectionModel.isSelected(newValue);
+
// Deselect the old value.
- if (oldValue != null && selectionModel.isSelected(oldValue)) {
+ if (oldValueWasSelected) {
selectionModel.setSelected(oldValue, false);
}
// Select the new value.
- if (newValue != null && !selectionModel.isSelected(newValue)) {
+ pending.selectedValue = newValue;
+ if (newValue != null && !newValueWasSelected) {
selectionModel.setSelected(newValue, true);
}
}
diff --git a/user/test/com/google/gwt/user/cellview/client/AbstractCellTreeTestBase.java b/user/test/com/google/gwt/user/cellview/client/AbstractCellTreeTestBase.java
index 574c596..e988b99 100644
--- a/user/test/com/google/gwt/user/cellview/client/AbstractCellTreeTestBase.java
+++ b/user/test/com/google/gwt/user/cellview/client/AbstractCellTreeTestBase.java
@@ -27,6 +27,7 @@
import com.google.gwt.safehtml.shared.SafeHtmlBuilder;
import com.google.gwt.user.client.ui.RootPanel;
import com.google.gwt.view.client.ListDataProvider;
+import com.google.gwt.view.client.MultiSelectionModel;
import com.google.gwt.view.client.ProvidesKey;
import com.google.gwt.view.client.TreeViewModel;
@@ -49,6 +50,8 @@
*/
protected class MockTreeViewModel implements TreeViewModel {
+ private static final int MAX_DEPTH = 4;
+
/**
* The cell used to render all nodes in the tree.
*/
@@ -59,16 +62,30 @@
*/
private final ListDataProvider<String> rootDataProvider = createDataProvider("");
+ /**
+ * The selection models at each level of the tree.
+ */
+ private final List<MultiSelectionModel<String>> selectionModels = new ArrayList<MultiSelectionModel<String>>();
+
+ public MockTreeViewModel() {
+ for (int i = 0; i < MAX_DEPTH; i++) {
+ selectionModels.add(new MultiSelectionModel<String>());
+ }
+ }
+
public <T> NodeInfo<?> getNodeInfo(T value) {
if (value == ROOT_VALUE) {
- return new DefaultNodeInfo<String>(rootDataProvider, cell);
+ return new DefaultNodeInfo<String>(rootDataProvider, cell,
+ selectionModels.get(0), null);
} else if (value instanceof String) {
String prefix = (String) value;
- if (prefix.length() > 3) {
- throw new IllegalStateException(
- "Prefix should never exceed four characters.");
+ int depth = prefix.length();
+ if (depth >= MAX_DEPTH) {
+ throw new IllegalStateException("Prefix should never exceed "
+ + MAX_DEPTH + " characters.");
}
- return new DefaultNodeInfo<String>(createDataProvider(prefix), cell);
+ return new DefaultNodeInfo<String>(createDataProvider(prefix), cell,
+ selectionModels.get(depth), null);
}
throw new IllegalArgumentException("Unrecognized value type");
}
@@ -77,12 +94,12 @@
if (value == ROOT_VALUE) {
return false;
} else if (value instanceof String) {
- String s = (String) value;
- if (s.length() > 4) {
+ int depth = ((String) value).length();
+ if (depth > MAX_DEPTH) {
throw new IllegalStateException(
"value should never exceed five characters.");
}
- return ((String) value).length() == 4;
+ return depth == MAX_DEPTH;
}
throw new IllegalArgumentException("Unrecognized value type");
}
@@ -92,6 +109,16 @@
}
/**
+ * Get the {@link MultiSelectionModel} for the nodes at the specified depth.
+ *
+ * @param depth the depth of the node
+ * @return the {@link MultiSelectionModel} at that depth
+ */
+ public MultiSelectionModel<String> getSelectionModel(int depth) {
+ return selectionModels.get(depth);
+ }
+
+ /**
* Create a data provider that extends the prefix by one letter.
*
* @param prefix the prefix string
diff --git a/user/test/com/google/gwt/user/cellview/client/CellBrowserTest.java b/user/test/com/google/gwt/user/cellview/client/CellBrowserTest.java
index 11364e3..273f9d1 100644
--- a/user/test/com/google/gwt/user/cellview/client/CellBrowserTest.java
+++ b/user/test/com/google/gwt/user/cellview/client/CellBrowserTest.java
@@ -118,6 +118,37 @@
assertTrue(browser.treeNodes.get(0).isFocusedOpen());
}
+ /**
+ * Test that even when keyboard selection is bound to the selection model, we
+ * do not automatically select items in child lists until the child list is
+ * actually touched.
+ */
+ public void testSetKeyboardSelectionPolicyBound() {
+ CellBrowser browser = (CellBrowser) tree;
+
+ // Bind keyboard selection to the selection model.
+ browser.setKeyboardSelectionPolicy(KeyboardSelectionPolicy.BOUND_TO_SELECTION);
+ assertEquals(KeyboardSelectionPolicy.BOUND_TO_SELECTION,
+ browser.getKeyboardSelectionPolicy());
+
+ // Select an item at depth 0. Nothing should be selected at depth 1.
+ BrowserCellList<?> list0 = browser.treeNodes.get(0).getDisplay();
+ list0.getPresenter().setKeyboardSelectedRow(1, false, false);
+ list0.getPresenter().flush();
+ browser.treeNodes.get(1).getDisplay().getPresenter().flush();
+ assertEquals(1, model.getSelectionModel(0).getSelectedSet().size());
+ assertEquals(0, model.getSelectionModel(1).getSelectedSet().size());
+
+ // Select an item at depth 1. Nothing should be selected at depth 2.
+ BrowserCellList<?> list1 = browser.treeNodes.get(1).getDisplay();
+ list1.getPresenter().setKeyboardSelectedRow(2, false, false);
+ list1.getPresenter().flush();
+ browser.treeNodes.get(2).getDisplay().getPresenter().flush();
+ assertEquals(1, model.getSelectionModel(0).getSelectedSet().size());
+ assertEquals(1, model.getSelectionModel(1).getSelectedSet().size());
+ assertEquals(0, model.getSelectionModel(2).getSelectedSet().size());
+ }
+
public void testSetKeyboardSelectionPolicyDisabled() {
CellBrowser browser = (CellBrowser) tree;
diff --git a/user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java b/user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java
index 6796d2b..e387698 100644
--- a/user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java
+++ b/user/test/com/google/gwt/user/cellview/client/HasDataPresenterTest.java
@@ -31,6 +31,7 @@
import com.google.gwt.view.client.MockHasData.MockRangeChangeHandler;
import com.google.gwt.view.client.MockHasData.MockRowCountChangeHandler;
import com.google.gwt.view.client.MockSelectionModel;
+import com.google.gwt.view.client.ProvidesKey;
import com.google.gwt.view.client.Range;
import com.google.gwt.view.client.RangeChangeEvent;
import com.google.gwt.view.client.SelectionChangeEvent;
@@ -47,6 +48,49 @@
public class HasDataPresenterTest extends GWTTestCase {
/**
+ * A mock {@link SelectionChangeEvent.Handler} used for testing.
+ */
+ private static class MockSelectionChangeHandler implements
+ SelectionChangeEvent.Handler {
+
+ private boolean eventFired;
+
+ /**
+ * Assert that a {@link SelectionChangeEvent} was fired and clear the
+ * boolean.
+ *
+ * @param expected the expected value
+ */
+ public void assertEventFired(boolean expected) {
+ assertEquals(expected, eventFired);
+ eventFired = false;
+ }
+
+ public void onSelectionChange(SelectionChangeEvent event) {
+ assertFalse(eventFired);
+ eventFired = true;
+ }
+ }
+
+ /**
+ * A mock {@link SelectionModel} used for testing without used any GWT client
+ * code.
+ *
+ * @param <T> the selection type
+ */
+ private class MockSingleSelectionModel<T> extends SingleSelectionModel<T> {
+
+ public MockSingleSelectionModel(ProvidesKey<T> keyProvider) {
+ super(keyProvider);
+ }
+
+ @Override
+ public void fireSelectionChangeEvent() {
+ super.fireSelectionChangeEvent();
+ }
+ }
+
+ /**
* A mock view used for testing.
*
* @param <T> the data type
@@ -794,6 +838,16 @@
presenter.flush();
assertEquals(0, model.getSelectedSet().size());
+ // Clear the data and push new data. This should not select an item because
+ // there has not been any user interaction.
+ presenter.setRowCount(0, true);
+ presenter.flush();
+ presenter.setRowCount(100, false);
+ presenter.flush();
+ populatePresenter(presenter);
+ presenter.flush();
+ assertEquals(0, model.getSelectedSet().size());
+
// Select an element.
presenter.setKeyboardSelectedRow(5, false, false);
presenter.flush();
@@ -809,8 +863,9 @@
// Select an element on another page.
presenter.setKeyboardSelectedRow(11, false, false);
presenter.flush();
- // Nothing is selected yet because we don't have data.
- assertEquals(0, model.getSelectedSet().size());
+ // The previous value is still selected because we don't have new data.
+ assertEquals(1, model.getSelectedSet().size());
+ assertTrue(model.isSelected("test 9"));
populatePresenter(presenter);
presenter.flush();
// Once data is pushed, the selection model should be populated.
@@ -818,6 +873,62 @@
assertTrue(model.isSelected("test 10"));
}
+ /**
+ * Test that we only get one selection event when keyboard selection changes.
+ */
+ public void testSetKeyboardSelectedRowFiresOneSelectionEvent() {
+ HasData<String> listView = new MockHasData<String>();
+ MockView<String> view = new MockView<String>();
+ HasDataPresenter<String> presenter = new HasDataPresenter<String>(listView,
+ view, 10, null);
+ presenter.setVisibleRange(new Range(0, 10));
+ populatePresenter(presenter);
+ presenter.flush();
+
+ // Bind keyboard selection to the selection model.
+ presenter.setKeyboardSelectionPolicy(KeyboardSelectionPolicy.BOUND_TO_SELECTION);
+ presenter.setKeyboardPagingPolicy(KeyboardPagingPolicy.CHANGE_PAGE);
+
+ // Add a selection model.
+ MockSingleSelectionModel<String> model = new MockSingleSelectionModel<String>(
+ null);
+ presenter.setSelectionModel(model);
+ presenter.flush();
+ assertNull(model.getSelectedObject());
+
+ // Add an selection event handler.
+ MockSelectionChangeHandler handler = new MockSelectionChangeHandler();
+ model.addSelectionChangeHandler(handler);
+
+ // Select an element.
+ presenter.setKeyboardSelectedRow(5, false, false);
+ presenter.flush();
+ model.fireSelectionChangeEvent();
+ handler.assertEventFired(true);
+ assertEquals("test 5", model.getSelectedObject());
+
+ // Select another element.
+ presenter.setKeyboardSelectedRow(9, false, false);
+ presenter.flush();
+ model.fireSelectionChangeEvent();
+ handler.assertEventFired(true);
+ assertEquals("test 9", model.getSelectedObject());
+
+ // Select an element on another page.
+ presenter.setKeyboardSelectedRow(11, false, false);
+ presenter.flush();
+ model.fireSelectionChangeEvent();
+ // The previous value is still selected because we don't have new data.
+ handler.assertEventFired(false);
+ assertEquals("test 9", model.getSelectedObject());
+ populatePresenter(presenter);
+ presenter.flush();
+ model.fireSelectionChangeEvent();
+ // Once data is pushed, the selection model should be populated.
+ assertEquals("test 10", model.getSelectedObject());
+ handler.assertEventFired(true);
+ }
+
public void testSetKeyboardSelectedRowChangePage() {
HasData<String> listView = new MockHasData<String>();
MockView<String> view = new MockView<String>();
