Closing temporarily while I update

Re-opened - updated the title and description

Updated branch to trigger drone

I really really really like this changes, they make absolutly sense for me, my (very small) pain at the moment is the missing genericity (copy&paste of the code, if we want to change something, we have to change it on all places), perhaps put in in a layout?

It is in layouts ;)

I get your point though and I had tried to put it in renderfield.php but it didnt work - i will try again

Found the problem. It works when i put the code in renderfield.php but not for the search box.

Any ideas?

Do you have working code, where I can look into it?

in renderfield.php replace the code with this

<div class="control-group<?php echo $class; ?>"<?php echo $rel; ?>>
	<?php if (empty($options['hiddenLabel'])) : ?>
		<div class="control-label"><?php echo $label; ?></div>
	<?php endif; ?>
	<div class="controls">
		<?php echo $input; ?>
		<?php if (!empty($description)) : ?>
			<div role="tooltip" id="<?php echo $name . '-desc'; ?>">
				<?php echo $description; ?>
			</div>
		<?php endif; ?>			
	</div>
</div>

and then remove the code from this pr in the text.php layout

You will see the tip is displayed on a normal input field but its not shown in the search box

@bembelimen I should point out that there might be an issue with that code when used on fields like calendar which have an appended button. The "tip" might display in the wrong place with the generic code

@zero-24 advice on rip please

rips is happy now.

@zero-24. Thanks

@bembelimen did you have a look at preventing the copy/paste? If it cant be done then fine and we can proceed with this method and I can extend it to other places that we have non-accessible popover

@bembelimen your comment above has killed anyone from testing this. Can you please respond

Do these descriptions contain HTML markup? If not, could you please escape them?

@SniperSister why?

Escaping prevents XSS, especially important in situations where a description is provided by users (com_fields)

1. So why isnt that done in j3?
2. Only someone with admin access can create a field description
3. If they can create an xss with a description then they can do the same with a field label
4. Surely the correct thing to do is to always filter the input to prevent it being created in the first place
5. Point 4 above is already taking place

1. So why isnt that done in j3?

It is: https://github.com/joomla/joomla-cms/blob/3.10-dev/layouts/joomla/form/renderlabel.php#L37

2. Only someone with admin access can create a field description

True, but a non-superadmin could use such an attack to trigger a XSS in super admin session and escalate it's privileges. Minor risk, I know, but still true.

3. **If** they can create an xss with a description then they can do the same with a field label

Fair point, I'll do a PR for this

4. Surely the correct thing to do is to always filter the input to prevent it being created in the first place

Proper input filtering should be applied too, however as these core fields are used by 3rd party developers too and they often forget about those filters, output escaping helps preventing XSS in such scenarios.

I've escaped the $description but still waiting for @bembelimen to respond

@brianteeman thanks :)

@SniperSister the pr is still held up by @bembelimen who has been promising to look at this re his comments above for six weeks

Hey @brianteeman sorry for the delay, couldn't manage it earlier. I created a PR here. Perhaps this works for you.

Testing it now. The delay is unavoidable but it would have been polite to at least update with a message.

Yes, sorry for that.

Btw. (only half-way related to this PR): the ID will be created via the name of the field not the ID (also the aria-describedby), the ID is something like: jform[foobar]-desc instead of jform_foobar-desc, perhaps it's worth looking into it in another PR?

I am reviewing your changes now

I have merged your changes - they appear to work - however this now applies to all fields so I have to look at fixing some of the field types such as switcher where the description is not displayed. A job for tomorrow

@bembelimen looking further I am not convinced your pr is the way to go. As I suspected #24441 (comment) it doesnt work on append fields and it now applies to all fields not just the existing field layouts that had an aria-described attribute

I will have to see if those fields can be modified

The easiest way would be a small JS onFocus/blur (for input fields) setting/removing a class at .controls, because otherwise there is always a case where the HTML structure will kill the tooltip. But I guess that's not the solution.

Probably just put the desciptiption part in one own layout and every field can load it (to avoid copy/paste of HTML code)?

Js is definitely not the solution as the entire point was to remove it

Having thought about it this more overnight this approach is not correct. It started off just for the search box but then I got carried away adding ito to all the other fields. It was correct for the search box but not for the fields as it takes us back to the J3 situation of not knowing if something has a description or not. For the fields I think that displaying them as they are now is the only option. I will close this PR and create a new one for the search field only AND to escape the description. Thanks @bembelimen and @SniperSister for your input etc

See #24892