? Success

User tests: Successful: Unsuccessful:

avatar andrepereiradasilva
andrepereiradasilva
24 Nov 2015

Description

This PR adds addHTMLData public function to JDocumentHTML
This can be used for creanting and adding values to data-* HTML5 attributes in any HTML element (by tag or by id).
This work in frontend and backend.

Examples of usage:

JFactory::getDocument()->addHTMLData('caption', 'img.caption');
JFactory::getDocument()->addHTMLData('caption', 'img.othercaption');
JFactory::getDocument()->addHTMLData('keepalive', array('uri' => '/index.php', 'seconds' => 60000));
JFactory::getDocument()->addHTMLData('somevar', 'somevalue', '#content');

How to test

  1. Add one of the examples above somewhere in /libraries/cms/html/behaviour.php. I tested in caption and keepalive functions.
  2. Go to homepage and check if in the HTML code there is the data-* attribute you added with the value you added in the element you selected.

B/C

None

Note: Improvements are welcome

avatar andrepereiradasilva andrepereiradasilva - open - 24 Nov 2015
avatar andrepereiradasilva andrepereiradasilva - change - 24 Nov 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Nov 2015
Labels Added: ?
4580e32 24 Nov 2015 avatar cs
avatar andrepereiradasilva andrepereiradasilva - change - 24 Nov 2015
The description was changed
avatar dgt41
dgt41 - comment - 24 Nov 2015

Proper camelCase is addHtmlData()
But I'm not so sure that we need this if it's usage is to pass some attributes for JS.
We can do that in the component/module/plugin/library level (I think so)

avatar mbabker
mbabker - comment - 24 Nov 2015

:-1:

IMO this too easily presents the ability to corrupt the rendered HTML and by its very design can cause major issues. Given this only supports altering an HTML element or a given element ID, one can easily call JFactory::getDocument()->addHTMLData('somevar', 'somevalue', 'div'); and would add their data-somevar element to every div on the page.

Any changes like this to any library classes also should be accompanied with unit tests to validate behaviors.

avatar Bakual
Bakual - comment - 24 Nov 2015

I also don't really like that approach. Imho it's also perfectly fine to add inline JavaScript for such cases.

avatar andrepereiradasilva
andrepereiradasilva - comment - 24 Nov 2015

First of all, thanks all of you for the feedback. As you know i'm not and expert in Joomla as you guys are (still learning) so your comments are very important.

Reggarding your comments

@dgt41

Proper camelCase is addHtmlData()

Yes true, that can be corrected.

But I'm not so sure that we need this if it's usage is to pass some attributes for JS.
We can do that in the component/module/plugin/library level (I think so)

That i know of, since there is no class to add/change html in the HTML body we can only do it with replaces in the rendered html or with overrides. As that is not possible in the core Joomla, don't think is possible.

@mbabker

IMO this too easily presents the ability to corrupt the rendered HTML and by its very design can cause major issues. Given this only supports altering an HTML element or a given element ID, one can easily call JFactory::getDocument()->addHTMLData('somevar', 'somevalue', 'div'); and would add their data-somevar element to every div on the page.

Yes you're right! Thanks.
I think this can be corrected by only allowing body tag and id selectors.

Any changes like this to any library classes also should be accompanied with unit tests to validate behaviors.

That i have to see how to do.

I also don't really like that approach. Imho it's also perfectly fine to add inline JavaScript for such cases.

True, you can. It's working good as it is with inline js.

Conclusion

Ok. For what i read, you guys don't see the need for this PR.

I see the need and i think it adds more flexibility to pass data to javascript in joomla core instead of adding inline javascript that can present several problems when defering/async js.

