? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
16 Mar 2022

Pull Request for Issue #37299

Summary of Changes

  • Load the showon script if there are showon attributes even in the options (one level down)

Testing Instructions

  • Open file: \joomla-cms\administrator\components\com_banners\forms\banner.xml
  • Find the state field and replace with the following
		<field
			name="state"
			type="list"
			label="JSTATUS"
			class="form-select-color-state"
			default="1"
			validate="options"
			showon="state!:X"
			>
			<option value="1" showon="sticky:0">JPUBLISHED</option>
			<option value="0" showon="sticky:0">JUNPUBLISHED</option>
			<option value="2" showon="sticky:0">JARCHIVED</option>
			<option value="-2" showon="sticky:0">JTRASHED</option>
		</field>
  • Create a new banner: /administrator/index.php?option=com_banners&view=banner&layout=edit
  • Toggle Banner's Pinned button. You'll see the Status options disappear after toggling. (You'll first have to click the status list in order to see it change. It would be better for it to update, but that's another issue)
  • Now remove the showon="state!:X" line from the code above. Press CTRL+F5.
  • Retry toggling, it's broken now.

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

avatar dgrammatiko dgrammatiko - open - 16 Mar 2022
avatar dgrammatiko dgrammatiko - change - 16 Mar 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 16 Mar 2022
Category Layout
avatar dgrammatiko dgrammatiko - change - 17 Mar 2022
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 17 Mar 2022
Category Layout Layout Libraries
avatar dgrammatiko dgrammatiko - change - 17 Mar 2022
The description was changed
avatar dgrammatiko dgrammatiko - edited - 17 Mar 2022
avatar toroworx
toroworx - comment - 17 Mar 2022

I just tested this patch and can confirm it fixes the issue.
Thanks for creating a patch! Awesome feature btw.

avatar richard67
richard67 - comment - 17 Mar 2022

@toroworx Could you go to the PR in the issue tracker here https://issues.joomla.org/tracker/joomla-cms/37300 and use the "Test this" button to submit your test result? Thanks in advance.

avatar toroworx
toroworx - comment - 17 Mar 2022

I have tested this item successfully on ac8bed2

I just tested this patch and can confirm it fixes the issue. After the patch you don't need to manually load the showon.min.js script anymore.

I just found out though: to replicate the issue, you have to make sure there's not a field with a showon attribute, else this will load the showon.min.js script, preventing you from replicating the issue. Nevertheless the patch works fine.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37300.

avatar toroworx toroworx - test_item - 17 Mar 2022 - Tested successfully
avatar joomla-cms-bot joomla-cms-bot - change - 18 Mar 2022
Category Layout Libraries Libraries
avatar dgrammatiko
dgrammatiko - comment - 18 Mar 2022

I have changed this PR so it will use the Platform wide $options['showonEnabled']

if ($this->showon)
{
$options['rel'] = ' data-showon=\'' .
json_encode(FormHelper::parseShowOnConditions($this->showon, $this->formControl, $this->group)) . '\'';
$options['showonEnabled'] = true;
}
which is held automatically here
if (!empty($options['showonEnabled']))
{
/** @var Joomla\CMS\WebAsset\WebAssetManager $wa */
$wa = Factory::getApplication()->getDocument()->getWebAssetManager();
$wa->useScript('showon');
}

avatar richard67
richard67 - comment - 18 Mar 2022

@toroworx Could you test again with the latest changes? Thanks in advance.

avatar toroworx
toroworx - comment - 18 Mar 2022

Seems to be broken right now.

Test instructions:

  1. Open file: \joomla-cms\administrator\components\com_banners\forms\banner.xml
  2. Find the state field and replace with the following
		<field
			name="state"
			type="list"
			label="JSTATUS"
			class="form-select-color-state"
			default="1"
			validate="options"
			showon="state!:X"
			>
			<option value="1" showon="sticky:0">JPUBLISHED</option>
			<option value="0" showon="sticky:0">JUNPUBLISHED</option>
			<option value="2" showon="sticky:0">JARCHIVED</option>
			<option value="-2" showon="sticky:0">JTRASHED</option>
		</field>
  1. Create a new banner: /administrator/index.php?option=com_banners&view=banner&layout=edit
  2. Toggle Banner's Pinned button. You'll see the Status options disappear after toggling.
    (You'll first have to click the status list in order to see it change. It would be better for it to update, but that's another issue)
  3. Now remove the showon="state!:X" line from the code above. Press CTRL+F5.
  4. Retry toggling, it's broken now.
