Allowing styles to be applied to text fields - problem
For h5p content, I have added some font styling (e.g. font-size, background-color). I ahve altered the semantics so that semantics->font->size = true; and $field->font->background = true;. However the rendered html does not include these styles.
Looking into this a bit further I belive the problem could lie in the following module / function profiles\opigno_lms\modules\contrib\h5p\library\h5p.classes.php:_filter_xss_attributes() ~line3518
<?php
if ($allowedStyles && $attrName === 'style') {
// Allow certain styles
foreach ($allowedStyles as $pattern) {
if (preg_match($pattern, $match[1])) {
$attrArr[] = 'style="' . $match[1] . '"';
break;
}
}
break;
}
?>
This seems to cater only for a single style to be checked against the allowed styles. If the styles="..." has a list of styles separated by semi-colons as expected, it seems to come up with not allowed (as the preg_match is looking for start of string).
I am testing a fix for this, and will post it here if anyone else has a similar issue, and concurs with my analysis.
falcon
Fri, 07/15/2016 - 15:35
Permalink
The CK Editor adds span tags
The CK Editor adds span tags per style, see https://h5p.org/node/18890 - that is why the code is the way it is. Have added some comments to the code now. I thought it was wrong too until I created the test node.
What are you trying to do. Is this rather strange approach problematic?
Dominic Baker
Sat, 07/16/2016 - 09:31
Permalink
I would like to be able to style at a div level
Thanks. I see what you mean. And how the code copes with the specific html generated by the CKeditor. Ideally I would like to be able to use styled divs and even nest them. We are importing the html so not using CKeditor solely. Is there a reason as no why it would not be a good idea to have the code cope with both eventualities (essentially an extra foreach loop). So that more than a single style could be coped with.
style="text-align: center;background-color:#ff0000;color:#ffffff;font-size:2em"
I am still testing but this is the general idea<?php //$match[1] has the whole 'style' string of semicolon separated styles //we need to test these each to see if they area allowed and let them through if they are //split $match[1] into an array of individual styles $requestedStyleA = explode(";",$match[1]); $requestedStyleA = array_map('trim', $requestedStyleA); $acceptedStyleA = array(); foreach ($requestedStyleA as $requestedStyle) { foreach ($allowedStyles as $pattern) { if (preg_match($pattern, $requestedStyle)) { $acceptedStyleA[] = $requestedStyle; break; //out of foreach ($allowedStyles as $pattern) } } } $attrArr[] = 'style="' . implode(";",$acceptedStyleA). '"'; ?>
Dominic Baker
Sat, 07/16/2016 - 09:44
Permalink
Sorry, more complete code
Thanks. I see what you mean. And how the code copes with the specific html generated by the CKeditor. Ideally I would like to be able to use styled divs and even nest them. We are importing the html so not using CKeditor solely. Is there a reason as no why it would not be a good idea to have the code cope with both eventualities (essentially an extra foreach loop). So that more than a single style could be coped with.
style="text-align: center;background-color:#ff0000;color:#ffffff;font-size:2em"
I am still testing but this is the general idea<?php if ($allowedStyles && $attrName === 'style') { //$match[1] has the whole 'style' string of semicolon separated styles //we need to test these each to see if they area allowed and let them through if they are //split $match[1] into an array of individual styles $requestedStyleA = explode(";",$match[1]); $requestedStyleA = array_map('trim', $requestedStyleA); $acceptedStyleA = array(); foreach ($requestedStyleA as $requestedStyle) { foreach ($allowedStyles as $pattern) { if (preg_match($pattern, $requestedStyle)) { $acceptedStyleA[] = $requestedStyle; break; //out of foreach ($allowedStyles as $pattern) } } } $attrArr[] = 'style="' . implode(";",$acceptedStyleA). '"'; break; } ?>
thomasmars
Tue, 07/19/2016 - 16:56
Permalink
Excellent,
We definitively should have a solution that can cope with both use cases. Please let us know how it turns out for you. We would love a pull request if you find a good solution for https://github.com/h5p/h5p-php-library
Dominic Baker
Wed, 07/20/2016 - 09:17
Permalink
Will do, but....
Thanks Thomas. I will happily do so, but must confess to never having used github before, so do not know how to do a 'pull request'. I will have to check out what I need to do re git software and accounts first. (Unless you could direct me to a 'Getting started with git' that you recommend.)
thomasmars
Wed, 07/20/2016 - 10:40
Permalink
Github help
The Github help documentation is a pretty good I think :)
Dominic Baker
Fri, 07/22/2016 - 13:02
Permalink
Test cases
What would be the best way to supply test cases? Is there a standard way? And coding standards, I've seen them somewhere. I'd like to get this right!
icc
Fri, 07/22/2016 - 16:19
Permalink
Currently, I don't believe
Currently, I don't believe there are any test cases. This is the GPL part of the code and has been copied from Drupal 7 and then modified to support CSS styles, in a more or less restrictive/whitelist way. I imagine they have some test cases but not much related to CSS.
Coding wise, sticking to the standards of Drupal should be good enough: https://www.drupal.org/coding-standards