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

[JENKINS-21613] f:combobox may break form.onsubmit #1111

Conversation

@ikedam
Copy link
Member

commented Feb 2, 2014

f:combobox prevents form submission when the dropdown is open.
This is done by interrupting form.onsubmit.
It handles form.onsubmit in the following way:

  1. ComboBox saves the original onsubmit handler in its initialization.
  2. When the combobox is selected, it shows the dropdown and overwrites the onsubmit handler. The ovirridden onsubmit prevents form submission of the dropdown is open.
  3. When the dropdown is closed, recovers the saved form.onsubmit.

If the form.onsubmit was overridden between step 1 and 2, it would be overridden in step 3 and lost.

This change saves the original onsubmit handler just before overriding it.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Feb 2, 2014

core » jenkins-core #116 SUCCESS
This pull request looks good

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Apr 13, 2014

This JENKINS-21613 becomes annoying in the case of using "Apply" hotkey.
Any "autosave" plugin (if such feature exist somewhere) may suffer from the issue.

+1

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jun 10, 2016

retriggering the PR.
BTW I feel my comments have not been addressed

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2016

BTW I feel my comments have not been addressed

I might misunderstood your comment.
Do you mean this request cause a problem for the case of using "Apply" hotkey ?
I don't get how the problem occurs.

Anyway, new plugins should use Behavior mechanisms for onsubmit events and they shouldn't be affected by JENKINS-21613, and this request is completely deprecated.
I'm thinking of closing this request and JENKINS-21613 with "Won't fix".

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Jul 20, 2016

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 20, 2016

@ikedam @daniel-beck
Went through the code again. I'm actually fine with the approach. I'm not sure what my comment from Jun 11 meant

@oleg-nenashev
Copy link
Member

left a comment

I see no reason why we keep it unmerged. :shipit:

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

Retriggering CI

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

@ikedam Could you please re-merge with the master to retrigger CI? https://ci.jenkins.io/job/Core/job/jenkins/job/PR-1111/9/console

IIUC one cannot just retrigger it after the recent changes in PR handling by @rtyler and @daniel-beck

@oleg-nenashev oleg-nenashev requested a review from daniel-beck Mar 4, 2018

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

I don't understand the problem well enough to have an opinion on this.

@daniel-beck daniel-beck removed their request for review Mar 4, 2018

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

On hold till it's remerged with master

@ikedam ikedam force-pushed the ikedam:feature/JENKINS-21613_onsubmit_for_combobox branch from 9f45ffd to d0ff413 Mar 25, 2018

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2018

Rebased to master (no conflicts) to re-trigger the CI.

combobox.js is really old-fashioned, and it might be better to reform it in the modern way (e.g. using event listeners).
Anyway, I believe this change makes combobox.js a little safer and is good to merge.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 25, 2018

@ikedam Could you provide an example how the previous implementation could lead to problems? It's unclear to me how this works.

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2018

@daniel-beck This issue can cause a problem when a component (component X) tries to hook onsubmit event by decorating form.onsubmit in this way:

  1. Initialization of combobox.js. It saves form.onsubmit.
  2. Initialization of the component X. It decorates form.onsubmit to hook onsubmit event.
  3. Onfocus of combobox.js fires. It decorates form.onsubmit to hook onsubmit event.
  4. Onblur of combobox.js fires. It restores form.onsubmit to the state of 1. This results the decoration of 2 loses.

This can be fixed in any of following ways:

A. Uses addEventListener to hook events. (This wasn’t an option when I created this request as IE8- had problems in event handlings)
B. Initialize the component X before combobox.js. This can be done by specifying the priority order in Behavior.register.
C. (This request) Save form.onsubmit not when component initialization but when just before decorating it.

I found this issue when I developed the first version of extensible-choice-parameter-plugin.
It developed targetting Jenkins 1.466, and found it get broken with Jenkins1.480+.
That plugin uses a customized combobox and hit this issue.
ikedam/extensible-choice-parameter#3
jenkinsci/extensible-choice-parameter-plugin@66a0103#diff-3f9234fad1a0c518288418c0f36e1ba6

I fixed that problem in the way same to this request and wanted to apply the same change to core as it seemed a fundamentally unsafe behavior even though there’re workarounds as described above.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

@ikedam Thanks for the explanation.

@oleg-nenashev
Copy link
Member

left a comment

Fine with me, reconfirm my feedback

@oleg-nenashev oleg-nenashev removed the on-hold label Mar 31, 2018

@oleg-nenashev oleg-nenashev merged commit 3f8a843 into jenkinsci:master Mar 31, 2018

1 of 2 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.