Joomla! Issue Tracker - CMS

Download
Launch
Feeling Lucky

[#19106] - [4.0] Changes to bootstrap-init.js to support joomla:updated event

Conflicting Files ? ? Failure

User tests: Successful: Unsuccessful:

avatar cheesegrits
cheesegrits
20 Dec 2017

Pull Request for Issue # .

Summary of Changes

Refactored the code into a function, and added events handlers for DOM ready and joomla:updated. In order to allow the code to be re-entrant, need to remove the options after BS init has been run, so we don't add multiple event delegations.

Also tidied up a few missing arguments, unused variables, and changed some 'var' to 'let'.

Testing Instructions

Basic testing is to load a page with Bootstrap widgets on it, like anything with tabs on the backend, and observe that everything still works. Testing the joomla:updated event isn't really possible, as it's designed to allow adding new forms to the DOM on the fly. You can prove that the code runs by putting a breakpoint somewhere in the _initBootstrap function, then fire the event in the console with ...

Joomla.Event.dispatch(document, 'joomla:updated')

... and you'll hit your breakpoint. But without adding a new form to the DOM and adding the options for it, which is hard to provide a test case for, that won't actually prove it works. But it does. My extension (Fabrik) uses this extensively, and I can confirm it does work.

Expected result

Actual result

Documentation Changes Required

avatar cheesegrits cheesegrits - open - 20 Dec 2017
avatar cheesegrits cheesegrits - change - 20 Dec 2017
Status New ⇒ Pending
avatar joomla-cms-bot joomla-cms-bot - change - 20 Dec 2017
Category ⇒ JavaScript
avatar Fedik
Fedik - comment - 22 Dec 2017

@cheesegrits thanks for doing it

Here is a couple mistakes which need to fix.
I think it from misunderstanding of 'joomla:updated' concept.

Couple main mistakes:

  • do not call Joomla.Event.dispatch(document, 'joomla:updated') over document (unless you changed whole <body>), because this will lead do double initialization for some scripts
  • do not reset the options, it can break other scripts, they should remain
  • do not need to force Joomla.addOptions();, unless you really have loaded a new options

The event joomla:updated should be dispatched over the changed container, not over a whole document.
Then other scripts should use target instead of document as starting point, that will prevent double initialization, see example 576a4b4#diff-6d7703e5e0a21c853fc1ee5549e9e79dR13

Example:

<body>
...
  <div id="regular-content"></div>
...
  <div id="dynamic-content">Content changed with ajax</div>
...
</body>

On DOMContentLoaded the scripts will be applied to both regular-content and dynamic-content. And when you change the content of dynamic-content block you need to dispatch joomla:updated.
Example of AJAX callback:

function ajaxCallback(response) {
  var container = document.getElementById('dynamic-content');
  Joomla.Event.dispatch(container, 'joomla:removed');

  ... here some code to update the "container"

  Joomla.Event.dispatch(container, 'joomla:updated');
}

Example of script initialization:

function doRandomStuff(event) {
   // Instead of "document" we should use "target" with fallback to "document",
   // "target" is a updated container
   var target = event && event.target ? event.target : document;

  // Do jQuery stuff
  $(target).find('.some-element').css('color', 'green');
  
  // Do vanila stuff
  target.querySelector('.some-element').style.color = 'green';
}

// Initialize at an initial page load (domready)
document.addEventListener("DOMContentLoaded", doRandomStuff);
// Initialize when a part of the page was updated
document.addEventListener("joomla:updated", doRandomStuff);

As you can see we dispatch the event joomla:updated over dynamic-content, then the event bubbling up and catched by eventListener("joomla:updated", doRandomStuff). And event.target is our updated container dynamic-content. So we work only with a new content and do not touch existing, this is prevent a double initialization.

If you still have a questions just ask ?

avatar cheesegrits
cheesegrits - comment - 23 Dec 2017

Thanks for the feedback. And indeed. that's how I thought joomla:updated was supposed to work ... but if you look at the original bootstrap init code, it's not getting the Bootstrap objects to build from the DOM, it's getting them from the options. Well, except for the modals.

So in order to get it to work with the "initialize a container" way of working, we'll have to flip that around, so instead of each()'ing through the options and applying the BS to the option key, with any added BS options coming from the option values ... like it does now ...

let accordion = Joomla.getOptions('bootstrap.accordion', null),

...

		if (accordion) {
			$.each(accordion, function(index, value) {
				$('#' + index).collapse(
					{
						parent:value.parent,
						toggle:value.toggle
					}
				).on("show", new Function(value.onShow)())
					.on("shown", new Function(value.onShown)())
					.on("hideme", new Function(value.onHide)())
					.on("hidden", new Function(value.onHidden)());
			});

			Joomla.loadOptions({'bootstrap.accordion': null})
		}

... we'd have to find each of the Boostrap constructions in the DOM target, then find the specific options for that id (or class, depending on the "thing", like popovers use class, not id) ...

let accordion = Joomla.getOptions('bootstrap.accordion', null),

...

$(target).find('.accordion').each(function() {
   let $self = $(this);
   if (accordion.has($self.id)) {
      let value = accordion[$self.id];
      $('#' + index).collapse(
         {
            parent:value.parent,
            toggle:value.toggle
         }
      ).on("show", new Function(value.onShow)())
      .on("shown", new Function(value.onShown)())
      .on("hideme", new Function(value.onHide)())
      .on("hidden", new Function(value.onHidden)());
   }
}

So would that be the right approach?

I did consider it, but it seems a little cumbersome ... as we already have all the objects we need to BS-ify in the options, to then add DOM-aware code that needs to know the selectors for every structure it is working with ... and as this script is (afaict) the only one that references those bootstrap.foo options, and ONLY for the purposes of init ... and there is precedence for doing it this way, of disposing of the options after initializing, it's how jtext does it ...

https://github.com/joomla/joomla-cms/blob/staging/media/system/js/core-uncompressed.js#L121

... I thought I'd give it a go this way. But I'm cool with whichever way you want it done.

As for loading the new options - yes, my comment was assuming the app has added new BS options in the AJAX loaded content. So I just wanted to get that documented somewhere. So is ...

\JFactory::getDocument()->getScriptOptions()

... the officially blessed way of doing that?

BTW ... in dicking around trying to get this to work with AJAX added content, I found that using the embedded script tag with type="application/json" for new options was actually kind of difficult, because I'm having to parse scripts in the returned AJAX (I'm using requirejs to load dependencies). Which means I can't embed the JSON in a script tag, as that then gets parsed as a script, and blows up.

What I wound up having to do was the rather nasty way of adding a script to load the options and fire joomla:updated as I'm building the AJAX content on the server side ...

	    $scriptOptions = \JFactory::getDocument()->getScriptOptions();

	    if (!empty($scriptOptions))
	    {
		    $jsonOptions = json_encode($scriptOptions);
		    $jsonOptions = $jsonOptions ? $jsonOptions : '{}';
		    HTML::addScriptDeclaration("Joomla.loadOptions(JSON.parse('" . $jsonOptions . "')); Joomla.Event.dispatch(document, 'joomla:updated')");
	    }

... although once we get this init code "containerized" I'll have to change that dispatch().

Am I missing something obvious with the JSON script tag method?

avatar Fedik
Fedik - comment - 23 Dec 2017

hm, I see, it relay to id everywhere ?
well, but still, your example:

if (accordion) {
	$.each(accordion, function(index, value) {
		$('#' + index)
			.collapse({parent:value.parent, toggle:value.toggle})
			.on("show", new Function(value.onShow)())
			.on("shown", new Function(value.onShown)())
			.on("hideme", new Function(value.onHide)())
			.on("hidden", new Function(value.onHidden)());
	});
}

will become:

var $target = $(target);

if (accordion) {
	$.each(accordion, function(index, value) {
		$target.find('#' + index)
			.collapse({parent:value.parent,toggle:value.toggle})
			.on("show", new Function(value.onShow)())
			.on("shown", new Function(value.onShown)())
			.on("hideme", new Function(value.onHide)())
			.on("hidden", new Function(value.onHidden)());
	});
}

Without resetting of Joomla.loadOptions({'bootstrap.accordion': null}).

Maybe use a class selector would be more smart, but we have what we have ?

and there is precedence for doing it this way, of disposing of the options after initializing, it's how jtext does it

I think jtext a bit more special, because jtext store all strings in own storage. so it just to avoid confusion (where the strings is located) and to save a little bit of memory ?
In case of other scripts I would not do it, unless a script require it itself. it allow to other scripts to check which bootstrap options are active/available, and if you reset them then it become more complicated.

BTW ... in dicking around trying to get this to work with AJAX added content, I found that using the embedded script tag with type="application/json" for new options was actually kind of difficult, because I'm having to parse scripts in the returned AJAX

The main point of use <script type="application/json"> for options is to avoid an inline scripts, and still be able to set JavaScript options from PHP.

I agree that can be a bit too complicated, when it come to AJAX.
In theory you have at least 3 way:

  • use &tmpl=component request, then just include result to document, without parsing, and just call Joomla.loadOptions(), a new options will be parsed and merged automatically
  • you can include <script type="application/json" class="joomla-script-options new">{"myStuff":[1,2,3]}</script> to your response HTML (it not required to be in <head>), and then, as in previous case, in ajax callback just call Joomla.loadOptions(), all new options will be parsed and merged automatically.
    Also you not limited to use only one <script type="application/json">, they can be multiple.
  • if it JSON request, then you have no choice, you need to send the options as part of JSON and then merge with existing options, eg Joomla.loadOptions(jsonResponse.options)

your example:

$scriptOptions = \JFactory::getDocument()->getScriptOptions();
if (!empty($scriptOptions))
{
  $jsonOptions = json_encode($scriptOptions);
  $jsonOptions = $jsonOptions ? $jsonOptions : '{}';
  HTML::addScriptDeclaration("Joomla.loadOptions(JSON.parse('" . $jsonOptions . "')); Joomla.Event.dispatch(document, 'joomla:updated')");
}

I would do (if you do not use &tmpl=component):

$response = '<div>blabla html</div>';
$scriptOptions = \JFactory::getDocument()->getScriptOptions();
if (!empty($scriptOptions))
{
  $jsonOptions = json_encode($scriptOptions);
  if ($jsonOptions) {
    $response .= '<script type="application/json" class="joomla-script-options new">' . $jsonOptions . '</script>'
  }
}

Then client side:

function ajaxCallback(responseHtml) {
  let container = ... get your container element
  Joomla.Event.dispatch(container, 'joomla:removed');
  container.innerHtml(responseHtml); // set new HTML
  Joomla.loadOptions(); // parse new options
  Joomla.Event.dispatch(container, 'joomla:updated');
}
avatar cheesegrits
cheesegrits - comment - 23 Dec 2017

Popover and tooltip use a class rather than individual ids. Well, more correctly I guess they use the selector as-is, without prepending the '#', and it happens to be a class (like .hasPopover).

https://github.com/joomla/joomla-cms/blob/4.0-dev/media/system/js/bootstrap-init.js#L104

OK, I shall redo the PR this way.

Thanks for your time and patience.

-- hugh

avatar brianteeman
brianteeman - comment - 24 Dec 2017

Thanks for your time and patience.

and thank you for your contribution

avatar cheesegrits cheesegrits - change - 24 Dec 2017
Labels Added: ?
avatar cheesegrits
cheesegrits - comment - 24 Dec 2017

OK, I think that's now doing it the right way.

Here's a screencast of it working ...

https://www.screencast.com/t/VFNQdQDDRH

Sorry about the Flash.

BTW, would it make sense to add that code that builds the JSON script tag as a helper in J! somewhere? I just copped that code from the StyleRenderer class and stuck it in my own helper, but if we're going to be expecting extensions to add that to AJAX output, it'd make sense to have a function that builds it.

avatar Fedik
Fedik - comment - 24 Dec 2017

looks good to me,
@cheesegrits thanks!

would it make sense to add that code that builds the JSON script tag as a helper in J! somewhere?

maybe not a bad idea, but to be honest I have no idea where ?

avatar dgrammatiko
dgrammatiko - comment - 24 Dec 2017

Ok couple notes from me:

  • J4 will not use the bootstrap JS in core. Neither the 3rd PD should. The main reason is that Bootstrap components (modals, accordions etc) are not a11y. Which means it breaks our promise that J4 would be fully accessible.
  • Personally I would propose to remove the JS part of the bootstrap or, worse case scenario, move it to a plugin! This thing shouldn't be part of Joomla as it won't be used by (core) Joomla!
  • The transition for 3rd PD from the bootstrap js components to Joomla's custom elements will require the least effort possible. Check the PR: #17375 and compare it to the J3 tabs! Literally all it is required is just renaming the class!

The code here is ok, btw!

avatar cheesegrits
cheesegrits - comment - 25 Dec 2017

@dgt41 - what's the status on the core UI work? Last activity on that PR was in August.

I'm actively working on getting a very large component (Fabrik), which has a massive amount of back and front end UI, moved to 4.0. I'm more than happy to help out testing the new core UI features, but it'd need to be in the 4.0-dev branch.

avatar Quy Quy - change - 20 May 2018
Title
…
Changes to bootstrap-init.js to support joomla:updated event
[4.0] Changes to bootstrap-init.js to support joomla:updated event
avatar joomla-cms-bot joomla-cms-bot - edited - 20 May 2018
avatar joomla-cms-bot joomla-cms-bot - change - 20 May 2018
Title
…
Changes to bootstrap-init.js to support joomla:updated event
[4.0] Changes to bootstrap-init.js to support joomla:updated event
avatar euismod2336
euismod2336 - comment - 19 Oct 2019

@cheesegrits Can you update this PR or comment on the status, perhaps it can be closed or updated according the discussion in here?

avatar brianteeman
brianteeman - comment - 9 Jan 2020

It is two years since the last significant comment. Is anything happening with this?

avatar Quy
Quy - comment - 7 Feb 2020

@Fedik Is this still valid or should be closed? It appears @cheesegrits is no longer active.

avatar Fedik
Fedik - comment - 7 Feb 2020

many things changed since these 2 years ?
hard to say,
the file seems still in use

joomla-cms/libraries/cms/html/bootstrap.php

Lines 165 to 170 in 77a6643

}
$wa
->registerScript('bootstrap.init.legacy', 'legacy/bootstrap-init.min.js', ['dependencies' => ['core', 'bootstrap.js.bundle']])
->useScript('bootstrap.init.legacy');

I guess it still valid pull

avatar roland-d
roland-d - comment - 1 Aug 2020

Due to inactivity I will close this PR, but it can always be reopened in case someone wants to continue working on this.

avatar roland-d roland-d - close - 1 Aug 2020
avatar roland-d roland-d - change - 1 Aug 2020
Status Pending ⇒ Closed
Closed_Date 0000-00-00 00:00:00 ⇒ 2020-08-01 15:26:44
Closed_By ⇒ roland-d
Labels Added: Conflicting Files ?

Add a Comment

Login with GitHub to post a comment