UI Disabled Inputs #1305

tfamula · 2018-11-09T14:54:05Z

Possibility to make inputs disabled in Forms and (future) Filters.
The need of disabled inputs came up by implementing the Filter

tfamula · 2018-11-09T15:08:24Z

src/UI/templates/default/Input/tpl.tag_input.html

@@ -1,5 +1,5 @@
 <div id="container-{ID}" class="form-control form-control-sm il-input-tag">
-	<input type="text" id="{ID}" value="{VALUE_COMMA_SEPARATED}" class="form-control form-control-sm"/> <input type="hidden" id="template-{ID}" value='{NAME}[]'>
+	<input type="text" id="{ID}" value="{VALUE_COMMA_SEPARATED}"<!-- BEGIN disabled --> {DISABLED}<!-- END disabled --> class="form-control form-control-sm"/> <input type="hidden" id="template-{ID}" value='{NAME}[]'>


Maybe this is not enough here. Technically, the tag-input will be disabled, so that the user cannot modify it. But visually, it still looks enabled because of the < div > over it. The user can also click on it and on the first click, even a pointer comes up.
My idea would be to additionally extend the < div > with a "disabled" class. Adding some css for the color and pointer-event would make the division visually disabled for the user.

@tfamula sounds reasonable. @chfsx you have to most know-how about the tax, can you leave a short feedback here?

klees · 2018-11-09T15:17:37Z

@tfamula Do you possess some mind-reading device? @Amstutz and me talked about this like four hours ago. Thx a lot already, I will have a look into this in the next week. Have a nice weekend later on!

Amstutz

Hi @tfamula really nice work and awsome timing. As @klees pointed out, where where just writting this on a todo list of ours when this came in. Nice. The interface change lgtm. I think we can discuss this directly at the JF since the change of the interface is really small. I'll attach the according label.

Nice work on the examples. Note that for a merge you need to:

Add some test coverage of this new "feature".

Amstutz · 2018-11-10T06:17:49Z

src/UI/templates/default/Input/tpl.tag_input.html

@@ -1,5 +1,5 @@
 <div id="container-{ID}" class="form-control form-control-sm il-input-tag">
-	<input type="text" id="{ID}" value="{VALUE_COMMA_SEPARATED}" class="form-control form-control-sm"/> <input type="hidden" id="template-{ID}" value='{NAME}[]'>
+	<input type="text" id="{ID}" value="{VALUE_COMMA_SEPARATED}"<!-- BEGIN disabled --> {DISABLED}<!-- END disabled --> class="form-control form-control-sm"/> <input type="hidden" id="template-{ID}" value='{NAME}[]'>


@tfamula sounds reasonable. @chfsx you have to most know-how about the tax, can you leave a short feedback here?

klees

LGTM in general, and thx a lot again.

As far as I understand investigations of @chfsx into the general state of the disabled-Attribute, not all browsers will properly display all types of disabled fields, so we might need some additional CSS at some point for some browser.

The changes in this PR currently do not cover Group, Section, MultiSelect, Numeric and Password (or I am missing something crucial). These are Inputs (hence get the disabled-property by the base class) but currently do not show any meaningful behaviour when the property is set. For Group and Section users imo would expect, that the contained inputs are disabled, the others should be just disabled like all other fields.

Did you check what happens when you actually process input from disabled fields?

matthiaskunkel · 2018-11-12T13:01:04Z

Jour Fixe, 12 NOV 2018 : We accept this PR for trunk. Thomas will tackle Group and Section inputs, Richard will take care of Required inputs before committing it to trunk.

alex40724 · 2018-11-13T10:52:30Z

Did you check what happens when you actually process input from disabled fields?

Imo we should simply skip disabled inputs in the withRequest/withInput loop. We should not try to pass the values somehow through the request, since we cannot ensure that the values are not manipulated. Consumers must not construct the form differently for the initial presentation and the input processing.

This follows up on the content/data discussion. Imo we need a function that runs through the operations without taking the request data.

So basically a withInput without the lines:

$value = $post_input->getOr($this->getName(), "");
$clone = $this->withValue($value);

getData should always deliver all inputs imo.

klees · 2018-11-13T10:56:15Z

@alex40724 Yes, I agree. We should skip the client-server-roundtrip that ilPropertyFormGUI performs. I do not expect this roundtrip to happen in the current implementation, though. As @tfamula explained in the JF yesterday, the value set on the disabled input server-side seems to be used in processing of the response, without having been passed to the client in the meantime. This behaviour seems to be correct for me.

tfamula · 2018-11-15T15:38:10Z

Hi @Amstutz and @klees,
we made some modifications to prevent getting the value for disabled inputs from the post-request and instead getting the value from the client side.

We also added the possibility to make Group, Dependant Group and Section disabled. The Inputs, which are located inside these three types of Inputs, will be made disabled technically and visually.
Let me know if you wish additional visual modification for these Inputs (e.g. greying out the whole division). Please consider this would be possible for Dependant Group and Section, but not for Group, because it has no own template and therefore we have no div which we can adjust. Since this would not be consistent, I would leave it the way it looks now.

As far as I understand investigations of @chfsx into the general state of the disabled-Attribute, not all browsers will properly display all types of disabled fields, so we might need some additional CSS at some point for some browser.

My investigations are different. For all Inputs which we have in the KS currently, the disabled attribute is provided by all common browsers from the beginning. (see here -> Browser Support)