avatar richard67
richard67 - comment - 18 Mar 2022

@toroworx Have you applied the changes in ListField.php AND also reverted the List.php to the original state like without this PR? Or have you maybe forgotten the latter?

avatar laoneo
laoneo - comment - 20 Mar 2022

@toroworx what do you expect instead of that the status are hidden? For me it works exact the same way as you did describe it in the last comment which is for me the expected behavior.

avatar toroworx
toroworx - comment - 20 Mar 2022

@richard67 I've used the Joomla Patch Tester. I reverted the patch, fetched data, and applied the patch.
Just to be sure I undid all that. Switched to the patch-17 branch, pulled in the latest version and retested (so testing 723f805). Again I can confirm the latest patch-17 version is broken.

@laoneo did you refresh after step 5? The definition of broken is: not hiding the Status options any more after toggling the banner's Pinned button (you have to click the Status option to view if the options are hidden). To clarify, this only happens if only option showon is used. So like this:

<field
		name="state"
		type="list"
		label="JSTATUS"
		class="form-select-color-state"
		default="1"
		validate="options"
>
	<option value="1" showon="sticky:0">JPUBLISHED</option>
	<option value="0" showon="sticky:0">JUNPUBLISHED</option>
	<option value="2" showon="sticky:0">JARCHIVED</option>
	<option value="-2" showon="sticky:0">JTRASHED</option>
</field>

If on the page somewhere showon is used within a field tag, the showon is loaded and the issue can't be replicated.

avatar dgrammatiko
dgrammatiko - comment - 20 Mar 2022

@toroworx you're right the latest patch is not working as expected

@laoneo there's problem with the List field (which is also the base for many other fields). The showon is loaded only if the field is declaring a showon attribute in the root, if it is in a nested option the showon is not loaded. This affects many other fields. My first approach was naive, eg have a new option passed in the layout data but I realised that will not work for any of the fields that are extending the List field. Also I have no clue how this can be achieved without some refactoring (eg the getLayoutData or the getOptions fn)

avatar laoneo
laoneo - comment - 20 Mar 2022

Thanks for clarification, now I got it.

avatar dgrammatiko dgrammatiko - change - 20 Mar 2022
The description was changed
avatar dgrammatiko dgrammatiko - edited - 20 Mar 2022
avatar laoneo
laoneo - comment - 28 Mar 2022

@dgrammatiko did your last changes fix the bug which was found by @toroworx?

avatar dgrammatiko
dgrammatiko - comment - 1 Apr 2022

@laoneo it should solve the reported issue, if not then it should be closed

avatar toroworx
toroworx - comment - 1 Apr 2022

/edited:
I just pulled the latest commits of your patch-17 branch and tested it. It doesn't fix it. Still same state as before the patch. showon.min.js is not loaded. It's still acting like explained above.

I appreciate your pull request, but why do you suggest it's fixed before testing it? With the provided instructions it shouldn't take long.

avatar dgrammatiko
dgrammatiko - comment - 1 Apr 2022

but why do you suggest it's fixed before testing it? With the provided #37300 (comment) it shouldn't take long.

I did and the showon file was loaded. Anyways someone else could have better lack with this

avatar dgrammatiko dgrammatiko - change - 1 Apr 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-04-01 15:33:49
Closed_By dgrammatiko
avatar dgrammatiko dgrammatiko - close - 1 Apr 2022
avatar toroworx
toroworx - comment - 1 Apr 2022

I did and the showon file was loaded. Anyways someone else could have better lack with this

Okay, I'm sorry then. Weird it did work for you and not for me.

@dgrammatiko I've edited your approach and this works for me now. Can you please review the approach?

toroworx@d428b1f

avatar richard67
richard67 - comment - 1 Apr 2022

I‘ve re-opened the issue. But @dgrammatiko could you check the commit linked by @toroworx ?

avatar richard67
richard67 - comment - 1 Apr 2022

Meanwhile @toroworx has made a PR, see #37451 .

avatar toroworx
toroworx - comment - 1 Apr 2022

Meanwhile @toroworx has made a PR, see #37451 .

Thanks for reopening @richard67. I don't want to hijack anything, if I'll need to close something please let me know!

Add a Comment

Login with GitHub to post a comment