Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Introduce the $.mobile.formwidget class to handle the case of form reset in a unified manner for form widgets #5207

Closed
wants to merge 4 commits into from

3 participants

@gabrielschulhof
Collaborator

No description provided.

@johnbender

A couple questions:

  1. We're using the "widget inheritance" model when this, at least by my estimation, appears to be behavioral. That is, can we just extend the prototypes of our form widgets with a "mixin" object to capture this? Inheritance is generally extremely brittle.
  2. Have we looked into the setTimeout? Is that something we could solve with an event (seems unreasonable but worth asking)? I'm just curious why it requires the stack to unwind (I'm guessing something to do with the order in which we report the value on selection).
  3. Is the goal to move over to the _reset because it's the UI standard? If it is I'm for just moving the functionality in our widgets into a _reset and then calling that from refresh. This saves on confusion with calls to refresh because the call site is in the widget definition, without committing to yet another class in the widget hierarchy.
@gabrielschulhof
Collaborator

A couple questions:

  1. We're using the "widget inheritance" model when this, at least by my estimation, appears to be behavioral. That is, can we just extend the prototypes of our form widgets with a "mixin" object to capture this? Inheritance is generally extremely brittle.

I'm not sure what the effect of that would be on the widget prototype. UI has done a good job implementing inheritance properly, and we should be using it to override the _reset() method introduced by the form superclass. If we go the mixin way, we may end up having to reproduce some of UI's work.

  1. Have we looked into the setTimeout? Is that something we could solve with an event (seems unreasonable but worth asking)? I'm just curious why it requires the stack to unwind (I'm guessing something to do with the order in which we report the value on selection).

I couldn't figure it out either why the stack has to unwind. The fact is that, if we don't unwind, only the input fields are reset, and that's probably because in their case we do not hide the original native widget.

  1. Is the goal to move over to the _reset because it's the UI standard? If it is I'm for just moving the functionality in our widgets into a _reset and then calling that from refresh. This saves on confusion with calls to refresh because the call site is in the widget definition, without committing to yet another class in the widget hierarchy.

UI isn't really concerned with form inputs. The only widget they have that's tied to a form input is the button, which has reset logic - the same as ours, i.e., call refresh on form reset. So, I'm not modelling this after UI.

The best reason I see to avoid another class is that we should avoid having too many abstract classes. I'm not sure if we could call this class abstract, but the fact is it is not itself a widget, nor is it useful by itself. So, I suppose we ought to organize this such that every form widget makes a call to a common $.mobile._setMeUpForFormReset( callbackToRunInsteadOfTheDefaultRefresh ) which binds to "reset", does the stack unwind, and calls _reset() if no callback is given and if the widget has a refresh method. In fact, it's probably a bad idea to pass a callback at all, because, if anyone ever decides to subclass the widget whose _create() passes a callback, that widget won't have the option of overriding the passed-in function.

If we go this route, we should establish

  1. The namespace for the function - is it OK as a private function under $.mobile?
  2. The name of the function - setMeUpForFormReset sounds hokey ...
  3. Where we put this function - in which source file shall it reside?
@gabrielschulhof
Collaborator

Hmmm ... while implementing this other route, I ran into @johnbender's 3. from above: We cannot simply move the body of refresh() into _reset() and then call _reset() from refresh() because, for example, the slider's refresh() method takes parameters, whereas the _reset() method does not. Thus, in the current implementation, the slider overrides the form widget's implementation of _reset(), calling refresh() with some parameters.

This would not be possible if the body of refresh() would be executed without parameters.

@gabrielschulhof
Collaborator

Check out #5214 - it accomplishes the same thing without introducing a new base class.

@jaspermdegroot
Collaborator

Regarding the setTimeout. I don't think we can avoid this. When the reset event fires the values of the form elements aren't actually reset yet so we grab the old value when we call refresh(). Unfortunately the change event doesn't fire after the values has been updated, otherwise this wouldn't have been an issue at all.

