Bug 118486 - [DataBinding] If validation fails, focus should be kept in field
Summary: [DataBinding] If validation fails, focus should be kept in field
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Platform UI Triaged CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-11-29 14:25 EST by Dave Orme CLA
Modified: 2019-09-06 15:30 EDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Orme CLA 2005-11-29 14:25:59 EST
Given:

import java.beans.PropertyChangeListener;
import java.beans.PropertyChangeSupport;
import java.util.ArrayList;
import java.util.List;

import org.eclipse.jface.databinding.BindSpec;
import org.eclipse.jface.databinding.ChangeEvent;
import org.eclipse.jface.databinding.DataBinding;
import org.eclipse.jface.databinding.IChangeListener;
import org.eclipse.jface.databinding.IDataBindingContext;
import org.eclipse.jface.databinding.IUpdatableValue;
import org.eclipse.jface.databinding.Property;
import org.eclipse.jface.databinding.validator.RegularExpressionStringValidator;
import org.eclipse.jface.databinding.viewers.TableViewerDescription;
import org.eclipse.jface.databinding.viewers.ViewersProperties;
import org.eclipse.jface.viewers.TableViewer;
import org.eclipse.swt.SWT;
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Group;
import org.eclipse.swt.widgets.Label;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.swt.widgets.Table;
import org.eclipse.swt.widgets.TableColumn;
import org.eclipse.swt.widgets.Text;

public class TestTable {
   
   public static class Show {
      private String showName;
      
      public Show(String string) {
         this.showName = string;
      }

      public String getShowName() {
         return showName;
      }
      
      public void setShowName(String showName) {
         this.showName = showName;
      }

      private List people = new ArrayList();
      
      public List getPeople() {
         return people;
      }
      
      public void addPerson(Person person) {
         people.add(person);
      }
   }
   
   public static class Person {
      private String firstName;
      private String lastName;
      
      public Person(String firstName, String lastName) {
         this.firstName = firstName;
         this.lastName = lastName;
      }
      
      private PropertyChangeSupport pcs = new PropertyChangeSupport(this);
      
      public void addPropertyChangeListener(PropertyChangeListener l) {
         pcs.addPropertyChangeListener(l);
      }
      
      public void removePropertyChangeListener(PropertyChangeListener l) {
         pcs.removePropertyChangeListener(l);
      }
      
      public String getFirstName() {
         return firstName;
      }
      
      public void setFirstName(String firstName) {
         String oldValue = this.firstName;
         this.firstName = firstName;
         pcs.firePropertyChange("firstName", oldValue, firstName);
      }
      
      public String getLastName() {
         return lastName;
      }
      public void setLastName(String lastName) {
         this.lastName = lastName;
      }
   }
   
   private Shell sShell = null;  //  @jve:decl-index=0:visual-constraint="10,10"
   private Table peopleTable = null;
   private Group group = null;
   private Label label = null;
   private Label label1 = null;
   private Text firstName = null;
   private Text lastName = null;
   private Table showsTable = null;
   
   private List shows = new ArrayList();
   
   public TestTable() {
      Show show = new Show("Sally's party");
      show.addPerson(new Person("John", "Doe"));
      show.addPerson(new Person("George", "Gershwin"));
      show.addPerson(new Person("Bill", "Gates"));
      shows.add(show);
      show = new Show("Jane's party");
      show.addPerson(new Person("Jane", "Doe"));
      shows.add(show);
   }


   public List getShows() {
      return shows;
   }


   /**
    * This method initializes table	
    *
    */
   private void createTable() {
      GridData gridData = new org.eclipse.swt.layout.GridData();
      gridData.horizontalAlignment = org.eclipse.swt.layout.GridData.FILL;
      gridData.grabExcessHorizontalSpace = true;
      gridData.grabExcessVerticalSpace = true;
      gridData.verticalAlignment = org.eclipse.swt.layout.GridData.FILL;
      peopleTable = new Table(sShell, SWT.FULL_SELECTION);
      peopleTable.setHeaderVisible(true);
      peopleTable.setLayoutData(gridData);
      peopleTable.setLinesVisible(true);
      TableColumn tableColumn = new TableColumn(peopleTable, SWT.NONE);
      tableColumn.setWidth(150);
      tableColumn.setText("FirstName");
      TableColumn tableColumn1 = new TableColumn(peopleTable, SWT.NONE);
      tableColumn1.setWidth(150);
      tableColumn1.setText("LastName");
   }