chfsx · 2018-11-20T16:07:29Z

My investigations were for the attribute "readonly" not "disabled": https://www.w3schools.com/tags/att_readonly.asp

klees · 2018-11-20T16:47:48Z

Hi @tfamula, I'm really busy this week, but I promise you to give a feedback till end of the week. Sorry for the delay.

tfamula · 2018-11-21T13:07:33Z

Hi @klees, no problem. I just finished everything that was missing in my eyes and made some small changes here and there.

I would like to say a few words about the Tag Input:
After analysing the html, I found at least five different inputs, which take part in the Tag Input. I could not find out which of them is the "main input". Trying to give one or more them the disabled attribute caused strange behaviour. Moreover, this would not solve our problem anyway: making the input disabled visually. For the request, we do not even need this attribute, because this is handled by the methods withDisabled and isDisabled technically.
I decided to remove the disabled attribute from the chosen input and instead extend the class of the div with this property. So my solution is to prevent user changes by styling the disabled (pseudo) input with the same background-color as it would be with the disabled attribute, and making it not usable by mouse, touch and keyboard with the help of css/js.
I hope my proposal is acceptable for you.

klees

LGTM, besides the two minor issues in the comment. Thx a lot again.

@Amstutz: do you want to have another look?

klees · 2018-11-23T14:29:18Z

src/UI/Implementation/Component/Input/Field/Group.php

@@ -42,4 +42,15 @@ public function __construct(
 		$this->inputs = $inputs;
 	}

+	function withDisabled($is_disabled) {


Please add public here.

klees · 2018-11-23T14:29:41Z

src/UI/Implementation/Component/Input/Field/Group.php

+		{
+			$inputs[$key] = $input->withDisabled($is_disabled);
+		}
+		$this->inputs = $inputs;


This violates immutability of the object. Please clone before you assign.

Amstutz

Thx a lot for adding the tests. @klees you may merge as soon as your points are taken care of.

tfamula · 2018-11-27T13:11:50Z

Hi @klees,
the immutability violation was solved. We are ready to merge :)

Richard will take care of Required inputs before committing it to trunk.

I think this is not part of the PR and will be solved by you separately.

Interface, implementation and examples for disabled inputs

0c93f33

tfamula assigned Amstutz and klees Nov 9, 2018

tfamula added kitchen sink jour fixe labels Nov 9, 2018

tfamula commented Nov 9, 2018

View reviewed changes

Amstutz requested changes Nov 10, 2018

View reviewed changes

klees requested changes Nov 12, 2018

View reviewed changes

tfamula removed the jour fixe label Nov 14, 2018

tfamula added 2 commits November 15, 2018 15:41

Prevent request for disabled inputs

d5da4e5

Disabled state for (dependant) group and section

fd1d228

Tests for disabled inputs

8b3b3bd

Changes for disabled Tag Input

f3e28e9

Removed comment

87322a6

klees approved these changes Nov 23, 2018

View reviewed changes

Amstutz approved these changes Nov 27, 2018

View reviewed changes

Solved violation with immutability

020a66a

klees merged commit a1bc22b into ILIAS-eLearning:trunk Nov 27, 2018

tfamula mentioned this pull request Feb 13, 2019

UI Filters (Implementation) #1253

Closed

2 tasks

tfamula deleted the 6_0_ks_disabled_input branch February 23, 2023 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI Disabled Inputs #1305

UI Disabled Inputs #1305

tfamula commented Nov 9, 2018

tfamula Nov 9, 2018 •

edited

Loading

Amstutz Nov 10, 2018

klees commented Nov 9, 2018

Amstutz left a comment •

edited

Loading

Amstutz Nov 10, 2018

klees left a comment

matthiaskunkel commented Nov 12, 2018

alex40724 commented Nov 13, 2018 •

edited

Loading

klees commented Nov 13, 2018

tfamula commented Nov 15, 2018

chfsx commented Nov 20, 2018

klees commented Nov 20, 2018

tfamula commented Nov 21, 2018

klees left a comment

klees Nov 23, 2018

klees Nov 23, 2018 •

edited

Loading

Amstutz left a comment

tfamula commented Nov 27, 2018

UI Disabled Inputs #1305

UI Disabled Inputs #1305

Conversation

tfamula commented Nov 9, 2018

tfamula Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

Amstutz Nov 10, 2018

Choose a reason for hiding this comment

klees commented Nov 9, 2018

Amstutz left a comment • edited Loading

Choose a reason for hiding this comment

Amstutz Nov 10, 2018

Choose a reason for hiding this comment

klees left a comment

Choose a reason for hiding this comment

matthiaskunkel commented Nov 12, 2018

alex40724 commented Nov 13, 2018 • edited Loading

klees commented Nov 13, 2018

tfamula commented Nov 15, 2018

chfsx commented Nov 20, 2018

klees commented Nov 20, 2018

tfamula commented Nov 21, 2018

klees left a comment

Choose a reason for hiding this comment

klees Nov 23, 2018

Choose a reason for hiding this comment

klees Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

Amstutz left a comment

Choose a reason for hiding this comment

tfamula commented Nov 27, 2018

tfamula Nov 9, 2018 •

edited

Loading

Amstutz left a comment •

edited

Loading

alex40724 commented Nov 13, 2018 •

edited

Loading

klees Nov 23, 2018 •

edited

Loading