Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "[3] Pagebreak navigation cosmetic. Icons, aria-label, title, tooltips and things" #27400

Closed
wants to merge 1 commit into from

Conversation

HLeithner
Copy link
Member

This PR easily breaks existing installations.

@brianteeman
Copy link
Contributor

Could you please explain in more detail what breaks

@HLeithner
Copy link
Member Author

Every template loading it's own jquery could break because if you don't have a override to not load the tooltip has an additional jquery loaded. I had this problem already with another bugfix release in plg_content_pagenavigation

@brianteeman
Copy link
Contributor

Sorry but I dont see how the changes here could do that

@mbabker
Copy link
Contributor

mbabker commented Jan 5, 2020

The only side effect introduced by that PR is the potential load of jQuery and Bootstrap on the frontend in a new location. If you’re saying that is a problem then the frontend needs to be rewritten so that it ONLY generates semantic markup with zero JavaScript enhancement or make Joomla a headless CMS and tell people they’re SOL if looking for core to be usable out-of-the-box.

@HLeithner
Copy link
Member Author

The concept is that I don't have to override each template of joomla and adding a javascript framework in the bugfix release is unexpected

@brianteeman
Copy link
Contributor

but it doesnt break anything

@HLeithner
Copy link
Member Author

it does if you don't use joomla jquery

@Bakual
Copy link
Contributor

Bakual commented Jan 5, 2020

Afaik JQuery as well as tooltips are loaded using a JTHML function and thus should be overrideable. So if you did the JQuery and/or tooltip replacement the proper way, then this doesn't break anything at all.
If your template however manipulated the head of the document and this way removed the core JQuery(which it shouldn't), then it may have unforeseen issues.

In the end I agree with Brian and Michael that this shouldn't be reverted. It's not really a new feature which would mandate to go into a minor release. Also, the rendered markup isn't part of our backward compatibility promise as per https://developer.joomla.org/development-strategy.html. We excluded that at the time especially in mind of issues like this one.

@brianteeman
Copy link
Contributor

Can you please provide a more detailed real word example of how this breaks a site. I have tested this on several sites that do not use joomla jquery without issue

@HLeithner
Copy link
Member Author

It's not impossible that I'm doing it wrong so what would be the prefered method not getting additional js loaded (jquery and tooltip) without overriding each layout? Or don't wanting to get jquery loaded at all?

@brianteeman
Copy link
Contributor

brianteeman commented Jan 5, 2020

I am sure there are a million ways to do it but this is what I do

On my main site I do

function RemoveJavascript()
{
	$removeJs = array(
		'media/jui/js/jquery.min.js',
		'media/jui/js/jquery.js',
		'media/jui/js/jquery-noconflict.js',
		'media/jui/js/jquery-migrate.min.js',
		'media/jui/js/jquery-migrate.js',
		'media/system/js/tabs-state.js',
		'media/jui/js/bootstrap.min.js',
		'media/jui/js/bootstrap.js',
		'media/system/js/mootools-core.js',
		'media/system/js/core.js',
		'media/system/js/caption.js',
	);

	$document = JFactory::getDocument();

	$scripts = $document->_scripts;

	foreach ($removeJs as $signature)
	{
		foreach($scripts as $script => $scriptdata)
		{
			if(stristr($script, $signature))
			{
				unset($scripts[$script]);
			}
		}
	}

	$document->_scripts = $scripts;
}


and then if I want to use bs from a cdn or something completely different I do something like


	<script src="//code.jquery.com/jquery-1.10.2.min.js"></script>
	<script src="//code.jquery.com/jquery-migrate-1.2.1.min.js"></script>
	<script src="//netdna.bootstrapcdn.com/bootstrap/3.0.3/js/bootstrap.min.js"></script>

Or if I want to use bootstrap locally just a newer version then I override the files in media/jui/js/ by creating a template override folder as follows containing files with the same name
templates/<templatename>/js/jui/

@HLeithner
Copy link
Member Author

I'm doing something similar in a plugin onBeforeCompileHead but got a problem on a change in the plg_content_pagenavigation

@brianteeman
Copy link
Contributor

i would say that the problem is in your code then. I tested this on three different sites all of which are not using joomla jquery in different ways and had no problems at all

@Bakual
Copy link
Contributor

Bakual commented Jan 5, 2020

If you want to get rid of tooltips or use a different implementation, you can write a plugin which overrides the JHtml::tooltip function. I don't know exactly how it has to be done, but I'm sure I've read explanations from Michael once.
The same should be possible for JQuery as it is loaded with a JHtml function as well (in core, 3rd party is a different topic).

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Jan 5, 2020

@brianteeman @HLeithner couple of things here as I see that there are some comments that are totally wrong here:

  • Multiple CDNs will make your site slower!. The reason is that the time for resolving the DNS, do the TLS handshake and establishing a connection is not negligible, it's around 300-500ms per domain. Also ALL NEW BROWSERS RUN SITES IN TOTAL ISOLATION meaning that the cached jQuery from Joomla.org is not available to myawesomesite.com...

  • The way to override static assets is by putting the override in the template css/js/images folder.

  • Removing an asset requires you to create an HTMLHelper override plugin that will not add the asset in question. Of course this is quite a ride for any frontender and thus Joomla is pretty much a no-go for anyone that doesn't want to write some 1000's lines of code to achieve what is a no brainer in other systems.

  • [EDIT] Last but not least LAYOUTS are not part of the B/C promise according to the docs

@HLeithner
Copy link
Member Author

i would say that the problem is in your code then. I tested this on three different sites all of which are not using joomla jquery in different ways and had no problems at all

I didn't tested this PR if it breaks, I only know that the last PR adding a tooltip has broken many of my sites.

@dgrammatiko I don't use cdn ;-)

Last but not least LAYOUTS are not part of the B/C promise according to the docs

It's not necessary needed to do it ;-)

I will test this later, but a good solution to this would be interesting (overriding jhtml correctly). And simply overriding jquery doesn't work for me because I use jquery 1.12 and 2.x (I know not necessary any longer but I still have customers requiring legacy browsers...)

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Jan 5, 2020

@dgrammatiko I don't use cdn ;-)

Well, you might not but Brian shared some snippet above which is totally outdated. Since Spectre and Meltdown ALL browsers run each site in isolation, meaning the cache is per site, meaning that no asset will ever be shared across different domains, even if those assets point in the same CDN (the cache bucket is per site).
I just wanted to point this out as people might have wrong assumptions on how browsers really work these days and blindly copy paste those lines...

PS This could be educational: https://youtu.be/yOcgGSCrn-c?t=789

@ReLater
Copy link
Contributor

ReLater commented Jan 6, 2020

Simply remove the line JHtml::_('bootstrap.tooltip'); if you think that's a B\C break. All other changes are not.

@ReLater
Copy link
Contributor

ReLater commented Jan 6, 2020

BTW. I hate these ugly BS tooltips ;-)

@chmst
Copy link
Contributor

chmst commented Jan 6, 2020

If it is about the tooltip - remove it. There are situations when they are useful or necessary but not here

@HLeithner
Copy link
Member Author

Merged #27436 as compromise.

@HLeithner HLeithner closed this Jan 8, 2020
@Bakual Bakual deleted the revert-27398-patch-1 branch January 9, 2020 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants