Allow Wysiwyg styles

The whole styles formatting thing is a bit tough to deal with, and this doesn't seem like the right approach. BUT I'd like to see something more than nothing. At least we should define the list of allowed properties in a variable instead of hardcoding it.

subscribe

ops

Actually, this is a WTF... why is it in the ->thumbnail method? That makes no sense, but I don't understand styles 7.2 yet, so I might need to do a little research.

Okay, I spent a 1/2 hr running styles through my debugger...

The code could use a lot more comments. I kinda get what is going on, but the variables are so abstract and so it's easy to lose oneself. There are several things I noticed which could be fixed to use more modern OOP techniques in PHP, but that's for a separate issue #[1103716] is a good start, and I'd like to see it committed.

Regarding the rest of the structure of overrides, I don't get why width and height should be treated differently from other attributes, or why there is a hardcoded list of allowed attributes there...

I'm not sure what the right answer is yet, but it's confusing me.

I'm a little confused by this too. Maybe we need a high level overview - why does the File Styles module need to hardcode a whitelist of style attributes at all?

Styles 7.x-1.x didn't do this at all (as far as I know) and the WYSIWYG stuff seemed to work fine - I believe we fixed that in #841104: File styles suppress most image attributes.

Obviously there does need to be security, but the question is whose job it is to provide it; is it always the Styles module's job? When this stuff gets called from Media, it's used within the text format system, so depending on what filtering you have (or intentionally choose not to have) as part of your text format you might be taking care of this on your own and not really want Styles to hardcode a whitelist of style attributes for you.

(On the other hand, if this patch gets things working for the time being, maybe the approach here is good enough to go with for now, until the larger issues are worked out...)

subscribe

Subscribe

Obviously there does need to be security, but the question is whose job it is to provide it; is it always the Styles module's job?

I agree with David. It is NOT the Styles module's job to remove unwanted attributes and css properties. That's what the text filter system is for. Any whitelisting needs to happen in the appropriate filter (e.g., media_filter()). media_filter() already whitelists attributes. If whitelisting css properties is desired, it can be added there.

Unfortunately, the hard-coded list in the Styles module is currently necessary, not for security, but because $override contains a mix of things: html attributes, css properties, settings, flags, and more.

Here's an updated patch that adds 'display' and 'float' as per #1, also adds attributes which were missing (e.g., CKEditor allows setting the html id and class), and adds some todo comments for removing the hard-coded lists, which may require adding better organization to $override. Note that patch is against alpha5, which is only insignificantly behind HEAD, but that's why it's a CVS patch instead of a GIT patch.

#10 works. Float right is saved in my case. But for TinyMCE right now, CKeditor tries to save following code and fails saving "align":"right".

[[{"type":"media","view_mode":"media_preview","fid":"2","attributes":{"align":"right","alt":"","border":"2","class":"media-image","style":"padding:5px;margin-left: 5px;","typeof":"foaf:Image"}}]]

Two things that probably belongs to another issue

1) HTML is not nice, I get "float: right;;" with double semicolon
2) I wish class would be added, not style

Cross-posting from #1043570: Floating images left and right:

If I set the width, height, and float, this is what is output: img width="300" height="100" style="float: right; width: 300px; height: 100px; ;;float:right;;float:right;".

subscribe

The patch seems to be working well for me with images, but I need to figure how to do this with embed code for youtube media.

I wasn't able to get the patch in #10 to apply -- looks like it was built on CVS and my Git install was having nothing to do with it. I made the changes by hand and rerolled using Git, in case that helps anyone. This patch worked for me to get images floating left/right correctly using CKEditor. See my comment and patch in the Media project for more details.

Maybe this issue #1170736: Better compatibility with media filter (WYSIWYG related) helps in some way.

here it is against HEAD, which has recently changed considerably, so the old patch didn't apply.

In the interest of moving things along, I've committed this patch. Can we open new issues for the other issues?

Thanks,
Aaron