Fix color field attributes onchange and required. #30743

Harmageddon · 2020-09-23T10:49:06Z

Summary of Changes

Fixed the attributes onchange and required for the color form field.
Improved the documentation in the layout files.

Testing Instructions

In some kind of form, create or edit a form field of type="color". Add the attributes required and/or onchange. Test with both color field layouts (control="simple" for the simple version).
Alternatively, you can use the demo plugin at https://github.com/Harmageddon/plg_demo_colorfield
Open the form.
Test the required attribute: Try saving the form without entering a value in the required color field.
Test the onchange attribute: Select a value for the field that has the onchange attribute and see whether the JavaScript handler is called.
Check the HTML source of the form for validity.

Actual result BEFORE applying this Pull Request

The values for the two attributes are printed directly in the source code, leading to invalid HTML like:

<input type="text" name="jform[params][color2]" id="jform_params_color2" value="none" placeholder="#rrggbb" class="minicolors hex" data-position="default" data-control="hue"alert('New value: ' + this.value); data-format="hex"/>

The onchange handler is not executed.

Required fields don't get the required attribute, neither the aria-required="true" attribute.

Expected result AFTER applying this Pull Request

Onchange handlers are executed correctly. Required fields get the required attribute.

Documentation Changes Required

The documentation of the color form field is very rudimentary and doesn't mention most of the available attributes. It should be improved overall.

Open Issues

The required attribute still has no effect for the simple color picker variant. The reason is that the empty value is not an empty string, but 'none', which counts as a value both for server-side and client-side validation. I'm not sure how to fix this.
I also fixed the documentation in the PHP code of the layouts files. For $control and $position, the original description was copy-pasted, so it should be changed to something more meaningful. However, I'm not sure about a good description here. Maybe @dgrammatiko can help here, as you created these layouts?

dgrammatiko · 2020-09-23T16:17:53Z

Maybe @dgrammatiko can help here, as you created these layouts

You can never escape git blame 😔.

I really don't remember what these 2 attributes are doing but the script docs gives some hints:

control: has a 'hue', so it should be the color selection part (the vertical one)
position: has a 'bottom left' so I guess it controls where the pop up will be positioned (relative to the input)

Please check the docs, I might got it wrong https://labs.abeautifulsite.net/jquery-minicolors/#settings

layouts/joomla/form/field/color/advanced.php

Co-authored-by: Quy <[email protected]>

Harmageddon · 2020-09-23T18:09:18Z

@dgrammatiko Okay, thank you! I'll look into it tomorrow.

Harmageddon · 2020-09-24T09:23:53Z

Updated the docblock accordingly. The other open issue (required validation for the simple color picker) remains, but I'd regard it as separate from this PR. If this is regarded as a bug, it should be fixed in another PR.

thednp · 2021-09-02T14:59:49Z

layouts/joomla/form/field/color/simple.php

 */

 $class    = ' class="' . trim('simplecolors chzn-done ' . $class) . '"';
 $disabled = $disabled ? ' disabled' : '';
 $readonly = $readonly ? ' readonly' : '';
+$required = $required ? ' required aria-required="true"' : '';


The aria-required="true" attribute is no longer required.

thednp · 2021-09-02T15:00:02Z

layouts/joomla/form/field/color/advanced.php

@@ -65,6 +67,8 @@
 $readonly     = $readonly ? ' readonly' : '';
 $hint         = strlen($hint) ? ' placeholder="' . $this->escape($hint) . '"' : ' placeholder="' . $placeholder . '"';
 $autocomplete = ! $autocomplete ? ' autocomplete="off"' : '';
+$required     = $required ? ' required aria-required="true"' : '';


The aria-required="true" attribute is no longer required.

dgrammatiko · 2021-09-02T15:01:49Z

@thednp hey, quick question: did you manage to get that wasm scss compiler working?

thednp

Some cleanup needed. I hope devs push this to J4.x as well.

thednp · 2021-09-02T15:05:42Z

@thednp hey, quick question: did you manage to get that wasm scss compiler working?

