Fixes ToggleSwitch inconsistent return of getValue() method. #196
Conversation
to call these methods when not attached and return consistent results.
|
I have not checked when attaching the toggle switch to the DOM whether the value of the underlying(hidden) is synchronized with the toggle switch value. But logically, the previous implementation is more and less "natural". And this is supposed to be what a wrapper should do (ideally a wrapper is not supposed to fix or workaround any bug from the wrapped API). Your implementation ensures the synchronization. It's good but it's not natural. The widget synchronization with the DOM value should be ensured be ensured by the Toggle Switch JavaScript API. If it's not ensured, for me, it's a bug of the JS API, not the wrapper. What you do may probably be correct. But it's more like you fix the JS API bug is our wrapper. Personally if the synchronization is not ensured by the JS API (one more time, I've not checked that yet), I think that it should be fixed by the original JS API, but not in our wrapper. |
|
I agree completely with your assessment. I have modified the code to rely completely on the value native API. It seems to be working in my tests. |
|
I also think that if we want to rely completely on the underlying native API we should remove the references to gwt widgets. It adds some unnecessary bloat and is confusing. |
|
Please take a look at the latest revisions of this PR. I really don't think the SimpleCheckBox widget has any purpose other that providing the underlying DOM element. |
|
I don't think that you know what I mean. In fact, your code will not work if the ToggleSwitch is not attached to the DOM. Because you call directly So what I mean is : the original implementation is correct (no need to change), but I'm agree with your second comment on SimpleCheckBox. That means if the synchronization is not ensured between the wrapped input checkbox and the ToggleSwitch widget, we have to create an issue on the native JS project to fix it properly. |
|
OK. I was hoping that the current implementation would work. It has been working in my tests but I am not sure all of the different tests that are necessary. I will switch back to my previous implementation. |
when widget has not been attached to the DOM.
|
I think that we are not mutually understand... :-) What I mean is : the version before any of your implementation was correct (If there is an issue of synchronization, you have to create an issue on the native JS project to get it fixed). But, I'm agree with you on using GWT Element rather than SimpleCheckBox. I hope this would be clear for you :-) |
|
OK. I understand now what you are saying ... :-) ... but I don't really agree. The current implementation of the ToggleSwitch is completely useless to me because getValue doesn't return the correct value when using the GWT Editor Framework. I completely agree that if it is the fault of the underlying library then it should be fixed there. :-) However, shouldn't we also make sure that our version of ToggleSwitch works as expected? I have verified that this PR keeps the value in the SimpleCheckBox component synchronized with the ToggleSwitch and would suggest using this implementation until we are sure its is fixed in the underlying JS library. When I have some time I will look at removing the SimpleCheckBox in favor of a GWT element. @jgodi I would be interested in your input when you have some time. |
|
I think this is the bug we are talking about. It was reported on April 27th. Just FYI. |
| switchState(getElement(), value, true); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Why do not call ONLY once switchState() ? It ensures the value synchronization between toggleSwitch and input. If they are different, you don't need to call it twice.
// This method is a workaround of the native JS issue : #...
// It should be removed when it gets fixed...
@Override
public void onAttach() {
super.onAttach();
// Ensure synchronization between DOM value and toggle switch value
switchState(getElement(), getValue(), true);
}
There was a problem hiding this comment.
Yes. Your suggestion is correct. Thanks!
|
@sjardine OK, if you need to get it work for you before they fix the JS issue, why not. I inserted my comments in your code, and for me, only two places should be modified : |
of a InputElement.
|
@crevete Thanks for your comments and proposed changes. I made some comments to your line comments. After I did I decided to remove the SimpleCheckBox completely. I think there is a disconnect between the underlying SimpleCheckBox Element and its getValue and setValue methods. Anyway, by reverting the changes to its original implementation and removing the SimpleCheckBox completely the problem is resolved. I think this updated implementation more accurately reflects the way ToggleSwitch should be implemented. I think in general it's a mistake to use GWT widgets only for their underlying element. In this case it caused a bunch of problems because our assumptions of how getValue and setValue worked within the widget was incorrect. Please review the update PR and provide your comments. Thanks! |
|
Perfect! In fact, the bug was using GWT checkbox to hold the DOM input value, right ? When wrapping native JS library on GWT, it is always better to use DOM elements rather than GWT widgets. |
Fixes ToggleSwitch inconsistent return of getValue() method.
|
Yeah...I am still a little irritated it took me so long to figure out the problem. Thanks for your help! |
The getValue() method returned inconsistent values depending on the isAttached state of the editor. Often in the editor framework, the getValue and setValue methods will be called when the editor is not attached to the DOM. The previous implementation only set the underlying checkBox's value when the editor wasn't attached to the DOM. This means the getValue will return a different value when attached vs not attached.
This implementation uses the checkBox as the underlying authority for the value of the editor and synchronizes its value with the ToggleSwitch when it is attached to the DOM. If the use toggles the value the underlying checkBox's value is updated.
I also agree with @crevete's comment in #158 that we should rely on the native API when possible but it seems that updating the toggle switch when its not attached to the DOM is problematic.