com_redirect modal close button check in fix #19116

Ruud68 · 2017-12-21T09:43:49Z

Pull Request for Issue # .

Summary of Changes

Changed the close button (footer) to execute the plugin.cancel task

Testing Instructions

[tab 1]open com_redirect
[tab 2] open second tab with com_plugins and filter on 'redirect' plugin
in [tab 2] enable the redirect plugin and disable the collect urls > save the plugin
in [tab 1] in com_redirect click the 'Redirect System Plugin' link in the notice
clicking the [Close] button will close the modal

Expected result

The plg_system_redirect should be checked in

Actual result

The plg_system_redirect is NOT checked in.
in [tab 2] press ctrl-f5 and you can see that although the modal in tab 1 is closed, the plugin is not checked in correct.

Documentation Changes Required

none.

Note

this was previously fixed here: #18292 but unfortunately reintroduced with normalize patch: #18913

Quy · 2017-12-21T18:07:24Z

administrator/components/com_redirect/views/links/tmpl/default.php

 					'height'      => '400px',
 					'width'       => '800px',
 					'bodyHeight'  => '70',
 					'modalWidth'  => '80',
 					'closeButton' => false,
 					'backdrop'    => 'static',
 					'keyboard'    => false,
-					'footer'     => '<button class="btn" data-dismiss="modal" aria-hidden="true">'
-						. JText::_("JLIB_HTML_BEHAVIOR_CLOSE") . '</button>'
+					'footer'      => '<button type="button" class="btn" data-dismiss="modal" aria-hidden="true"'


The other buttons don't have type="button". Can it be omitted?

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button
http://w3c.github.io/html-reference/button.button.html

Omitting the type defaults to type = submit.
Following the info provided by @brianteeman all three buttons should be of type = button because:

"The button has no default behavior. It can have client-side scripts associated with the element's events, which are triggered when the events occur."

The assumption I make here is that both the save and save & close buttons do not submit the form (then they should be of type=submit), but trigger jQuery script that does the checking and saving.

If the above is the way to go (all buttons type = button), then there are probably other areas where we can improve.

Can somebody confirm?

Quy · 2017-12-23T18:09:21Z

In the edit article modal, <a> is used instead of <button>. Maybe use <a> to be consistent.

Ruud68 · 2017-12-23T18:48:47Z

What I learned is that using the < a> in a form is wrong. Although it works, it behaves differently in e.g. accessibility use cases (for a Screen-Reader a link behaves different to a button (tab index, space bar / enter behavior)). https://css-tricks.com/use-button-element/ Op 23 dec. 2017 19:11, om 19:11, Quy <notifications@github.com> schreef:

…

In the edit article modal, `<a>` is used instead of `<button>`. Maybe use `<a>` to be consistent. ![article-edit-modal](https://user-images.githubusercontent.com/368084/34321642-068edda2-e7c9-11e7-8dad-4661edbe8e87.gif) -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #19116 (comment)

Quy · 2017-12-24T15:48:35Z

I have tested this item ✅ successfully on 71bb5d9

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19116.}

sandewt · 2017-12-25T10:41:25Z

I have tested this item ✅ successfully on 71bb5d9

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19116.}
Tested in: Firefox 57.0.3

dgrammatiko · 2017-12-25T11:08:48Z

What I learned is that using the < a> in a form is wrong

The reason for A tag instead of BUTTON, which is the correct one, has to do with the stupid bootstrap modal script! To verify this go to the page with this modal and press enter (it doesn't matter which element has focus), the form will be submitted although it shouldn't. Now change the Button to A tags and retest, form will not submit!

This is a hack that is needed atm as we cannot change the Bootstrap 2.3.2 javascript!

ghost · 2017-12-25T11:27:38Z

Ready to Commit after two successful tests.

dgrammatiko · 2017-12-25T11:39:54Z

@franz-wohlkoenig NO, the tags need to be converted to A for the reason I stated above. Please remove the RTC label

Ruud68 · 2017-12-29T08:33:47Z

Hello @dgt41, just tried what you described: open the modal, press enter key.
The form stays open and doesn't close...
I think this is because on the form there is now NO button of type submit (all buttons are of type = 'button').
So I tested this successfully

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19116.}

dgrammatiko · 2017-12-29T08:47:47Z

You need to test this with Firefox

Ruud68 · 2017-12-29T09:16:54Z

I am testing this with Firefox (Quantum): 57.0.3 (64-bit)
cannot reproduce the case you are describing: sorry :(

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19116.}

sandewt · 2017-12-29T19:19:20Z

I have tested this item 🔴 unsuccessfully on 71bb5d9

This time a tested in Google Chrome 63.0.3239.108

Because here is it not a real issue, because I can not reproduce it !!!

See earlier test too.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19116.}

I can not reproduce the case of @dgt41 in Firefox and Google Chrome.

sandewt · 2017-12-29T19:28:57Z

@Ruud68 have you tested in Google Chrome (without the patch tester)? Result?
So not, can you test?

Ruud68 · 2017-12-30T09:26:41Z

Hi @sandewt I have just installed Google Chrome (Version 63.0.3239.108 (Official Build) (64-bit))
my patch works, and also the issue reported by @dgt41 regarding the closing of the modal when pressing the [enter] key: pressing the [enter] key does NOT close the modal (tested now on both Firefox and Chrome)

You tested unsuccessful, what is it that you tested? I am confused :)

sandewt · 2017-12-30T16:27:48Z

Hi @Ruud68,

There is a misunderstanding.

I mean with unsuccessfull in Google Chrome that the plugin is (remains) checked in without and with the patch tester. So you don't need the patch tester!

I can't reproduce the case of @dgt41 in Firefox and Google Chrome.

Gr.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19116.}
Note: Tested in Firefox 57.0.3 successfull.

Ruud68 · 2017-12-31T10:02:15Z

Okay, retested everything both with FF and Chrome.

without patch:
Firefox has the issue that the plugin is not checked in when closing the modal window via the [close] button
Chrome does NOT have this issue: the modal is checked in correct (as found by @sandewt )
The issue reported by @dgt41 that pressing the enter key in the modal window closes the modal is NOT reproducible on both FF and Chrome.

with the patch applied:
Firefox checks in the modal CORRECT when closing the modal with the [close] button
Chrome checks in the modal CORRECT when closing the modal with the [close] button
Again: The issue reported by @dgt41 that pressing the enter key in the modal window closes the modal is NOT reproducible on both FF and Chrome.

So the issue raised by @dgt41 that blocks this patch from becoming RTC CANNOT be reproduced (at least by me and @sandewt ). I have tried to reproduce on both a clean install as on (multiple) production sites).

How to proceed?

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19116.}

dgrammatiko · 2017-12-31T10:12:08Z

@Ruud68 I raised the issue because I had to patch this couple of times in the past, if this is not an issue anymore then this is RTC!
Just to be clear here I was talking about #14483
@franz-wohlkoenig you can move this to RTC again

ghost · 2017-12-31T10:57:00Z

Ready to Commit as stated above.

com_redirect modal close check in fix

1dcaa1c

joomla-cms-bot added the PR-staging label Dec 21, 2017

Quy reviewed Dec 21, 2017

View reviewed changes

Add corect button type to buttons

71bb5d9

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 25, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 25, 2017

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 31, 2017

mbabker added this to the Joomla 3.8.4 milestone Jan 3, 2018

mbabker merged commit 20424c4 into joomla:staging Jan 3, 2018

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

com_redirect modal close button check in fix #19116

com_redirect modal close button check in fix #19116

Ruud68 commented Dec 21, 2017 •

edited by joomla-cms-bot

Loading

Quy Dec 21, 2017

brianteeman Dec 21, 2017

Ruud68 Dec 22, 2017

Quy commented Dec 23, 2017

Ruud68 commented Dec 23, 2017 via email

Quy commented Dec 24, 2017

sandewt commented Dec 25, 2017 •

edited

Loading

dgrammatiko commented Dec 25, 2017

ghost commented Dec 25, 2017

dgrammatiko commented Dec 25, 2017

Ruud68 commented Dec 29, 2017

dgrammatiko commented Dec 29, 2017

Ruud68 commented Dec 29, 2017

sandewt commented Dec 29, 2017 •

edited

Loading

sandewt commented Dec 29, 2017 •

edited

Loading

Ruud68 commented Dec 30, 2017

sandewt commented Dec 30, 2017 •

edited

Loading

Ruud68 commented Dec 31, 2017

dgrammatiko commented Dec 31, 2017 •

edited

Loading

ghost commented Dec 31, 2017

com_redirect modal close button check in fix #19116

com_redirect modal close button check in fix #19116

Conversation

Ruud68 commented Dec 21, 2017 • edited by joomla-cms-bot Loading

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Note

Quy Dec 21, 2017

Choose a reason for hiding this comment

brianteeman Dec 21, 2017

Choose a reason for hiding this comment

Ruud68 Dec 22, 2017

Choose a reason for hiding this comment

Quy commented Dec 23, 2017

Ruud68 commented Dec 23, 2017 via email

Quy commented Dec 24, 2017

sandewt commented Dec 25, 2017 • edited Loading

dgrammatiko commented Dec 25, 2017

ghost commented Dec 25, 2017

dgrammatiko commented Dec 25, 2017

Ruud68 commented Dec 29, 2017

dgrammatiko commented Dec 29, 2017

Ruud68 commented Dec 29, 2017

sandewt commented Dec 29, 2017 • edited Loading

sandewt commented Dec 29, 2017 • edited Loading

Ruud68 commented Dec 30, 2017

sandewt commented Dec 30, 2017 • edited Loading

Ruud68 commented Dec 31, 2017

dgrammatiko commented Dec 31, 2017 • edited Loading

ghost commented Dec 31, 2017

Ruud68 commented Dec 21, 2017 •

edited by joomla-cms-bot

Loading

sandewt commented Dec 25, 2017 •

edited

Loading

sandewt commented Dec 29, 2017 •

edited

Loading

sandewt commented Dec 29, 2017 •

edited

Loading

sandewt commented Dec 30, 2017 •

edited

Loading

dgrammatiko commented Dec 31, 2017 •

edited

Loading