When an image button appears after another button in a form, the wrong triggering element and #submit handlers are detected

Reproduced.

Confirmed issue is with image-button (switching to submit types does not cause the error. Having both submit and image_button on the same form causes the final image_button's value to be used)

Issue related to form.inc/_form_button_was_clicked/2070

This code is browser-dependant and not checked thoroughly enough.

Chrome sets $form_state['input'][$element['#name']] so that the first if returns TRUE. However the second element basically always returns true for image_button elements (so the last image_button is always the clicked element).

However, in Firefox the POST name=>value is *not* set, so the first if returns false for all image_button's. The issue with the second if also fails.

Fixed in Chrome + Firefox.

Both add NAME_x and NAME_y to the POST data, so testing for their existence should work. Needs testing across multiple browsers (I'm on linux so have no IE) although I don't see any problems there.

Minor points on code style:

+++ b/includes/form.incundefined
@@ -2076,15 +2076,17 @@ function _form_button_was_clicked($element, &$form_state) {
+  // their $_POST data should be ignored. To get the clicked element we test
+  // against the keys passed (eg if #name = "foo" then POST['foo_x'] and
+  // POST['foo_y']) should be set.

"e.g." should have periods here; also, should "should be set" be inside the parentheses? Edit: Or the sentence needs to be reworded somehow, anyway. It's a little confusing at present.

+++ b/includes/form.incundefined
@@ -2076,15 +2076,17 @@ function _form_button_was_clicked($element, &$form_state) {
+  else if ($element['#type'] == 'image_button' && isset($form_state['input'][$element['#name'] . '_x']) && isset($form_state[$element['#name'] . '_y'])) {

elseif (one word) is the preferred standard for Drupal. (See http://drupal.org/coding-standards.)

Also, I think we can probably use the test module from the original post to create an automated test for this (or we should try to, at least).

Thanks @richthegeek!

Tested and patched against d8, coding style fixed, comments updated to be not rubbish.

Reproduced the problem and confirmed that patch work in Chrome 17.0.963.79, Firefox 10.0.2 and Safari 5.1.

I am on Mac so no IE so someone needs to test it on IE.

Thanks @kgoel!

We should test IE6-9 since this is tagged for backport.

Also still needs an automated test. Thanks!

Writing a test for this.

My first attempt at a core patch. Could anyone review this?

Thanks

+++ b/core/modules/system/tests/modules/form_test/form_test.module
@@ -2100,6 +2108,48 @@ function form_test_clicked_button_submit($form, &$form_state) {
+ * Form submit handler for the first button of the form_test_clicked_image_button() form.
...
+ * Form submit handler for the first button of the form_test_clicked_image_button() form.

Please make these comment lines wrap at 80 chars. See http://drupal.org/coding-standards

Also, please make two patches, one with the fix and the test (which should pass the test), and one with just the test (which should fail the test).

I ran into some trouble completing the review, but this is a start.

Fixed the comment line wrapping inconsistency.

I added two patches to this comment; one should fail (without fix) and the other one should pass (includes fix).

Marking this "needs review" as the test failure was expected.

@cvangysel if you upload the patch without the fix first and then the patch with the fix, the status should be fine.

looks like tests were added, so removing needs test tag.

#14: image_button_submit-5481602-14.patch queued for re-testing.

I rerolled this patch. Hopefully, correctly. Thanks @disasm for your patience.

removing reroll tag

Reproduced the problem and confirmed that patch drupal-image_button_submit-873070-22.patch by stpaultim in #22 shows "clicked submit button 1" for the first button in Chrome, Firefox, Safari and Opera on Drupal 8.x-dev My windows computer is back at the office so can't test for IE.

Thanks @stpaultim and @andymartha!

This issue has several duplicates (in one case, the issue is older) but the patch here seems to be furthest along so I'm tentatively closing the others instead.

However, the other issues also have some good information that is worth reading (and in some cases some good code examples that could be used for tests - it sounds like the tests should be extended a bit to also assert that $form_state['triggering_element'] was set correctly):
#206955: image_button: clicked_button value is wrong
#1438614: image_button type and $form_state['clicked_button']
#1452894: Elements with #has_garbage_value (image buttons) are always set as a triggering element

From reading the above issues, my understanding is that this isn't necessarily a bug (although certainly confusing) for reasons semi-explained in the code comments visible in the patch:

-  // coordinates of the click on the button image. This means that image
-  // buttons MUST have unique $form['#name'] values, but the details of
-  // their $_POST data should be ignored.

So the way to handle this in calling code is to just set a unique #name on the image button (and don't set a #value at all), and then in theory everything should work. But, removing that restriction as the patch here does sounds like a good idea.

Finally, looking over the patch:

-  elseif (!empty($element['#has_garbage_value']) && isset($element['#value']) && $element['#value'] !== '') {
....
+  elseif ($element['#type'] == 'image_button' && isset($form_state['input'][$element['#name'] . '_x']) && isset($form_state['input'][$element['#name'] . '_y'])) {

I don't understand why the #has_garbage_value check is being removed and replaced with a hardcoded check for a particular form element type?

Actually, looking a bit more closely at #1452894: Elements with #has_garbage_value (image buttons) are always set as a triggering element, I'm not sure if the patch here is further along or that one is, so I haven't closed that issue yet.

Also, clarifying the title a bit. You don't need two image buttons on the same form to reproduce this. (The example module posted in #1438614: image_button type and $form_state['clicked_button'] is particularly useful for seeing this, and would be a great basis for expanded/improved automated tests.)

Same at the other issue:

+  $form['button1'] = array(
+    '#type' => 'image_button',
+    '#value' => t('button1'),
+    '#submit' => array('form_test_clicked_image_button_submit_one'),
+  );
+  $form['button2'] = array(
+    '#type' => 'image_button',
+    '#value' => t('button2'),
+    '#submit' => array('form_test_clicked_image_button_submit_two'),
+  );

#value is *not* an allowed property of <input type="image">.

I'm closing this one as there is no actual bug in here, but I agree this is confusing. There is a DX discussion going on in #1452894: Elements with #has_garbage_value (image buttons) are always set as a triggering element.