Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass correct arguments to onFinished in Modal #224

Closed
wants to merge 2 commits into from

Conversation

@aviraldg
Copy link
Collaborator

@aviraldg aviraldg commented Mar 16, 2016

Fixes vector-im/element-web#1169

Works, and I couldn't find a case where this breaks existing behaviour, but since there are no tests to verify I can't be sure.

@@ -41,7 +41,7 @@ module.exports = {
var closeDialog = function() {
ReactDOM.unmountComponentAtNode(self.getOrCreateContainer());

if (props && props.onFinished) props.onFinished.apply(null, arguments);
if (props && props.onFinished) props.onFinished.call(null, false);

This comment has been minimized.

@ara4n

ara4n Mar 16, 2016
Member

This doesn't look right - i assume someone passed in closeDialog's arguments to the onFinished prop for a reason... What is the rationale for switching from apply() to call()?

This comment has been minimized.

@aviraldg

aviraldg Mar 16, 2016
Author Collaborator

Exactly, I did some searching but couldn't find a case where that was actually used (the event that caused the dialog to be closed)

This comment has been minimized.

@aviraldg

aviraldg Mar 16, 2016
Author Collaborator

UserSettings.onEmailDialogFinished is a similar onFinished callback and expects its first argument to be whether the dialog was accepted or dismissed. It should probably have the same bug (as leave room)

This comment has been minimized.

@aviraldg

aviraldg Mar 16, 2016
Author Collaborator

Looks like that code was added in d938ba7, which also carries this bug (and does not use arguments in any way)

This comment has been minimized.

@dbkr

dbkr Mar 17, 2016
Member

Looks like this has been the case every since Modal.js was first written in vector-im/element-web@1b43586

Either way, your fix always passes false to onFinished which means the user won't leave then room even if they click OK. The correct fix here is to explicitly call closeDialog(false) in the background div onClick handler rather than using it as the onClick handler directly (since the event handler will be passed an event as an argument which, when it propagates down to the onFinished handler, will evaluate to true.

This comment has been minimized.

@aviraldg

aviraldg Mar 17, 2016
Author Collaborator

Right, @dbkr, just did not notice that (or test it.) Is there any plan to add e2e tests for situations like this?

This comment has been minimized.

@dbkr

dbkr Mar 18, 2016
Member

Yeah, we need tests in general in vector.

@dbkr dbkr removed their assignment Mar 17, 2016
@aviraldg aviraldg force-pushed the aviraldg:fix-1169 branch from 0236900 to f20d02e Mar 17, 2016
@ara4n
Copy link
Member

@ara4n ara4n commented Mar 18, 2016

I tried to dig into this last night as part of vector-im/element-web#1121 and got completely stuck - the Modal.js API seems entirely confused here in terms of how it handles onFinished(). @aviraldg: looks like you picked one of the most tangled bits of the codebase for a quick fix - sorry! am working through it IRL with @dbkr now.

@dbkr dbkr assigned ara4n and unassigned dbkr Mar 18, 2016
@ara4n
Copy link
Member

@ara4n ara4n commented Mar 18, 2016

Okay - so there were two confusions here:

  1. mx_Dialog_background's onClick needs to pass false through to onFinished() rather than an object. The right place to do this is at the onClick - see deaa5c3#diff-3ddc036af206541151e86f03cfc0c0eeR54. This is because closeDialog() can be called from multiple places (the .close() method on the returned dialog object; the onFinished() of the wrapped Element) and still needs to pass through whatever arguments it takes to onFinished() - we can't just have it always pass through false. This is correctly using apply(null, arguments) in case there are more than one arguments to be passed.
  2. Modal.createDialogWithElement()'s onFinished semantics were completely broken, and has been remove entirely.

I've fixed both in #232.

Huge thanks for doing the initial digging on this, and sorry that Modal.js is such a mess. Totally agreed that we urgently need UI-layer tests on vector, via karma or selenium or whatever. Patches welcome...

@ara4n ara4n closed this Mar 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.