Yes man, thank you for asking. It was a lot of PHP "autoresolve" work.

dgrammatiko · 2021-09-02T15:13:38Z

Yes man, thank you for asking. It was a lot of PHP "autoresolve" work.

The reason I asked you was a recent discussion at #35403
Glad to hear that you've figured it out 👍

dgrammatiko · 2021-09-02T15:14:41Z

Some cleanup needed. I hope devs push this to J4.x as well.

Once you're ready let me know and I'll give it a test

thednp · 2021-09-02T15:18:53Z

Yes man, thank you for asking. It was a lot of PHP "autoresolve" work.

The reason I asked you was a recent discussion at #35403
Glad to hear that you've figured it out 👍

I'm using your sass.js implementation with a custom script to run via a blank custom field and works excellent, the only caveat is file names: if multiple files in multiple locations have same name, it might skip some of them in the auto-resolve process, other than that, it's alright.

So yea, I'm not going for the scssphp implementation any time soon, as you nicely advised (thanks).

Quy · 2021-09-02T15:28:30Z

Some cleanup needed. I hope devs push this to J4.x as well.

@thednp Is this PR okay other than your suggested changes? Changes in v3.10 will be upmerged into v4.

thednp · 2021-09-02T15:33:01Z

layouts/joomla/form/field/color/simple.php

 */

 $class    = ' class="' . trim('simplecolors chzn-done ' . $class) . '"';
 $disabled = $disabled ? ' disabled' : '';
 $readonly = $readonly ? ' readonly' : '';
+$required = $required ? ' required aria-required="true"' : '';
+$onchange = $onchange ? ' onchange="' . $onchange . '"' : '';

 // Include jQuery
 JHtml::_('jquery.framework');


JHtml::_('jquery.framework');

This is surely different in J4.0

thednp · 2021-09-02T15:33:33Z

layouts/joomla/form/field/color/advanced.php

@@ -9,40 +9,42 @@

 defined('_JEXEC') or die;

+use Joomla\CMS\Language\Language;


Why add this here? I don't think it's used anyway.

I guess it was autamatically added by the IDE due to

* @var Language $lang Language that is active on the site.

thednp · 2021-09-02T15:37:15Z

@Quy if my concerns get resolved, all should be fine, I even tested the changes in regards to "onchange", successfully.

Quy · 2021-09-02T15:45:33Z

@thednp The author appears to be inactive on GitHub so if no response in a few days, then I will make the changes. Thank you so much for your feedback!!

thednp · 2021-09-08T16:40:27Z

@dgrammatiko do you think dart.sass would work instead of sass.js by medialize?

In my initial impression, I might need some webpack action to create a bundle similar to the one by medialize. Your thoughts?

dgrammatiko · 2021-09-08T17:11:52Z

@thednp it should, but I haven't tried it. And yes you will need some sort of bundler (I'm in favor of Rollup as their docs are way more approachable and also it's a better bundler for most use cases)

thednp · 2021-09-09T15:22:34Z

@thednp it should, but I haven't tried it. And yes you will need some sort of bundler (I'm in favor of Rollup as their docs are way more approachable and also it's a better bundler for most use cases)

I've done some tests into it and haven't managed to get to "consume" the node modules and expose them for web, I used the ESM (nppm install esm) which aims to bridge the node and ES APIs, so I'm open to any suggestion you might have.

dgrammatiko · 2021-09-09T15:38:31Z

@thednp I think dart sass is not yet ready for the browser: sass/dart-sass#25 (comment) so for the time stay with sass.js: medialize/sass.js#135

Also, there are some other side projects from the (dart) team so it might be a browser-ready version there, check their repo's and their branches...

thednp · 2021-09-09T17:01:37Z

I already checked that (sass/dart-sass#25 (comment)), however, even if I change files manually and replace all require() instances with their module code I'm still getting tons of errors.

I actually don't know how to bundle npm modules together the way I manage to do with ES6+ modules.

zero-24 · 2022-06-12T18:15:36Z

@dgrammatiko @thednp can you please confirm what the status here is and whether it should be better closed here and moved to j4 as mentiond inline there are some changes between j3 and j4 already.

zero-24 · 2022-07-21T17:11:41Z

Will close here for now. Thanks for the work done here :)

