Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 years ago

#5835 closed enhancement (fixed)

Allow a few more tags/attr on bp allowed tags filter.

Reported by: hnla Owned by: boonebgorges
Milestone: 2.2 Priority: low
Severity: minor Version:
Component: Core Keywords: has-patch
Cc: hnla

Description

Running on from ticket ##3374

Perhaps allowing a few more elements and atts through the bp_allowed_activity_tags filter might be useful?

I would suggest adding <b>, <i> as two essential typographic visual conventions (not strong or em).

The anchor element would benefit from having the 'title' attr - this is arguably a more or as equally important attr as the provided 'rel' one.

The provided span tag in this context (similar in vein to the wp editor usage) is really only of use if one is able to also add the 'style' attr without this attr it's not really useful to general users who won't know what class to add nor developers/site admins to know what styles to apply to a possibly unknown token (granted probably the majority of users won't know what properties might be added via the style tag)

If there's agreement I can test and patch.

Attachments (1)

5835.patch (1.4 KB) - added by hnla 6 years ago.
Add b,i, elements & title attr to allowed tags

Download all attachments as: .zip

Change History (10)

#1 @boonebgorges
6 years ago

b, i, a:title - yes

What are you suggesting regarding span? That we remove it, or that we add the style attribute?

#2 @boonebgorges
6 years ago

  • Keywords needs-patch added

#3 @hnla
6 years ago

What are you suggesting regarding span?

That we add the style attribute, without it in this sort of context i.e outside external styles where only inline styles would be applicable, it's fairly useless, as a dev I won't necessarily know what class someone may decide to add or where the span appears thus can't target any specific styles in advance - but I'll concede that general users might be hard pressed to know how to use the style attribute, but think it might as well be allowed as not?

#4 @boonebgorges
6 years ago

  • Milestone changed from Awaiting Review to 2.2

without it in this sort of context i.e outside external styles where only inline styles would be applicable, it's fairly useless,

I don't know about that. I've seen plenty of places where people use this kind of selector:

div.some-specific-class-name > span {

That suggests that a span alone could be useful in some cases.

I'm pretty sure the motivation for not allowing this attributes is, in part, that we don't see an obvious need to allow users to manually enter this kind of HTML into a status update. That said, *all* activity passes through these filters, not just status updates - so we should be more sensitive to the possibility that fully-formed markup may be provided in some cases.

On the other hand, we don't allow the 'style' attribute in any of our allowed_tags filters, and there are good reasons why we may want to leave this as the default. The activity stream should act like a "controlled" area. For example, in the case of blog posts, we strip inline images, and use the first image in the post (or the featured image) as a leader in the activity content. We do this to maintain uniformity in the appearance of the activity stream. Permitting BP components (or, heaven forbid, users) to send arbitrary styling into the activity stream seems like it's asking for trouble. Would you want an activity item's content to contain 64px pink Comic Sans letters?

So, in the case of 'span', I'm leaning toward leaving it as is. I'd welcome another dev's opinion, though.

hnla - Feel free to write up a patch for the other tags, and we can at least take care of that early in 2.2.

#5 @DJPaul
6 years ago

I also don't think allowing style tags is a great idea, for similar reasons.

#6 @hnla
6 years ago

Fair enough. it is a possible issue that could arise and for that reason best to avoid. However think 64px pink comic sans could work under the right circumstances, but I would mal-form the closing span to ensure all following elements inherited the same then it wouldn't look out of place. :)

I'll patch for the other tags/attr.

#7 @boonebgorges
6 years ago

However think 64px pink comic sans could work under the right circumstances

Yes, and for those circumstances developers can filter the allowed_tags. We've gotta provide sensible defaults.

I'll patch for the other tags/attr.

Thanks!

@hnla
6 years ago

Add b,i, elements & title attr to allowed tags

#8 @hnla
6 years ago

  • Keywords has-patch added; needs-patch removed

#9 @boonebgorges
6 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 9050:

Add 'b', 'i', and 'a:title' to allowedtags in bp-activity

Fixes #5835

Props hnla

Note: See TracTickets for help on using tickets.