[4.0][RFC] Custom Element: Joomla! Toolbar button #19635

Fedik · 2018-02-10T12:53:04Z

This is sketch of <joomla-toolbar-button/>.
It allow us to move most of inline onclick="" to separate file.

From JavaScript side it mostly ready, but PHP require some more editing.

Any thoughts, is it need?

dgrammatiko · 2018-02-10T13:10:01Z

@Fedik one thing that would be awesome to implement is hide/show buttons depending on the selected rows of the list view. I started working on that idea so maybe something helpful there: https://github.com/joomla/joomla-cms/compare/4.0-dev...dgt41:§4.0-dev-toolbar-cleanup?expand=1

PS And yes this is needed to achieve CSP strict mode! Great stuff

dgrammatiko · 2018-02-10T13:12:47Z

build/webcomponents/js/toolbar-button/toolbar-button.js

+
+                if (form.boxchecked.value == 0) {
+                    perform = false;
+                    Joomla.renderMessages({'error': [Joomla.JText._('JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST')]});


how about: this.setAttribute('hidden', true); instead of the stupid alert?

Of course you need a way to exclude buttons from the hide/show behaviour (eg new, options, etc)

check how i did it: https://github.com/joomla/joomla-cms/compare/4.0-dev...dgt41:§4.0-dev-toolbar-cleanup?expand=1#diff-281a38474e3458134cb4943c1aa2fa73R55

for now it just keeps an old logic

I'm fine let's do this in steps...

brianteeman · 2018-02-10T13:21:15Z

One issue with the current code we are using is that the "icon" name is tied to the button id. So for example in the sysinfo there is an a11y fail as the id is the same on both buttons because the icon is the same. it would be good if that could be avoided with this new code

Fedik · 2018-02-10T13:24:07Z

would be awesome to implement is hide/show buttons depending on the selected rows of the list view

that also would be cool,
but it another complex task, I think will be better to separate this to 2 task, it more easy to test, and sync in case of conflict

dgrammatiko · 2018-02-10T13:29:10Z

@Fedik one more thing about accessibility:
It would be better if you create a button element and put the contents of the ce in there (button.innerHTML = this.innerHTML).
The reason is that custom elements in fact are like span elements and since we want here to have all the good browser built in accessibility for buttons is better to create a button instead of manually patch the keyboard and mouse behaviours. To make this more clear try to tab to the buttons or enter/space when selected, we're missing crucial functionality here ;)

Fedik · 2018-02-10T13:50:48Z

@dgt41 good point,
hmm, I think tabindex="0" will do the work also, or?
because I not very like the idea of using these "extra wrappers" 😉

need to check

dgrammatiko · 2018-02-10T13:53:25Z

well it's not only tabindex you need to patch the key listeners as well. Basically because custom element extends HTMLElement just extending the base Html element there is no special functionality there. In v0 of ce you could extend other elements like class Something extends HTMLButtonElement, but I think this doesn't work anymore with v1

Fedik · 2018-02-10T14:27:24Z

yes, that was another idea in my head,
should be no problem:

C-Lodder · 2018-02-11T11:40:35Z

build/webcomponents/js/toolbar-button/toolbar-button.js

+
+            // If list selection are required, set button to disabled by default
+            if (this.listConfirmation) {
+                button.setAttribute('disabled', 'disabled');


button.setAttribute('disabled', true);

C-Lodder · 2018-02-11T11:41:12Z

build/webcomponents/js/toolbar-button/toolbar-button.js

-                Joomla.submitbutton(this.task, form, this.formValidation);
+                    // Check whether we have selected something
+                    if (this.formElement.boxchecked.value == 0) {
+                        this.buttonElement.setAttribute('disabled', 'disabled');


button.setAttribute('disabled', true);

Fedik · 2018-02-11T13:15:12Z

@brianteeman the issue with non-unique ID is fixed

@dgt41 any idea why the link button use <button onlick="bla/link"> instead of a <a href="bla/link">?

brianteeman · 2018-02-11T13:34:00Z

libraries/src/Toolbar/Button/StandardButton.php

+	 * @param   boolean $list  True to allow lists
+	 * @param   boolean $group Does the button belong to a group?
+	 *
+	 * @param   string  $form            The form CSS selector eg '#adminForm'


remove extra tab

brianteeman · 2018-02-11T13:34:31Z

libraries/src/Toolbar/ToolbarHelper.php

@@ -465,37 +465,44 @@ public static function trash($task = 'remove', $alt = 'JTOOLBAR_TRASH', $check =
 	 *
 	 * @param   string  $task  An override for the task.
 	 * @param   string  $alt   An override for the alt text.
+	 * @param   boolean $group True or false
+	 *
+	 * @param   string  $form            The form ID


remove extra tab

dgrammatiko · 2018-02-11T13:39:28Z

@Fedik I guess the href attribute wasn't used due to the alert that had to be triggered if the link was referring to a list row element and no row element was selected...

Fedik · 2018-02-11T13:43:59Z

@dgt41 hmm, no should be something diferent, because the link not require a list selection:

I could change it to simple <a href=""> tag, and it will work in same way.
But maybe there was a reason why we have a <button> there, something with a11y?

asika32764 · 2018-02-13T16:38:36Z

Just reference I create a PR #19670 to handle the php side.

dgrammatiko · 2018-02-17T22:52:54Z

@Fedik one more thing here, a few months back I implemented a shortcut key handler for CTRL( or CMD)+S. That code somehow disappeared in some sync of the repos but you can check it here: joomla/40-backend-template#55. So it would be cool to have it back, probably it needs some changes as the codebase progressed a bit since

C-Lodder · 2018-02-18T09:25:22Z

@dgt41 I swear you've added that feature like 4 times now

Fedik · 2018-02-18T14:46:17Z

@dgt41 I have restored "control+s"

However if we can use one of these:
https://github.com/RobertWHurst/KeyboardJS
https://github.com/ccampbell/mousetrap
then I can integrate keyboard combos (shortcuts) to JoomlaToolbarButton (instead of hardcoding in core.js) that will allow to add own combos (one or multiple) to each toolbar button, in theory.

C-Lodder · 2018-02-18T14:53:04Z

media/system/js/core.js

+	document.addEventListener( 'DOMContentLoaded', function() {
+		if (Joomla.getOptions( 'keySave' ) ) {
+			document.addEventListener("keydown", function(e) {
+				if (e.keyCode == 83 && (navigator.platform.match("Mac") ? e.metaKey : e.ctrlKey)) {


Please use https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key in favour of keyCode

dgrammatiko · 2018-02-18T17:24:23Z

@Fedik the code was in core.js because we didn't have a toolbar.js or had a clue that custom elements was going to be part of J4 back then. Now the approach should be a little different IMHO:
in the CE before the class define a const like:

	const keyboardSpecialKeys = (key, modifier, task) => {
if (modifier === 'control') { modifier = (navigator.platform.match("Mac") ? e.metaKey : e.ctrlKey); }
		document.addEventListener("keydown", function(e) {
			if (e.key === key && e.key === modifier ) {
				e.preventDefault();
				Joomla.submitbutton(task); // Pretty sure we can use submitForm here
			}
		}, false);
	}

in the class connectedCallback function:

keyboardSpecialKeys(83, 'control', 'article.save')

Or you can use this.keyboardSpecialKeys(...) if you get the function inside the class as another option.
The main point is that this functionality is tightly connected to the buttons (crtl+s is for save, but we can introduce more later) so I think the whole code should be in the custom elements.

dneukirchen · 2018-02-24T16:41:08Z

I would not put too much stuff into this.
We can always add shortcuts and bonus stuff later.

But we should improve this a little:

There is no need to put the listselection logic INSIDE the custom element. This knowledge belongs in the outside world. A <joomla-toolbar-button> is - depending on those or other outside conditions - either disabled, expanded, hidden or whatever. Imagine a "merge" button which merges 2 db entries. With the current implementation the button gets enabled when at least one item in the list is checked, which makes no sense. There is also no need to show the ugly alert to the user when he clicks a button he should not be able to click at all. UI should indicate (hidden/disabled) the state.
Something similar is true for the confirm attribute. Its not flexible enough. Perhaps we should put this into another element like <joomla-confirm-toolbar-button>? Or implement some onBeforeSubmit callback, where we can (later) replace the ugly browser confirm with a more user-friendly, modern confirmation dialog? Still new to custom elements and i dont know whats possible. Not that i expect you to implement a modern confirm dialog in this PR, but i think putting the confirm INSIDE the custom element is not flexible enough to fulfill the requirements of modern UIs.
I dont like that the toolbar button is coupled to a form and especially the form validation... The formvalidation should not depend on an attribute of a toolbar button... Could be also a candidate for an onBeforeSubmit Callback / Event (see above)... ?

Conflicts: media/system/js/core.min.js

Fedik · 2018-03-03T13:14:15Z

There is no need to put the listselection logic INSIDE the custom element

I think it is fine,
but if there a better solution, then it can be better 😄

Imagine a "merge" button which merges 2 db entries

There could be 3 db entries 😉
for it we can make <joomla-toolbar-button list-selection="3"/> or <joomla-toolbar-button list-selection="some fancy condition"/>
But it require extra work, and clear decision.

There is also no need to show the ugly alert to the user when he clicks a button he should not be able to click at all. UI should indicate (hidden/disabled) the state.

That actually what we have here disabled state 😉

Perhaps we should put this into another element like ? Or implement some onBeforeSubmit callback, where we can (later) replace the ugly browser confirm with a more user-friendly, modern confirmation dialog?

I would not do a Zoo of "toolbar-buttons" which on 90% will be copy of each other.
Technically <joomla-toolbar-button/> is a wrapper around Joomla.submitbutton which is made to be overridden by extension developer. So technically it is onBeforeSubmit callback 😉

I dont like that the toolbar button is coupled to a form and especially the form validation...

It is not very coupled.
There just a marker/flag which passed to Joomla.submitbutton that say "I would like to validate the form". But the validation is in Joomla.submitbutton, or where developer decide.
It allow to avoid hard-coding, like we have:
if ((task == 'cancel' ) || document.formvalidator.isValid( form))

Quy · 2018-03-03T13:56:53Z

layouts/joomla/toolbar/apply.php

+$group    = $displayData['group'];
+$task     = '';
+$list     = !empty($displayData['list'])     ? ' list-selection' : '';
+$form     = !empty($displayData['form'])     ? ' form=' . $displayData['form'] . '"' : '';


Missing " after form=?

wilsonge · 2018-05-14T12:02:25Z

I've merged the PHP API Updates which of course has made this blow up with conflicts. Can we get this back in sync

Fedik · 2018-05-14T12:17:29Z

I will check

Fedik · 2018-06-02T16:25:22Z

I made new one, please check there #20650

Fedik added 4 commits February 10, 2018 13:50

joomla-toolbar-button for standard button

5ff1aef

JoomlaToolbarButton custom element

4122250

clean up

761209b

move to webcomponents

09e572d

joomla-cms-bot added PR-4.0-dev RFC Request for Comment labels Feb 10, 2018

dgrammatiko reviewed Feb 10, 2018

View reviewed changes

Fedik added 5 commits February 11, 2018 11:07

Merge branch '4.0-dev' into toolbar-button

db79853

make a button to support button behavior

a16fbe6

Disable the button if list selection are required

7afd80e

Update Apply toolbar button

23b8847

Add minified version

ef55a80

C-Lodder reviewed Feb 11, 2018

View reviewed changes

Fedik added 2 commits February 11, 2018 13:47

disabled:true

b15353e

Make sure the id of toolbar buttons are unique

bcaeab4

brianteeman reviewed Feb 11, 2018

View reviewed changes

Better handling of list selection

abc913d

code style

74a18fc

asika32764 added a commit to asika32764/joomla-cms that referenced this pull request Feb 16, 2018

Sync unique id from joomla#19635

95b7dc2

asika32764 added a commit to asika32764/joomla-cms that referenced this pull request Feb 16, 2018

Sync formValidation from joomla#19635

d2494d9

Fedik added 4 commits February 18, 2018 15:53

Sync. Merge branch '4.0-dev' into toolbar-button

0346c3d

Use a real button

7509dc7

Update minified version

55c045d

restore control+s

a415aa6

C-Lodder reviewed Feb 18, 2018

View reviewed changes

dgrammatiko mentioned this pull request Feb 23, 2018

Too many buttons on the toolbar joomla/40-backend-template#348

Closed

Fedik mentioned this pull request Feb 24, 2018

Remove inline scripts/components/com content/tmpl/form/edit.php joomla-projects/joomla-es6#63

Merged

Fedik added 3 commits March 3, 2018 14:47

Merge branch '4.0-dev' into toolbar-button

0757815

Conflicts: media/system/js/core.min.js

update core.min.js after conflict

6017c0f

revert control+s for now

318b686

c.s.

5ee2fb2

Quy reviewed Mar 3, 2018

View reviewed changes

Fedik added 2 commits March 3, 2018 16:10

fix karma core.js test of isChecked

5c9e73b

fix typo

8523793

joomla-cms-bot added the Unit/System Tests label Mar 3, 2018

Fedik mentioned this pull request Jun 2, 2018

[4.0][RFC] Custom Element: Joomla! Toolbar button #20650

Merged

Fedik closed this Jun 2, 2018

Fedik deleted the toolbar-button branch June 13, 2018 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4.0][RFC] Custom Element: Joomla! Toolbar button #19635

[4.0][RFC] Custom Element: Joomla! Toolbar button #19635

Fedik commented Feb 10, 2018

dgrammatiko commented Feb 10, 2018 •

edited

Loading

dgrammatiko Feb 10, 2018

dgrammatiko Feb 10, 2018

dgrammatiko Feb 10, 2018

Fedik Feb 10, 2018

dgrammatiko Feb 10, 2018

brianteeman commented Feb 10, 2018

Fedik commented Feb 10, 2018

dgrammatiko commented Feb 10, 2018

Fedik commented Feb 10, 2018

dgrammatiko commented Feb 10, 2018 •

edited

Loading

Fedik commented Feb 10, 2018 •

edited

Loading

C-Lodder Feb 11, 2018

C-Lodder Feb 11, 2018

Fedik commented Feb 11, 2018

brianteeman Feb 11, 2018

brianteeman Feb 11, 2018

dgrammatiko commented Feb 11, 2018

Fedik commented Feb 11, 2018 •

edited

Loading

asika32764 commented Feb 13, 2018

dgrammatiko commented Feb 17, 2018 •

edited

Loading

C-Lodder commented Feb 18, 2018

Fedik commented Feb 18, 2018

C-Lodder Feb 18, 2018

dgrammatiko commented Feb 18, 2018 •

edited

Loading

dneukirchen commented Feb 24, 2018 •

edited

Loading

Fedik commented Mar 3, 2018

Quy Mar 3, 2018

wilsonge commented May 14, 2018

Fedik commented May 14, 2018

Fedik commented Jun 2, 2018

[4.0][RFC] Custom Element: Joomla! Toolbar button #19635

[4.0][RFC] Custom Element: Joomla! Toolbar button #19635

Conversation

Fedik commented Feb 10, 2018

dgrammatiko commented Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianteeman commented Feb 10, 2018

Fedik commented Feb 10, 2018

dgrammatiko commented Feb 10, 2018

Fedik commented Feb 10, 2018

dgrammatiko commented Feb 10, 2018 • edited Loading

Fedik commented Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fedik commented Feb 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrammatiko commented Feb 11, 2018

Fedik commented Feb 11, 2018 • edited Loading

asika32764 commented Feb 13, 2018

dgrammatiko commented Feb 17, 2018 • edited Loading

C-Lodder commented Feb 18, 2018

Fedik commented Feb 18, 2018

Choose a reason for hiding this comment

dgrammatiko commented Feb 18, 2018 • edited Loading

dneukirchen commented Feb 24, 2018 • edited Loading

Fedik commented Mar 3, 2018

Choose a reason for hiding this comment

wilsonge commented May 14, 2018

Fedik commented May 14, 2018

Fedik commented Jun 2, 2018

dgrammatiko commented Feb 10, 2018 •

edited

Loading

dgrammatiko commented Feb 10, 2018 •

edited

Loading

Fedik commented Feb 10, 2018 •

edited

Loading

Fedik commented Feb 11, 2018 •

edited

Loading

dgrammatiko commented Feb 17, 2018 •

edited

Loading

dgrammatiko commented Feb 18, 2018 •

edited

Loading

dneukirchen commented Feb 24, 2018 •

edited

Loading