   /**
    * This method initializes group	
    *
    */
   private void createGroup() {
      GridData gridData3 = new org.eclipse.swt.layout.GridData();
      gridData3.grabExcessHorizontalSpace = true;
      gridData3.verticalAlignment = org.eclipse.swt.layout.GridData.CENTER;
      gridData3.horizontalAlignment = org.eclipse.swt.layout.GridData.FILL;
      GridData gridData2 = new org.eclipse.swt.layout.GridData();
      gridData2.grabExcessHorizontalSpace = true;
      gridData2.verticalAlignment = org.eclipse.swt.layout.GridData.CENTER;
      gridData2.horizontalAlignment = org.eclipse.swt.layout.GridData.FILL;
      GridLayout gridLayout = new GridLayout();
      gridLayout.numColumns = 2;
      GridData gridData1 = new org.eclipse.swt.layout.GridData();
      gridData1.horizontalAlignment = org.eclipse.swt.layout.GridData.FILL;
      gridData1.grabExcessHorizontalSpace = true;
      gridData1.grabExcessVerticalSpace = false;
      gridData1.verticalAlignment = org.eclipse.swt.layout.GridData.CENTER;
      group = new Group(sShell, SWT.NONE);
      group.setText("Current person");
      group.setLayout(gridLayout);
      group.setLayoutData(gridData1);
      label = new Label(group, SWT.NONE);
      label.setText("First Name:");
      firstName = new Text(group, SWT.BORDER);
      firstName.setLayoutData(gridData2);
      label1 = new Label(group, SWT.NONE);
      label1.setText("Last Name:");
      lastName = new Text(group, SWT.BORDER);
      lastName.setLayoutData(gridData3);
   }

   /**
    * This method initializes table	
    *
    */
   private void createTable2() {
      GridData gridData4 = new org.eclipse.swt.layout.GridData();
      gridData4.horizontalAlignment = org.eclipse.swt.layout.GridData.FILL;
      gridData4.grabExcessHorizontalSpace = true;
      gridData4.grabExcessVerticalSpace = true;
      gridData4.verticalAlignment = org.eclipse.swt.layout.GridData.FILL;
      showsTable = new Table(sShell, SWT.NONE);
      showsTable.setHeaderVisible(true);
      showsTable.setLayoutData(gridData4);
      showsTable.setLinesVisible(true);
      TableColumn tableColumn2 = new TableColumn(showsTable, SWT.NONE);
      tableColumn2.setWidth(200);
      tableColumn2.setText("Show Name");
   }

   /**
    * @param args
    */
   public static void main(String[] args) {
      Display display = Display.getDefault();
      TestTable thisClass = new TestTable();
      thisClass.createSShell();
      thisClass.bindControls();
      thisClass.sShell.open();
      while (!thisClass.sShell.isDisposed()) {
         if (!display.readAndDispatch())
            display.sleep();
      }
      display.dispose();
   }

   private void bindControls() {
      // Initialize data binding
      IDataBindingContext dbc = DataBinding.createContext(sShell);
      
      TableViewer showTableViewer = new TableViewer(showsTable);
      TableViewerDescription columns = new TableViewerDescription(showTableViewer);
      columns.addColumn("showName");
      
      dbc.bind(columns, new Property(this, "shows", Show.class, true), null);
      
      IUpdatableValue selectedShow = (IUpdatableValue) dbc
         .createUpdatable(new Property(showTableViewer, ViewersProperties.SINGLE_SELECTION));
      
      final TableViewer peopleTableViewer = new TableViewer(peopleTable);
      columns = new TableViewerDescription(peopleTableViewer);
      columns.addColumn("firstName");
      columns.addColumn("lastName");
      
      selectedShow.addChangeListener(new IChangeListener() {
         public void handleChange(ChangeEvent changeEvent) {
            if (((Show)changeEvent.getNewValue()).getPeople().size() > 0) {
               Display.getCurrent().asyncExec(new Runnable() {
                  public void run() {
                     peopleTableViewer.getTable().select(0);
                     peopleTableViewer.getTable().notifyListeners(SWT.Selection, null);
                  }
               });
            }
         }
      });
      
      dbc.bind(columns, new Property(selectedShow, "people", Person.class, true), null);

      IUpdatableValue selectedPerson = (IUpdatableValue) dbc
         .createUpdatable(new Property(peopleTableViewer, ViewersProperties.SINGLE_SELECTION));

      dbc.bind(firstName, new Property(selectedPerson, "firstName", String.class, false), 
            new BindSpec(null, new RegularExpressionStringValidator("^.*$", "^.+$", "Please enter at least one character")));
      dbc.bind(lastName, new Property(selectedPerson, "lastName", String.class, false), null);
   }

   /**
    * This method initializes sShell
    */
   private void createSShell() {
      sShell = new Shell();
      sShell.setText("Test");
      sShell.setLayout(new GridLayout());
      createTable2();
      createTable();
      createGroup();
      sShell.setSize(new org.eclipse.swt.graphics.Point(379,315));
   }

}

Select the first show and the first person, then move to the First Name field in the Text widget at the bottom of the window.  Backspace/delete all characters in the field.  This will make the field invalid (per the RegularExpressionValidator in the code above).  Press tab to leave the field.  The field value will correctly not be saved, however, the focus will still move to the last name field.

Suggest we include code something like the following in the focusLost method for the SWT Text updatable:

if (validator.validate(getValue()) != null) {
   Display.getCurrent().asyncExec(new Runnable()) {
      public void run() {
          textField.setFocus();
      }
   }
}
Comment 1 Boris Bokowski CLA 2005-11-29 15:27:24 EST
I object, at least to doing this by default. There are lots of existing dialogs or wizard pages where you can enter invalid information, but still move around to other text fields. The only thing that happens is that the "OK"/"Finish" button is disabled.
Comment 2 Dave Orme CLA 2005-11-29 15:37:47 EST
(In reply to comment #1)
> I object, at least to doing this by default. There are lots of existing dialogs
> or wizard pages where you can enter invalid information, but still move around
> to other text fields. The only thing that happens is that the "OK"/"Finish"
> button is disabled.

Okay; I agree that there needs to be a mode where the current behavior is allowed.  However, I think we might want to consider locking the user into the field as the default behavior for the following reasons:

1) In RCP applications, dialogs are the 20% case.  Most data will be edited in views/editors where there are no OK/Cancel buttons.

2) This is the way other client-server applications based on Microsoft Access or Visual Basic work, so this behavior is consistent with the principle of least suprise.

3) The current behavior presents a usability problem that can be observed if you run the code I pasted in above.  In this code, the firstName property is bound twice: once in the table and a second time to a Text control.  If you change value to an invalid value in one place, and tab away, the other control is *not* updated, leaving the user interface in an inconsistent state.

Comment 3 Joe Winchester CLA 2005-11-30 05:48:19 EST
I agree with Boris - we should never lock people into fields.  Forcing focus back can create deadlocks with situations where the user can't exit, for example with cross validation where fixing an error might involve visiting another field.  It's bad UI to force focus and we'll get burned for it.
The example you gave is where a text field is bound twice, and has two validators.  One of which allows it and the other rejects it - I think this is a rare case we should make the exception and not the norm. 
One thing I think we could do is thing of having a way of making fields show their contents are in error.  This could be to make the background red (we'd do it via a policy so people could customize the behavior) or else we should make it very very easy for people to drop a Label or One row high read only list that contained all of the errors on the GUI.  That way the case of the dialog with no buttons would still give the user feedback of any errors but without being in their face with forced focus events
Comment 4 Dave Orme CLA 2005-11-30 10:03:09 EST
(In reply to comment #3)
> I agree with Boris - we should never lock people into fields.  Forcing focus
> back can create deadlocks with situations where the user can't exit, for
> example with cross validation where fixing an error might involve visiting
> another field. 

There are two cases here, and I think it's helpful to separate them.

1) Syntactic validation, or making sure the string in the text field is in the correct format to be stored in the first place.  This can mean making sure that a date is in the correct format that the converter can successfully parse it back into a Date object or it can mean making sure that a telephone number is in the correct format.

2) Semantic validation.  Cross-field checks like you mentioned are one example of this.

To be clear, I'm talking about case #1, which is what you most often wind up implementing using our IValidators.

Consider a telephone number field.  There is absolutely no reason to let someone leave a telephone number field and come back when it is in an invalid state.  Similarly, if someone has entered a date in an invalid format and it simply can't be parsed by the date converter, there is no reason to let the user leave the date field.

> It's bad UI to force focus and we'll get burned for it.

On the contrary, it's bad UI to allow incorrect data into an application in the first place if you can help it.  If you allow syntactically invalid data on an input form, then you have to keep a list of things that are invalid.  If there is a tabbed notebook, the invalid data might be invisible, forcing the user to hunt for the invalid value and fix it.

On my current project, we had initially implemented exactly what you are proposing.  And after a week of testing it, we rejected it because the result was simply too hard for the users.

The solution we arrived at is to lock users into syntactically invalid fields, but to keep an error list for semantic errors.

> The example you gave is where a text field is bound twice, and has two
> validators.  One of which allows it and the other rejects it

Okay, in the code, that is true.  But to see the UI inconsistency, you could have identical validators on both fields and you would see the same behavior.

Leaving an invalid field leaves two fields bound to the same object, but with two different values.  This is inconsistent and a good UI will not allow that to happen.

> - I think this is
> a rare case we should make the exception and not the norm. 

On the contrary, people edit dates, social security numbers, product codes, and so on with syntactical validation constraints all the time.  This is the 80% case for validation.

> One thing I think we could do is thing of having a way of making fields show
> their contents are in error.  This could be to make the background red (we'd do
> it via a policy so people could customize the behavior) or else we should make
> it very very easy for people to drop a Label or One row high read only list
> that contained all of the errors on the GUI.  That way the case of the dialog
> with no buttons would still give the user feedback of any errors but without
> being in their face with forced focus events

This makes sense for semantic errors.  But not for syntactic errors.

We just went through this at my current client.  The users hated it.
Comment 5 Peter Centgraf CLA 2006-06-02 13:31:38 EDT
The appropriate behavior appears to depend on the details of UI design for a specific project.  The standard SWT behavior causes the least programmer pain as a default policy.  However, focus-locking is a generally useful feature.  Perhaps it can be made available via a policy mechanism, similar to eager vs. late validation.
Comment 6 Matthew Hall CLA 2009-01-06 13:26:00 EST
(In reply to comment #2)
> 3) The current behavior presents a usability problem that can be observed if
> you run the code I pasted in above.  In this code, the firstName property is
> bound twice: once in the table and a second time to a Text control.  If you
> change value to an invalid value in one place, and tab away, the other control
> is *not* updated, leaving the user interface in an inconsistent state.

This problem can be avoided by binding both controls to an intermediate observable such as WritableValue, then binding the intermediate observable to the model, using validators only in the latter binding.  In this way the related UI controls remains consistent to eachother and validation is preserved.

I would not be opposed to adding some sort of helper class to make this simple:

SWTObservables.forceFocusWhileInvalid(text, dbc.bindValue(
  SWTObservables.observeText(text, SWT.Modify),
  BeansObservables.observeValue(bean, "something),
  null,
  null) );

This would be syntactic sugar for a simple listener the focus observable which calls focusObservable.setValue(Boolean.TRUE) whenever the focus changes to Boolean.FALSE and the validation status observable has an invalid status.

We might also consider a generic approach which would be portable to other toolkits like GWT:

Observables.coerceValueOnInvalidStatus(
  // the observable to coerce
  SWTObservables.observeFocus(text),

  // the value to force on the observable
  Boolean.TRUE,

  // the validation status provider used to determine if the
  // observable needs to be coerced.
  dbc.bindValue(
    SWTObservables.observeText(text, SWT.Modify),
    BeansObservables.observeValue(bean, "something),
    null,
    null) );
Comment 7 Matthew Hall CLA 2009-01-21 15:55:22 EST
Will investigate for M6
Comment 8 Matthew Hall CLA 2009-03-03 11:11:09 EST
This won't make it into 3.5 unless there's some outside interest in pushing this through
Comment 9 Eclipse Webmaster CLA 2019-09-06 15:30:32 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.