For instance, the caption js (let's forget for now if can be replaced with HTML #7450). If you want to defer it's processing to the page onload event and make page load faster with that, how do you do it?

If you defer the caption.js, JCaption inline script will not work as it will return a javascript error.
As you know, this is will happen because JCaption is called in the head tag (page not loaded yet) and the caption.js, is this case, is only loaded on page onload event.

With values in data-* attributes you have no such problems because the is no inline js. All js in executed in the external js file, that can be executed in head or defered to page onload event.

By removing inline js, most of the scripts in behaviour.php could be defered to page onload event and make pages load faster.

This method can be used in several js files, which will make page load faster and better user experience, and with that better SEO.

But, of course, maybe there are cleaner ways to do this than this PR.

One idea that occurs to me is using data-* attributes directly in the external script tag, for instance:
<script src="/media/system/js/caption.js" data-caption="img.caption" async="true" /></script>

W3C HTML5 Recommendation says data-* are global attributes so data-* can be used in any tag, including the script tag:

Custom data attributes (e.g. data-foldername or data-msgid) can be specified on any HTML element, to store custom data specific to the page.
Source: http://www.w3.org/TR/html5/dom.html#global-attributes

So i will close this PR and investigate if this solution can be applied in a simplier and cleaner way and with B/C.

thansk agian for your feedback

avatar andrepereiradasilva andrepereiradasilva - change - 24 Nov 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-11-24 21:57:18
Closed_By andrepereiradasilva
avatar andrepereiradasilva andrepereiradasilva - close - 24 Nov 2015
avatar andrepereiradasilva andrepereiradasilva - close - 24 Nov 2015
avatar dgt41
dgt41 - comment - 24 Nov 2015

I totally support data attributes, don't get me wrong. It is THE PROPER way to pass vars from PHP to JS. But that can be done already thru Jlayout, or Layouts, so instead of having addScriptDeclaration('whatever'); you echo those params/vars into the actual html part wrapped in a data-attr.
The way that could work is demonstrated in #8278

avatar andrepereiradasilva
andrepereiradasilva - comment - 24 Nov 2015

@dgt41, nice to see you also support data-* :smile:

Yes, i know you can do it in template, but how to do it in Joomla core behaviours?

avatar mbabker
mbabker - comment - 24 Nov 2015

Without building all HTML output in something like DOMDocument, you don't without complex regex's.

avatar andrepereiradasilva
andrepereiradasilva - comment - 24 Nov 2015

@mbabker yes. the alternative is JHtml::_('script',...) allows to add data-* values to the script tag and then the external js script can use it's own data attributes.

avatar dgt41
dgt41 - comment - 24 Nov 2015

Or you can you echo a simple span element

echo '<span data-url="' . $url . '" data-time="' . $refresh_time . '"></span>';

this will be ok for keepalive() in behaviour.php

avatar andrepereiradasilva
andrepereiradasilva - comment - 24 Nov 2015

@dgt41
I didn't even thought to try that :smile:

But two things to consider with that method:

  • you can't really control where the span will apperar on every pages (i think that will appear where behaviour is called for the first time). This, that i see, is not very important.
  • data-* were not designed to be added in empty div/span tags. They should be related to the element they are in or in a element in the context.

But yes, it's and alternative that works as it is.

avatar mbabker
mbabker - comment - 24 Nov 2015

I think this can be corrected by only allowing body tag and id selectors.

Bad idea. If the spec allows data- attributes anywhere, any API Joomla implements shouldn't restrict this. So for an API endpoint to be useful, it would almost need to accept DOM style selectors and the method would have to be smart enough to decipher that and modify the right elements. So to do it "right", it immediately becomes far too complex to maintain at a high level IMO.

the alternative is JHtml::_('script',...) allows to add data-* values to the script tag and then the external js script can use it's own data attributes

Adding support for an attribs array to JHtml::script(), JDocument::addScript(), and the rendering support needed in JDocumentRendererHead::fetchHead() is rather trivial. Actually, it's copying the existing mechanisms for JHtml::stylesheet() and the associated methods. Just typehint the parameter as an array if someone does it. There are places passing a boolean false in for that attribs array for the stylesheet API and since Joomla does absolutely zero validation on this, it's going to become an issue when https://github.com/joomla/joomla-cms/blob/3.5.0-beta/libraries/joomla/document/html/renderer/head.php#L118 is changed to call the namespaced class Framework class which typehints to an array.

avatar andrepereiradasilva andrepereiradasilva - head_ref_deleted - 25 Nov 2015

Add a Comment

Login with GitHub to post a comment