Media type is disabled when disabled="false" #1188

Closed
wants to merge 9 commits into
from

Projects

None yet

8 participants

@benjaminpick

#1151 introduced unwanted behaviour: any value present with key "disabled" is interpreted as true (also 'false').

@elinw

In other fields we would also allow the equivalent of disabled='disabled' to mean true.

@benjaminpick
@realityking
Joomla! member

Since disabled is a very common argument we should consider moving it to JFormField (like the check for required). This could save some duplicate code and make sure the fields behave the same.

@AmyStephen

Understand the inconsistency and the reason this must be patched.

However, I don't think it's a good practice to treat 'true' as a literal.

IMO, it would be best to make certain the Boolean true and false values work properly (or change it to '0' and '1', or something).

@benjaminpick
@realityking
Joomla! member

Yep, that's how I imagined it :)

Now we'd only need to change the fields to use $this->disabled.

Next release will be 12.1 but this pull probably won't make it before 12.2.

@mbabker
Joomla! member

Looking back across the other field types, I obviously didn't check to make sure I was consistent with the code used in other field types. That said, I only thought to add the disabled option to the media field after having a case where I needed to disable the select/clear buttons in a non-edit type view (and outputting the data in other ways just didn't suit my needs). The behavior should be consistent across all fields for sure, and all fields should be allowed to be disabled if need be (just my opinion on the last part).

@benjaminpick
@mbabker
Joomla! member
@elinw

Don't get me started with the check boxes :P.

I didn't really mean that other fields handled disabled differently I meant that in general when we have an attribute like that it works that way (like required)

    // Set the required and validation options.
    $this->required = ($required == 'true' || $required == 'required' || $required == '1');

Theory would say any value means Boolean true, but as pointed out semantically having something="false" and something="0" mean true is just terrible. It's good usability practice to make those mean false and to let disabled="disabled" mean true because it lets users use the format that they are used to or want to use i.e. disabled="disabled" is proper XHTML.

In fact, I would point out, as an example in calendar we have
if ((string) $this->element['disabled'] == 'true')
{
$attributes['disabled'] = 'disabled';
}

Benjamin is right that there are important differences in how disabled and readonly are handled during processing (this this relates again to the specification for handling of check boxes).

The disadvantage of putting it in JFormField is that the attribute is not relevant for all field types according to the W3C standards which is why we don't have it for checkboxes.

I ask because there's a related issue which is people think that readonly as an attribute is protection from change but in fact readonly is NOT and I think it would be good/educational to note this in the docblocks. Again theoretically everyone knows better, practically this is not the case.

http://www.w3.org/TR/html4/interact/forms.html#h-17.12.1

Explains what the difference between readonly and disabled means and should probably be referenced in any docblock.

Also useful
http://www.w3.org/TR/css3-selectors/#enableddisabled
http://www.w3.org/TR/html4/interact/forms.html#h-17.2.1
Good discussion
http://kreotekdev.wordpress.com/2007/11/08/disabled-vs-readonly-form-fields/

@benjaminpick

The disadvantage of putting it in JFormField is that the attribute is not relevant for all field types according to the W3C standards which is why we don't have it for checkboxes.

I don't agree. Even if the attribute is parsed at JFormField, it doesn't have to be used in every field. I'd even go further and say: the fact that "disabled/read-only" doesn't always translate into a html attribute with this name (e.g. media), doesn't hinder us to use disable/read-only in every field. It should only "behave as if" (semantically).

Read-only: The user may not modify the value, but it is submitted.
Disabled: The user may not modify the value, and it musn't be submitted.

And for the media-element type of input I'd prefer to show disabled (greyed-out) buttons, as in "normally you can modify, but you can't here/at the moment".

@elinw

I'm just saying it is a disadvantage not that we shouldn't do it. Just like we need to document that checkboxes require special handling we need to document that they don't have a disabled state and that unchecked is handled differently than e.g a blank text field because documenting will reduce user confusion. If desired, we can even decide to handle user error by having disabled="true" implement the correct behavior (which is to say act like the field does now)l to short circuit what might be a common error.
Also we need to document what required means for the media field in order to reduce confusion.

Overall I think it's a good idea to encourage use of disabled rather than readonly whenever it is appropriate and will work since it's not susceptible to Firebug.
Also btw I have an old old tracker item http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=22954 in the cms tracker about editorarea not being able to be disabled and now I'm thinking we might want to decide what to there.

Benjamin Pick indenting 5e4328f
@realityking
Joomla! member

This doesn't merge cleanly. @benjaminpick could you please fix it? Thanks.

Benjamin Pick added some commits Jul 24, 2012
@benjaminpick
Benjamin Pick added some commits Jul 24, 2012
Benjamin Pick forgot getter f008c7c
Benjamin Pick another mc d9676ed
@subtext

Thanks Elin! I had opened the Issue #1543 discussion because I believed there was an issue with the multiple attribute not being set properly. It turned out, however, that the issue was that the values used in the xml element attribute had to be either the literal string "true" or "multiple". It did not respond to Boolean values as in multiple="1". The approach used here would correct that issue (if you apply it to multiple as well). But it still references specific strings like "readonly", "disabled", or "multiple" (if it applied). I would rather see something like this:

$this->readonly = ( ! ((bool) $readonly ) || $readonly == "false");

The only string referenced is "false". This way setting the attribute to "false" or zero would indeed register the property as false. But, any other non empty value would register as boolean true. I understand the HTML element must be set to readonly="readonly" ( or multiple="multiple" ), but I feel the attribute of the xml file should be any boolean value.

Though not every element will use each of these attributes, I think that if it is applicable to more than one field type, then it should be included in the JFormField so as not to duplicate code across additional subclasses.

Okay, that's my two cents.

@LouisLandry

I like the implementation as it stands today in this pull request. I'd like to get comments from another maintainer before merging it though. @eddieajau, @realityking what do you think? I went ahead and marked it for merging into 12.3 pending a second opinion.

@benjaminpick are you able to rebase this and squash the 9 commits down to one? It seems a bit much to have 9 commits for a total change of 26 lines of code. I understand why it ended up that way, but would be much cleaner IMO if we could get that squashed.

@eddieajau

Looks good to me.

@benjaminpick
@benjaminpick benjaminpick pushed a commit to benjaminpick/joomla-platform that referenced this pull request Oct 10, 2012
Benjamin Pick Close #1188: Consistent use of XML values for fields 'readonly' and '…
…disabled'
2255646
@LouisLandry

Closing in favor of #1592

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment