? Failure

User tests: Successful: Unsuccessful:

avatar nilesh-more
nilesh-more
20 May 2017

Pull Request for Issue #16034

Summary of Changes

  • HTML5 is designed with extensibility in mind for data that should be associated with a particular element but need not have any defined meaning. data-* attributes allow us to store extra information on the standard, semantic HTML elements without other hacks such as non-standard attributes, extra properties on DOM.

  • However, custom attributes have no special meaning generally and are only special to the owner's application. They can be used to simplify an application's logic. By allowing JForm to support data attributes, it can be greatly aid development of extensions both core and third party.

Testing Instructions

  • Add data attribute in XML form, for example

    • Open administrator/components/com_users/models/forms/user.xml file
    • Add data attribute in xml field, like
       <field name="name" 
              type="text"
              description="COM_USERS_USER_FIELD_NAME_DESC"
              label="COM_USERS_USER_FIELD_NAME_LABEL"
      	required="true"
              size="30"
              data-action-type="click"  // add data attribute like this
         />
        
  • Login into administrator

  • Click on menu Users => Manage => Add New User

  • Inspect the filed = Name and check input type html

Expected result

  • Output should be like this
     <input type="text" 
            name="jform[name]" 
            id="jform_name" 
            value="" 
            class="required invalid" 
            size="30" 
            required="required" 
            aria-required="true" 
            data-action-type="click"  // output 
            autocomplete="off" 
            aria-invalid="true"
       />
         
  • Output snapshot
    screenshot from 2017-05-15 19 27 38

Actual result

  • Added data attribute is missing in final output
     <input type="text"
    name="jform[name]"
    id="jform_name"
    value=""
    class="required invalid"
    size="30"
    required="required"
    aria-required="true"
    autocomplete="off"
    aria-invalid="true"
    />

Documentation Changes Required

avatar nilesh-more nilesh-more - open - 20 May 2017
avatar nilesh-more nilesh-more - change - 20 May 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 20 May 2017
Category Layout Libraries
avatar nilesh-more nilesh-more - change - 20 May 2017
The description was changed
avatar nilesh-more nilesh-more - edited - 20 May 2017
avatar dgrammatiko
dgrammatiko - comment - 20 May 2017

@nilesh-more the output needs to be escaped, e.g. data-action-type="click" the value needs htmlspecialchars($the_variable_name, ENT_COMPAT, 'UTF-8')

avatar nilesh-more nilesh-more - change - 20 May 2017
Labels Added: ?
avatar nilesh-more
nilesh-more - comment - 20 May 2017

@dgt41 fixed. Please check once

avatar parthlawate
parthlawate - comment - 25 Jul 2017

any update on this @nilesh-more any additional changes needed on this to merge ? @dgt41 any more fixes needed ?

avatar joomla-cms-bot joomla-cms-bot - change - 7 Aug 2017
Category Layout Libraries Unit Tests Repository Administration com_admin SQL Postgresql MS SQL com_categories com_config com_content com_fields com_finder com_joomlaupdate com_languages com_menus
avatar joomla-cms-bot joomla-cms-bot - change - 7 Aug 2017
Category Unit Tests Repository Administration com_admin SQL Postgresql MS SQL com_categories com_config com_content com_fields com_finder com_joomlaupdate com_languages com_menus Layout Libraries
avatar nilesh-more nilesh-more - change - 7 Aug 2017
Labels Added: ?
avatar nilesh-more nilesh-more - change - 23 Aug 2017
Labels Removed: ?
avatar Fedik
Fedik - comment - 24 Aug 2017

Addittionaly.
Why to use JText for each data- value,
data- value can be anything: int (123), boolean (true/false), json string,

I do not think that it is a good idea to use JText here.

avatar nilesh-more
nilesh-more - comment - 22 Sep 2017

@Quy and @Fedik @parthlawate any additional changes needed on this to merge?

avatar Fedik
Fedik - comment - 23 Sep 2017

there still can be improved a lot. $dataAttributeName, $dataAttributeValues and getDataAttributes() looks overcoded to me.

it can be more easy:

/**
  * The data attributes of the form field.
  *  For example, data-action-type="click" data-action-type="change"
  *
  * @var  string[]
  */
protected $dataAttributes = array();

which store attributes as key => value
$dataAttributes['data-country'] = 'data-country="India"'

Then in setup() you just iterate thought all attributes and check for data-

foreach ($element->attributes() as $k => $v) {
  if (strpos($attrName, "data-") === 0) {
     $this->dataAttributes[$k] = $k . '="' . htmlspecialchars($v, ENT_COMPAT, 'UTF-8') . '"'
  }
}

then in __get, inside default::

if (strpos($name, "data-") === 0 && array_key_exists($name, $this->dataAttributes)) {
  return $this->dataAttributes[$name]
}

in __set inside default:

if (strpos($name, "data-") === 0) {
 $this->dataAttributes[$k] = $name . '="' . htmlspecialchars($v, ENT_COMPAT, 'UTF-8') . '"'
}

and finally, if you still need this getDataAttributes():

public function getDataAttributes() {
  return $this->dataAttributes;
}

I think it best that we can do for handling data- attributes in JField, if we want to have it.

Additionally, all @since should be: @since __DEPLOY_VERSION__

avatar Fedik
Fedik - comment - 5 Oct 2017

@nilesh-more looks good

Please replace @since 3.8.1 to @since __DEPLOY_VERSION__, so it will be set automatically

avatar nilesh-more
nilesh-more - comment - 6 Oct 2017

@parthlawate @Fedik Done with requested changes

avatar Fedik
Fedik - comment - 6 Oct 2017

@nilesh-more cool thanks!

I think it is ready for testing,
just no idea what wrong with Travis ?

avatar brianteeman
brianteeman - comment - 6 Oct 2017

The travis issue is

1) JFormFieldCheckboxesTest::testGetOptions
The field with two values did not produce the right options
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         'onchange' => ''
+        'dataAttributes' => ''
@@ @@
         'onchange' => ''
+        'dataAttributes' => ''
     )
 )
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/form/fields/JFormFieldCheckboxesTest.php:685
avatar nilesh-more
nilesh-more - comment - 6 Oct 2017

@Fedik Yes... I'm looking into it.
@puneet0191 Need your help to figure out what wrong with Travis

avatar astridx
astridx - comment - 8 Mar 2018

I have tested this item successfully on 64e9d40

It worked for me on a text field.
(It did not work on a field of type radio and componentlayout.)


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

avatar astridx astridx - test_item - 8 Mar 2018 - Tested successfully
avatar bettyorganero
bettyorganero - comment - 5 Apr 2018

@Quy I did the test from this 0cc74d8 It worked for me in joomla 3.8.1 and 3.8.6.

The only issue is the multi language support. it don't show the text of the constant.

Thanks

avatar nilesh-more
nilesh-more - comment - 5 Apr 2018

@bettyorganero Based on a @Fedik suggestion I have removed multi-language support.

cc: @Fedik

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 5 Apr 2018

@bettyorganero @astridx can you please retest?

avatar bettyorganero
bettyorganero - comment - 5 Apr 2018

@nilesh-more ok thanks for your answer. It is unfortunate because some data-attributes are used to show content that in a multi-language site is necessary the translation. @Fedik

@franz-wohlkoenig I have tested yesterday night and works good the only problem is the multi-language support

Best

avatar Fedik
Fedik - comment - 5 Apr 2018

It is unfortunate because some data-attributes are used to show content that in a multi-language site is necessary the translation

Use JText::script() and Joomla.JText._() for frontend translation in your JavaScript. https://stackoverflow.com/a/30506820/1000711

avatar infograf768
infograf768 - comment - 5 Apr 2018

@Fedik @nilesh-more
Can't we add a new parameter to decide or not of the use of JText ?
It could be done the way we do for

case 'translateLabel':
			case 'translateDescription':
			case 'translateHint':

we would have translate-data-action-type

avatar Fedik
Fedik - comment - 5 Apr 2018

we can add anything,
but, personally, I do not see much sense in it

data attribute is mix of everything, it can be '0', 'true', '(function(){ some random code})()',
I do not think it is good to run everything through JText::_(), even with a new parameter.

avatar coolbung
coolbung - comment - 10 Apr 2018

With this implementation, fields can now have data-* attributes defined in the XML. However how to make use of these attributes is left to the developer. So supporting JText isn't needed. Wherever needed the developer can do it at the display/implementation time.

avatar manojLondhe
manojLondhe - comment - 10 Apr 2018

I do not think it is good to run everything through JText::_(), even with a new parameter. @Fedik

I feel the same, not all the times we will need JText for data attributes.

avatar bettyorganero
bettyorganero - comment - 20 Apr 2018

Hello guys.

Use JText::script() and Joomla.JText._() for frontend translation in your JavaScript. https://stackoverflow.com/a/30506820/1000711

@Fedik thanks but this not work with the data attributes into my xml form file.

I not an expert programmer. So, I have my data attribute into my field:
data-tooltip="COM_PRODCATALOG_ENTERPRISE_WEBSITE_FORMAT"

I don't understand why the label constant works but the constant into my data-tooltip not works. Is for this reason I asked you for the possibility to support the multi-language option. Maybe is for this that not works

Thanks

avatar Fedik
Fedik - comment - 20 Apr 2018

<label> and data- attributes is 2 different things.
<label> contain caption for the field, which obviously should be translated for multilanguage site,
data- can contain anything, that developer add, this make no sense to translate by default, in same way as it with label.

see my previous comment #16141 (comment)

avatar bettyorganero
bettyorganero - comment - 20 Apr 2018

@Fedik I know that <label>l and data- are different things and I understood your point but is not always true or false. Some case like mine, I have a tooltip: data-tooltip="SOME_TEXT_HERE" where the text working with data attributes and this text need to be translate.

Anyway, I will find a solution for this and I will you update, in any case thanks for your contribution is not bad work with data attributes now in joomla forms.

avatar Fedik
Fedik - comment - 20 Apr 2018

what script do you use for data-tooltip="SOME_TEXT_HERE" ?

avatar bettyorganero
bettyorganero - comment - 20 Apr 2018

@Fedik I'm working with materialize framework this is an example: <a class="btn tooltipped" data-position="bottom" data-delay="50" data-tooltip="I am a tooltip">Hover me!</a>
and I'm using this in my fields form.

avatar Fedik
Fedik - comment - 22 Apr 2018

@bettyorganero then it is easy, something like this:

$('.tooltipped').each(function(){
  var $el = $(this);
  $el.data('tooltip', Joomla.JText._($el.data('tooltip'))).tooltip();
});

And you have to call JText::script("BLABLA_TOOLTIP") to add translation strings on to client side.

but it is not related to current pull request

avatar coolbung
coolbung - comment - 4 Jun 2018

The PR is only about adding support for data attributes which otherwise get ignored when generating the markup from a XML field definition. How to use the values is left upto the developer. In the future we can have more PRs that use specific attributes, but I vote to keep this PR simple for now.

Anything else that is stopping this from being merged ?

avatar nilesh-more
nilesh-more - comment - 19 Jul 2018

@Fedik @brianteeman Anything else that is blocking this from being merged as It's open since one year

avatar brianteeman
brianteeman - comment - 19 Jul 2018

I don't see any successful tests marked by real people yet.

avatar manojLondhe manojLondhe - test_item - 20 Jul 2018 - Tested successfully
avatar manojLondhe
manojLondhe - comment - 20 Jul 2018

I have tested this item successfully on 64e9d40


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

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 20 Jul 2018

@nilesh-more so we need a second successfully Test ;-)

avatar uthorat uthorat - test_item - 20 Jul 2018 - Tested successfully
avatar uthorat
uthorat - comment - 20 Jul 2018