https://forum.jquery.com/topic/trapping-the-form-reset-event#14737000000771218

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
1  js/index.php
@@ -42,6 +42,7 @@
'widgets/listview.filter.js',
'widgets/listview.autodividers.js',
'jquery.mobile.nojs.js',
+ 'widgets/forms/formwidget.js',
'widgets/forms/checkboxradio.js',
'widgets/forms/button.js',
'widgets/forms/slider.js',
View
4 js/widgets/forms/checkboxradio.js
@@ -9,11 +9,11 @@
//>>css.structure: ../css/structure/jquery.mobile.forms.checkboxradio.css
//>>css.theme: ../css/themes/default/jquery.mobile.theme.css
-define( [ "jquery", "../../jquery.mobile.core", "../../jquery.mobile.widget", "../../jquery.mobile.buttonMarkup" ], function( $ ) {
+define( [ "jquery", "../../jquery.mobile.core", "./formwidget", "../../jquery.mobile.buttonMarkup" ], function( $ ) {
//>>excludeEnd("jqmBuildExclude");
(function( $, undefined ) {
-$.widget( "mobile.checkboxradio", $.mobile.widget, {
+$.widget( "mobile.checkboxradio", $.mobile.formwidget, {
options: {
theme: null,
initSelector: "input[type='checkbox'],input[type='radio']"
View
39 js/widgets/forms/formwidget.js
@@ -0,0 +1,39 @@
+//>>excludeStart("jqmBuildExclude", pragmas.jqmBuildExclude);
+//>>description: Base class for form widgets.
+//>>label: Baseclass
+//>>group: Forms
+
+define( [ "jquery", "../../jquery.mobile.core", "../../jquery.mobile.widget" ], function( $ ) {
+//>>excludeEnd("jqmBuildExclude");
+(function( $, undefined ) {
+
+$.widget( "mobile.formwidget", $.mobile.widget, {
+ _init: function() {
+ this._on( this.element.closest( "form" ), {
+ "reset": "_resetTimeout"
+ });
+ this._super( '_init' );
+ },
+
+ _resetTimeout: function() {
+ // For some reason, we need to take this extra step of calling _reset via
+ // setTimeout because, at least in the case of the select menu, if we call
+ // _reset directly, the widget receives the old input value rather than the
+ // reset input value when it attempts to reset its appearance to reflect
+ // the original value. However, this only happens when we hit reset the
+ // first time.
+ setTimeout( $.proxy( this, "_reset" ) );
+ },
+
+ _reset: function() {
+ // We call refresh() by default if that method is available
+ if ( this.refresh ) {
+ this.refresh.apply( this, arguments );
+ }
+ this._super( '_reset' );
+ }
+});
+})( jQuery );
+//>>excludeStart("jqmBuildExclude", pragmas.jqmBuildExclude);
+});
+//>>excludeEnd("jqmBuildExclude");
View
4 js/widgets/forms/select.js
@@ -5,11 +5,11 @@
//>>css.structure: ../css/structure/jquery.mobile.forms.select.css
//>>css.theme: ../css/themes/default/jquery.mobile.theme.css
-define( [ "jquery", "../../jquery.mobile.core", "../../jquery.mobile.widget", "../../jquery.mobile.buttonMarkup", "../../jquery.mobile.zoom" ], function( $ ) {
+define( [ "jquery", "../../jquery.mobile.core", "./formwidget", "../../jquery.mobile.buttonMarkup", "../../jquery.mobile.zoom" ], function( $ ) {
//>>excludeEnd("jqmBuildExclude");
(function( $, undefined ) {
-$.widget( "mobile.selectmenu", $.mobile.widget, {
+$.widget( "mobile.selectmenu", $.mobile.formwidget, {
options: {
theme: null,
disabled: false,
View
4 js/widgets/forms/slider.js
@@ -5,11 +5,11 @@
//>>css.structure: ../css/structure/jquery.mobile.forms.slider.css
//>>css.theme: ../css/themes/default/jquery.mobile.theme.css
-define( [ "jquery", "../../jquery.mobile.core", "../../jquery.mobile.widget", "./textinput", "../../jquery.mobile.buttonMarkup" ], function( $ ) {
+define( [ "jquery", "../../jquery.mobile.core", "./formwidget", "./textinput", "../../jquery.mobile.buttonMarkup" ], function( $ ) {
//>>excludeEnd("jqmBuildExclude");
(function( $, undefined ) {
-$.widget( "mobile.slider", $.mobile.widget, {
+$.widget( "mobile.slider", $.mobile.formwidget, {
widgetEventPrefix: "slide",
options: {
Something went wrong with that request. Please try again.