Harmageddon added 2 commits September 23, 2020 12:32

Fixed color field attributes onchange and required.

aa28ff5

Removed unused import.

a6ba863

joomla-cms-bot added the PR-staging label Sep 23, 2020

Quy reviewed Sep 23, 2020

View reviewed changes

layouts/joomla/form/field/color/advanced.php Outdated Show resolved Hide resolved

Changed line orders based on suggestion.

10e2c3f

Co-authored-by: Quy <[email protected]>

Fixed documentation.

9c31c9c

zero-24 added PR-3.10-dev and removed PR-staging labels Aug 9, 2021

Quy mentioned this pull request Sep 2, 2021

[4.x][BUG] Color form field onchange property not handled properly #35464

Closed

thednp reviewed Sep 2, 2021

View reviewed changes

thednp suggested changes Sep 2, 2021

View reviewed changes

thednp reviewed Sep 2, 2021

View reviewed changes

zero-24 closed this Jul 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix color field attributes onchange and required. #30743

Fix color field attributes onchange and required. #30743

Harmageddon commented Sep 23, 2020 •

edited

Loading

dgrammatiko commented Sep 23, 2020

Harmageddon commented Sep 23, 2020

Harmageddon commented Sep 24, 2020

thednp Sep 2, 2021

thednp Sep 2, 2021

dgrammatiko commented Sep 2, 2021

thednp left a comment •

edited

Loading

thednp commented Sep 2, 2021

dgrammatiko commented Sep 2, 2021

dgrammatiko commented Sep 2, 2021

thednp commented Sep 2, 2021

Quy commented Sep 2, 2021

thednp Sep 2, 2021

thednp Sep 2, 2021

dgrammatiko Sep 2, 2021

thednp commented Sep 2, 2021

Quy commented Sep 2, 2021

thednp commented Sep 8, 2021

dgrammatiko commented Sep 8, 2021

thednp commented Sep 9, 2021

dgrammatiko commented Sep 9, 2021 •

edited

Loading

thednp commented Sep 9, 2021

zero-24 commented Jun 12, 2022

zero-24 commented Jul 21, 2022

		@@ -9,40 +9,42 @@

		defined('_JEXEC') or die;

		use Joomla\CMS\Language\Language;

Fix color field attributes onchange and required. #30743

Fix color field attributes onchange and required. #30743

Conversation

Harmageddon commented Sep 23, 2020 • edited Loading

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

Open Issues

dgrammatiko commented Sep 23, 2020

Harmageddon commented Sep 23, 2020

Harmageddon commented Sep 24, 2020

thednp Sep 2, 2021

Choose a reason for hiding this comment

thednp Sep 2, 2021

Choose a reason for hiding this comment

dgrammatiko commented Sep 2, 2021

thednp left a comment • edited Loading

Choose a reason for hiding this comment

thednp commented Sep 2, 2021

dgrammatiko commented Sep 2, 2021

dgrammatiko commented Sep 2, 2021

thednp commented Sep 2, 2021

Quy commented Sep 2, 2021

thednp Sep 2, 2021

Choose a reason for hiding this comment

thednp Sep 2, 2021

Choose a reason for hiding this comment

dgrammatiko Sep 2, 2021

Choose a reason for hiding this comment

thednp commented Sep 2, 2021

Quy commented Sep 2, 2021

thednp commented Sep 8, 2021

dgrammatiko commented Sep 8, 2021

thednp commented Sep 9, 2021

dgrammatiko commented Sep 9, 2021 • edited Loading

thednp commented Sep 9, 2021

zero-24 commented Jun 12, 2022

zero-24 commented Jul 21, 2022

Harmageddon commented Sep 23, 2020 •

edited

Loading

thednp left a comment •

edited

Loading

dgrammatiko commented Sep 9, 2021 •

edited

Loading