#292 TEdit validation can be circumvented

[Vidar Hasfjord]

When validated fields (TValidator) or required fields [feature-requests:#77] are used, OWLNext will perform coercive validation, i.e. the user is not allowed to leave a field if the input is invalid. An attempt to do so produces an error message and forces focus back to the invalid field.

The only exceptions to this rule are actions that commit or cancel the dialog (OK and Cancel).

However, the current code is not robust. It handles focus changes (see TEdit::EvKillFocus), but to allow cancel and commit, it makes exceptions for buttons with the IDOK or IDCANCEL identifier. This opens a loophole in the validation logic, i.e. the possibility of moving focus to IDOK and IDCANCEL (without pressing the buttons) with no validation of the field left behind. The code attempts to close this loophole by processing Tab events, but overlooks that there are other ways to move focus.

For example, try this in "examples/classes/validate":

1. Clear the field "Name", thus making it invalid.
2. Press-and-hold the left mouse button over the Cancel button.
3. Now cancel your action by sliding the mouse pointer off the button.
4. Release the mouse button; focus is now on the Cancel button.
5. Move focus wherever you want, without error.

The attached experimental patch replaces the complex and faulty logic in TEdit by a scheme based on delay. To allow the user to commit and cancel the dialog by clicking OK or Cancel, and in general, allow the user to click any button unhindered, the error message from an invalid field is delayed by half a second.

If the dialog is committed or cancelled in the meantime, the processing of the invalid field is delegated to CanClose (which will only validate on commit).

Note: In the special case that focus moves between two invalid fields, the patch will not attempt to reclaim focus. This exception to the rule prevents focus bouncing between invalid fields. On the downside, in this special case, the focus remains in a field unrelated to the error message. This can be mitigated by better messages that explicitly refer to the invalid field, e.g. by label.

PS. The attached patch is applicable to revision 2894 of the trunk.

[Joe Slater]

Hi Vidar,

First, the outstanding issue with TEdit validations is when focus is transferred to either the Ok/Cancel button with neither committed, no validation is performed at that time on the edit control that lost focus (which is really only a problem if the control has invalid input). I know of only two ways that can happen: the example you provided (rather unlikely in reality), or under user-written program control (outside our scope I believe).

Validation will get performed however, if/when the Ok button is committed. But this is not how the validation was designed to work.

While I have not tested this patch, these are my thoughts upon looking at the code logic…

Since OWL5 compatibility is present, change the new class declaration for EvChildInvalidate in TEdit to be:

:::C++
void TEdit::EvChildInvalidate(HWND invalidChild, HWND focusGrabber = 0);

This allows existing child classes of TEdit to continue to work that might trap WM_CHILDINVALID messages for its own purposes that still call the parent to do the work.

Change the logic in TWindow::EvChildInvalid to first check if focusGrabber is 0 (which shouldn't happen unless OWL5 compatibility is used) and set the timer, such as:

:::C++
if (focusGrabber) // Not OWL5
{
  auto w = focusGrabber;
  while (w && !GetWindowPtr(w))
    w = ::GetParent(w);
  if (!w) return; // Quit; focus lost to another application.
}

Using a set timer ID of 13487399 isn't completely safe… In theory (albeit extremely unlikely), a TWindow descendent class may already have defined and set a timer with that ID. Unfortunately, there are no Windows API provisions such as GetTimer or GetUniqueTimerID, so unless TApplication and/or TWindow wants to be enhanced to maintain a list of used timer IDs and provide these functions (I'm not suggesting that), there's little to be done about it. Probably not worth mentioning, so I did anyway. :-)

Using a set timer of ½ second, gives me concerns. A lot can happen in that time.

Consider first what happens when the timer kicks off, in TWindow::EvTimer… Can we be sure that the handle InvalidChild still belongs to the control that started this? Again, in theory that control could have been deleted, and without knowing how Windows dynamically allocates window handles, it may now be in use by another window (and application) that happens to look for messages with the same id as WM_CHILDINVALID causing possible havoc to that window. Again, unlikely, but possible, no?

Second, look at what happens in TEdit::EvChildInvalid when the control is invalid:

:::C++
    auto f = dynamic_cast<TEdit*>(GetWindowPtr(GetFocus()));
    if (!f || (f != this && f->IsValid()))
      SetFocus();

Presuming the control is still invalid, the error message would get displayed. But it will only reclaim focus if the window that now has the focus is not a TEdit control or it is valid. Consider a dialog box with three edit controls, initialized empty and declared required. Focus is in field #1. I press the TAB key twice fast (easy to do within a ½ second). Focus is in field #3. Soon thereafter I get an error message about field #1 (though as a user I don't know it's about field #1), but I am in field #3. But if you do reclaim focus with field #3 invalid, then the osciallation between the two fields would occur.

Or what if I TAB and then ALT-TAB to another OWL application that just so happens to have focus on a validating TEdit control?

I have still tried to find a good solution to this, and while I believe it is up to the control to actually determine if its contents are valid, the parent window needs to manage validating controls such that the validating controls would register themselves with the parent as such, and the parent would decide when and whether to validate the children at appropriate times.

While these don't provide solutions, they may give some insight as to how OWL should better handle it:

http://msdn.microsoft.com/en-us/library/system.windows.forms.control.causesvalidation(v=vs.110).aspx
http://stackoverflow.com/questions/1882523/how-to-skip-validating-after-clicking-on-a-forms-cancel-button
http://stackoverflow.com/questions/15920770/closing-the-c-sharp-windows-form-by-avoiding-textbox-validation/19118939#19118939

[Vidar Hasfjord]

Hi Joe,

Thanks for the feedback.

There is a lot wrong with the experimental patch I provided, but in testing it actually seems to solve the ticket-title problem, i.e. enforce that an invalid field does not go unreported while moving focus around the dialog.

However, the corner-cases, brittleness and added complexity makes me uncomfortable with this approach. At the very least, the functionality should to be moved to a (mix-in) class that is designed to be a validated control parent. The functionality does not belong in the base class for all windows.

What I think is good about the patch, is the simplifications in TEdit. So, rather than explore the original patch any further, I've created a new one that goes in the opposite direction: Eliminate the problem altogether by relaxing validation. Rather than try to enforce validation error reporting in all circumstances, allow the user to move to OK and Cancel without validation. This simplifies the needed code in TWindow immensely; now it is all contained within EvChildInvalid, with no need for state (hence not affecting the class invariant).

The articles you referenced, indicate that the convention, based on the widely used .NET framework, is to exempt validation when focus changes to selected controls (by setting the CausesValidation property on those controls to false). The attached patch provides the same functionality, except that the exemptions must be specified by overriding EvChildInvalid.

PS. The attached patch is applicable to revision 2894 of the trunk.

[Vidar Hasfjord]

Consider a dialog box [more than one invalid field].

The ticket description notes this issue and the need in this case to leave focus alone to avoid oscillation. It can be solved by reintroducing a static flag, but I consider this an disproportionate amount of complexity for a corner-case. I think improving the messages to refer to the invalid field is generally more helpful.

what if I TAB and then ALT-TAB to another OWL application that just so happens to have focus on a validating TEdit control?

Then TWindow::GetWindowPtr will return nullptr. Only objects within the application will be looked up by this function.

[Vidar Hasfjord]

I now have a version of the latest code that will let the invalid field reclaim focus, even from another invalid field. It required a private flag, but unlike the old static flag, its exposure to the rest of the code is constrained to EvKillFocus and EvChildInvalid, so it is a bit simpler.

:::C++
void TEdit::EvChildInvalid(HWND invalidChild, HWND focusGrabber)
{
  PRECONDITION(invalidChild == GetHandle() && invalidChild != focusGrabber);
  if (!IsValid(true))
  {
    if (auto f = dynamic_cast<TEdit*>(GetWindowPtr(focusGrabber)))
      f->FocusLossValidationDisabled = true; // Prevent focus change oscillation.
    SetFocus();
  }
}

PS. The attached patch is applicable to revision 2896 of the trunk.