I have tested this item successfully on 64e9d40


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

avatar franz-wohlkoenig franz-wohlkoenig - change - 20 Jul 2018
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 20 Jul 2018

Ready to Commit after two successful tests.

Thanks for tests, Guys.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 20 Jul 2018

@nilesh-more now Maintainer decide if merge.

avatar nilesh-more nilesh-more - change - 10 Sep 2018
Labels Added: ?
avatar twister65
twister65 - comment - 19 Sep 2018

The build failed.
Tabs must be used to indent lines; spaces are not allowed :

avatar nilesh-more
nilesh-more - comment - 20 Sep 2018

The build failed.
Tabs must be used to indent lines; spaces are not allowed :
joomla-cms/layouts/joomla/form/field/color/advanced.php

Line 95 in f46c595

 $dataAttribute;

@twister65 MR updated.

avatar zero-24
zero-24 - comment - 26 Feb 2019

Should still be unescaped here, layouts should handle escaping the values.

Yes just fixed that with another commit.

avatar zero-24
zero-24 - comment - 26 Feb 2019

Sorry din't saw that it where that much files to fix. So here we go.

avatar zero-24 zero-24 - change - 26 Feb 2019
Status Ready to Commit Pending
Labels
avatar zero-24
zero-24 - comment - 26 Feb 2019

New tests required.


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

avatar zero-24
zero-24 - comment - 26 Feb 2019

New tests required.


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

avatar nilesh-more nilesh-more - change - 1 Mar 2019
Labels Removed: ?
avatar franz-wohlkoenig franz-wohlkoenig - change - 19 Apr 2019
Title
Issue #16034 feat: Allow jForm to support data attribute
Allow jForm to support data attribute
avatar franz-wohlkoenig franz-wohlkoenig - edited - 19 Apr 2019
avatar nilesh-more nilesh-more - change - 3 Oct 2019
Labels Removed: J3 Issue
avatar dgrammatiko
dgrammatiko - comment - 1 Dec 2019

@wilsonge can you press the button to update this repo?

avatar dgrammatiko dgrammatiko - test_item - 2 Dec 2019 - Tested successfully
avatar dgrammatiko
dgrammatiko - comment - 2 Dec 2019

I have tested this item successfully on 64e9d40


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

avatar alikon alikon - test_item - 4 Dec 2019 - Tested successfully
avatar alikon
alikon - comment - 4 Dec 2019

I have tested this item successfully on 64e9d40


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

avatar alikon alikon - change - 4 Dec 2019
Status Pending Ready to Commit
avatar alikon
alikon - comment - 4 Dec 2019

RTC


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

avatar HLeithner
HLeithner - comment - 4 Dec 2019

Since this is a new feature it will not go into J3 but I would really like to have this in J4.

avatar dgrammatiko
dgrammatiko - comment - 4 Dec 2019

@HLeithner is it too much to ask an exception to the rule here from the PLT.
Let me explain this is a fundamental feature of the HTML5 and J4 is months away from release so in sort I'm kindly asking the PLT to bend the rule here

Thanks

avatar HLeithner
HLeithner - comment - 5 Dec 2019

I'm sorry @dgrammatiko there will be no new features in J3 except they are security related or critical for operation.

PLT doesn't changed this for this feature.

The focus is clearly on getting Joomla 4.0 released.

So it would be great if @nilesh-more can update this PR against J4 or if he is not longer interested someone else can do this.

avatar Fedik
Fedik - comment - 5 Dec 2019

@dgrammatiko I do not see much sense to push it to j3, now.
It is not much critical feature. Who was really need it, already has own workaround.

It is better to concentrate the time and resources on j4.

avatar Quy Quy - close - 5 Dec 2019
avatar Quy Quy - change - 5 Dec 2019
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2019-12-05 15:13:28
Closed_By Quy
avatar Quy
Quy - comment - 5 Dec 2019

Please test PR #27212 rebased for J4.


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

Add a Comment

Login with GitHub to post a comment