No-js bulk deletion of messages

[lakrisgubben]

Currently messages can only be bulk selected/deleted if js is enabled. Not only this, but the dropdown for "selecting" messages as well as deleting selected messages are visible even if body.no-js.

This was discussed in #5513 and here's a first attempt at a patch for adding no-js support for bulk deletion of messages.

This patch hides the controls that are dependent on js if body.no-js, and moves the checkboxes to the left to be consistent with the notifications bulk management and other parts of bp+wp.

Some things might appear strange in this patch, namely that the <input type="submit"> for deleting marked messages is wrapped in a <noscript>-tag. I did this to not have to change anything in the javascript (which references an <a>-tag) so as to not make things break if people roll their own version of buddypress.js.

I've made use of, and altered a bit, the function messages_action_bulk_delete(). I couldn't find any place in BP where this function is used, but tried to make sure that it would work as expected if used with it's old values... Any feedback on this would be greatly appreciated though. Maybe I should write a separate function for this?

[boonebgorges]

Awesome - thanks for the patch.

I've made use of, and altered a bit, the function messages_action_bulk_delete(). I couldn't find any place in BP where this function is used, but tried to make sure that it would work as expected if used with it's old values... Any feedback on this would be greatly appreciated though.

I think it's used as the AJAX callback for the JS bulk delete (though this could use some testing). I guess it's probably fine to reuse it, but if so, we'd need some unit tests first to verify that we're not breaking the existing functionality. (Looks like you've covered it, but the unit tests will increase my confidence.)

The use of the "Delete Selected" link is in keeping with the current JS implementation of bulk deletion, but I wonder if we might be a bit bolder here: maybe this is an opportunity to bring this interface in line with the (IMO far superior) notifications interface. That'd mean:

• adding a th row at the top, which would include column headers as well as a "Select all" checkbox
• adding a "Bulk Actions" dropdown to the bottom of the list (alongside "Select")
• in "Bulk Actions", have "Delete" in addition to "Mark Read" and "Mark Unread"

So this ends up being a somewhat more significant overhaul of the functionality. What do you think of this? If you like the idea, I might ping one or two more people who have thought a lot about messages in the past, to get their feedback.

[lakrisgubben]

Replying to boonebgorges:

I think it's used as the AJAX callback for the JS bulk delete (though this could use some testing). I guess it's probably fine to reuse it, but if so, we'd need some unit tests first to verify that we're not breaking the existing functionality. (Looks like you've covered it, but the unit tests will increase my confidence.)

As far as I can tell, the JS (​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-templates/bp-legacy/js/buddypress.js#L1502) uses the function bp_legacy_theme_ajax_messages_delete() (​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-templates/bp-legacy/buddypress-functions.php#L1391)

The use of the "Delete Selected" link is in keeping with the current JS implementation of bulk deletion, but I wonder if we might be a bit bolder here: maybe this is an opportunity to bring this interface in line with the (IMO far superior) notifications interface. That'd mean:

• adding a th row at the top, which would include column headers as well as a "Select all" checkbox
• adding a "Bulk Actions" dropdown to the bottom of the list (alongside "Select")
• in "Bulk Actions", have "Delete" in addition to "Mark Read" and "Mark Unread"

So this ends up being a somewhat more significant overhaul of the functionality. What do you think of this? If you like the idea, I might ping one or two more people who have thought a lot about messages in the past, to get their feedback.

Totally agree that this'd be nice. Just wasn't sure if there where any interest (or if there'd be back-compat concerns) in such a "big" rewrite. :) But if you'd like, please get some more people involved and I can see what I can do with it.

[boonebgorges]

Ping sent. Thanks for your thoughts and for your work on this!

[lakrisgubben]

looking at how the js for bulk deletion of messages works now it's not really perfect. :) As talked about in #5513 regarding notifications, there is a similar problem here. When an ajax deletion is done, it doesn't change the "Viewing N - N of N messages" and it keeps the pagination which could 404 if you have a total of 11 messages and then delete like 2 or something.

So this behaviour should really be fixed as well. Either by ajaxing back a "full" template, or by only doing this via a page reload. Do we have any good examples in BP of loading in templates via ajax?

[boonebgorges]

Group invitations do a full-template refresh via AJAX. It has its own problems - namely, that it can feel slow and a bit jerky if doing multiple consecutive actions (there's a ticket somewhere for this). But it's a starting place to see how we do it elsewhere.

[hnla]

Would absolutely agree we should bring message screens into line where possible with the notifications ones. messaging table leaves a lot to be desired in terms of table layout so essentially replicating notifications will be a good thing. I'll download and test first pass patch.

[boonebgorges]

Many thanks, hnla. lakrisgubben - consider that your green light to go with the more expansive work.

[hnla]

In the css changes we have rather lot of display: none rulesets building up can we group those selectors and save a few lines.

[DJPaul]

Oh, I assume the patch needs updating after the changes in #5513.

[lakrisgubben]

So,

I've been tinkering a bit with this and some thoughts have come up that I'd love to get some feedback on (@boone @hnla).

1. At first I though about moving the "select (read/unread/all)"-box to the <th> instead of having a checkbox as on the notifications screen. But this doesn't really feel right. But having both a checkbox for selecting all (in the <th>) as well as the select-box seems like overkill. To be honest I almost feel like the select-box seems like overkill in itself. I mean you've got a screen with 10 messages on it, do you really need a way to toggle only the read/unread? (Might be out of line here, I don't know, just floating the idea). Especially if we add another select-box to deal with the choice of deleting/mark read/mark unread of the selected messages, it feels like it's going to get cluttered.

2. Why do we only show the unread count for the messages? Wouldn't it be nice to show the total number of messages in the thread (like in gmail or some other mail-app)? The fact that the thread contains unread messages is quite clear anyways, different background, red background for the count, bold text.

3. Which brings me on to the bold text on unread messages. I think it gets unnecessary hard to read and stands out in an annoying way that all of the text on unread messages are bold. Any reason for making all of it bold?

4. I think we should have a "mark read/mark unread" link along with the delete link next to each thread, just as we have with notifications. Anyone opposed to this?

[slaFFik]

2. Accessibility issues - can't rely on colour, background etc. So imo numbers for unread only is a good decision. We can display extra info on a thread page.
3. The same as gmail (as you are referring to it earlier) - author, subject and date are all bold there too.
4. Actually that is in the core already and in the other place, was just hidden due to a bug. Patched in #4531

[boonebgorges]

I mean you've got a screen with 10 messages on it, do you really need a way to toggle only the read/unread?

I agree. I'd remove it. Let's hear what others have to say.

Why do we only show the unread count for the messages?

I think slaFFik's answer is probably partly right - it provides an a11y-friendly way to see what's unread. But I think the a11y concerns could probably be accomplished in other ways. I personally am not attached to the count boxes, and I do think they clutter the interface a bit. I'd like to hear what hnla in particular thinks.

Which brings me on to the bold text on unread messages

I'm kinda indifferent, but I agree that other email clients do it this way, so I lean toward leaving it the way it is.

I think we should have a "mark read/mark unread" link along with the delete link next to each thread, just as we have with notifications. Anyone opposed to this?

#4531 refers to the bulk actions, and has now been resolved. I'm not opposed in theory to adding per-thread links, though I don't want it to introduce too much clutter to the page. I'd be glad to see a patch or a mockup.

[lakrisgubben]

Thanks for the feedback slaFFik and boonebgorges! :)

The same as gmail (as you are referring to it earlier) - author, subject and date are all bold there too.

Not against having it on the first row (i.e from/to: name & subject). But the thing that also the excerpt from the message and the date are bolded is a bit too much imho. Also if we're going to move the messages list in line with the notification list I'd suggest not having the delete-single-thread link as a button, and instead as a link. And this doesn't look particularly good if it's bold. :)

I'm not opposed in theory to adding per-thread links, though I don't want it to introduce too much clutter to the page. I'd be glad to see a patch or a mockup.

I'll try it out in a patch and we can discuss it more

[hnla]

Replying to boonebgorges:

I mean you've got a screen with 10 messages on it, do you really need a way to toggle only the read/unread?

I agree. I'd remove it. Let's hear what others have to say.

Although I do quite like the option to be able to select one or the other en masse, I agree it's too cluttered to have too many selects especially where we only show quite a limited max number of messages per screen; so think I would have to agree too.

Why do we only show the unread count for the messages?

I think slaFFik's answer is probably partly right - it provides an a11y-friendly way to see what's unread. But I think the a11y concerns could probably be accomplished in other ways. I personally am not attached to the count boxes, and I do think they clutter the interface a bit. I'd like to hear what hnla in particular thinks.

This is a slightly more involved issue, the backgrounds are, as slaFFik point out, simply visual cues carrying no real semantic meaning, nothing wrong with that, but it does mean we need a further identifier as to the status of the information being conveyed. The numbering goes some way towards this, however in its self again actually carries little actual meaning it's a number! we only generally now to what this pertains through experience; someone seeing this for the first time would be forgiven for looking blank.

Properly describing the table (part of the exercise and point of this ticket to match to notifications) with TH elements will allow for a clear 'Unread' column heading, added to this and given we count the number of unreads within the thread I would suggest we also add a '0' count for no unreads to keep the column reading in a sane manner "Unread: 0" rather than suddenly being devoid of content.

If we want to take things a step further we could consider adding a span with screen reader text e.g:
<td><span class="unread-count">2</span><span class="bp-screen-reader-text">Unread messages</span></td>

I feel with a correctly described table replete with TH headers and fully scope attributed we cover things though.

Which brings me on to the bold text on unread messages

I'm kinda indifferent, but I agree that other email clients do it this way, so I lean toward leaving it the way it is.

I would agree it was unnecessary and BP needn't have been doing this which is tantamount to visual styling with no particular purpose, but equally am indifferent to changing it it's been out in the wild now for some time.

I think we should have a "mark read/mark unread" link along with the delete link next to each thread, just as we have with notifications. Anyone opposed to this?

#4531 refers to the bulk actions, and has now been resolved. I'm not opposed in theory to adding per-thread links, though I don't want it to introduce too much clutter to the page. I'd be glad to see a patch or a mockup.

Agreed not opposed either, slightly in favour I think, but best to see this in action so to speak to decide if it feels too cluttered.

[hnla]

Replying to lakrisgubben:

Thanks for the feedback slaFFik and boonebgorges! :)

The same as gmail (as you are referring to it earlier) - author, subject and date are all bold there too.

Not against having it on the first row (i.e from/to: name & subject). But the thing that also the excerpt from the message and the date are bolded is a bit too much imho. Also if we're going to move the messages list in line with the notification list I'd suggest not having the delete-single-thread link as a button, and instead as a link. And this doesn't look particularly good if it's bold. :)

Re-thinking the typographic emboldened text and think we should remove it, we want parity with notifications table , bold text does tend to bulk out/thicken too much if not careful - when I remove the bold text things look less stodgy.

Delete link as a button: Again we want one or other text or buttonized text, we currently have both - the button is the one out of place and should be taken back to text imo. since BP theme compat having buddypress make any decisions on visual styling that is anything but basic should be avoided and the reason we did remove so many styles in the big purge of BP properties.

[boonebgorges]

Regarding bold font: I'd definitely like to see what it looks like to make only the subject and sender in bold for unread messages. I agree that it seems like overkill to have the message excerpt and action links be bold.

[lakrisgubben]

I've uploaded a screenshot of a mockup using the 2014 theme. Seeing as there's (depending on what we decide) going to need to be some new functions written (mark single thread read/unread link amongst others) I thought it'd be less work to first decide on a route we'd like to try and then code the stuff. :)

So, the attached screenshot has gotten rid of the unread column totally, and instead opts for having the total message count (which I think would need a function if we'd decide to use it) in parenthesis after the senders name. But it also has some extra screen-reader info that tells the user about how many of those messages are unread. So the markup would look something like:

<td width="30%" class="thread-from">
    From: <a href="http://bp.dev/members/test/" title="test">test</a>
    (3)<span class="bp-screen-reader-text">messages 0 unread</span>
    <br>
    <span class="activity">November 11, 2014 at 7:34 pm</span>
</td>

This might be too much, was just an idea I got when fiddling around with the html. @hnla suggestion from comment16 would also work, even though if the th says "unread" that column will be very wide considering that it'll only contain a number.

The screenshot has also made the delete button a link and added Read/Unread buttons on each thread. It only has bold on the first line of the unread messages (if: from: user and subject). And removes the select read/unread/all dropdown and replaces it with a bulk action (mark read/mark unread/delete) dropdown.

Regarding the th-headings. The ones I've added should only be seen as placeholders and would love to get some thoughts on what to name them, maybe a native english speaker would be better at that than me. ;)

Any and all thoughts welcome!

[boonebgorges]

I'm liking the direction of the mockup, lakrisgubben.

So, the attached screenshot has gotten rid of the unread column totally, and instead opts for having the total message count (which I think would need a function if we'd decide to use it) in parenthesis after the senders name.

This looks good to me.

The screenshot has also made the delete button a link and added Read/Unread buttons on each thread. It only has bold on the first line of the unread messages (if: from: user and subject). And removes the select read/unread/all dropdown and replaces it with a bulk action (mark read/mark unread/delete) dropdown.

+1

Regarding the th-headings. The ones I've added should only be seen as placeholders and would love to get some thoughts on what to name them, maybe a native english speaker would be better at that than me. ;)

"From" and "Date" are fine as headings, but they don't really represent what's shown in those columns: "From" only contains an avatar, while "Date" contains more than just a date. Unless we're going to enable interactive sorting by these column headers, I'd suggest we should combine them into one column, with the header "From".

[lakrisgubben]

"From" and "Date" are fine as headings, but they don't really represent what's shown in those columns: "From" only contains an avatar, while "Date" contains more than just a date. Unless we're going to enable interactive sorting by these column headers, I'd suggest we should combine them into one column, with the header "From".

Agreed, let's put the avatar in the same column as the 'from:username', it really has no semantic meaning being in it's own column.

[hnla]

Agree on column merge.
All looking good to me.
Of course we need to keep one eye on changes like this and any possible ramifications for existing layouts/themes although don't think there would be.

[lakrisgubben]

latest patch does the things we've talked about above, i.e. lots of them. :)

It reformats the messages table to be a bit cleaner and more in line with the notifications table.

It add supports for marking a single message thread as read/unread, as well as bulk management (read/unread/delete) of messages.

The patch grew quite big since I had to write lots of template functions, almost all of them where inspired by the notifications component. Seeing as this is quite big, I'm certain there's some stupid stuff in there, so please bear with me regarding that. :)

This patch doesn't have any js/ajax support (except for selecting all, and disabling the submit button if no bulk action is choosen), this have to come later. :) The thing is that the current ajax support for deleting and bulk managing messages is quite broken since it doesn't take into account pagination etc. So I'd say that this is more complete than what exists.

I've tried to make sure that someone using an old version of buddypress.js or rolling their own messages-loop.php template file would still be okay, even though of course they won't get the new stuff.

[boonebgorges]

lakrisgubben - This patch is not applying cleanly - can you refresh it against the latest trunk?

Can you also post a screenshot or two of the new interface? I'd like to make this an item on tomorrow's dev chat agenda.

[hnla]

Also we seem to have some patches in a svn format and others in git, git diff files are problematic in a svn checkout as I'm running on windows, I can't apply them via tortoise or cli iirc.

[lakrisgubben]

@bonnebgorges

lakrisgubben - This patch is not applying cleanly - can you refresh it against the latest trunk?

Sorry about that, still not really used to trac+svn etc. :/ New patch applies cleanly for me.

Can you also post a screenshot or two of the new interface? I'd like to make this an item on tomorrow's dev chat agenda.

Done. I'll see if I can make it to the dev chat, but can't promise.

@hnla

Also we seem to have some patches in a svn format and others in git, git diff files are problematic in a svn checkout as I'm running on windows, I can't apply them via tortoise or cli iirc.

My bad, thought it was ok to post patches from git done with the --no-prefix. Latest patch is done through svn, hope it works. :)

[hnla]

Patch looking great - like the table layout columns condensed, works nicely.

I might suggest a few little enhancements: On the 'From' cells and as much as I'm not too fond of overmarking up pcdata think we should wrap the text 'from' in a classed span just to ensure people can easily target and style if necessary (although they could quite easily target the other elements) likewise the thread count as class 'thread-count' ? and perhaps with a title attr '# messages in this thread' or is that getting a little too pedantic?

My bad, thought it was ok to post patches from git done with the --no-prefix. Latest patch is done through svn, hope it works. :)

Seems to patch fine although I had to revert some changes in files but more my issue than anything and one was a patch in stylesheet for another ticket. Not your bad on the svn/git side of things I think you're in good company with the majority of devs who might be issuing the same git style patches it's just an issue I ran into before with darned tortoise and searching around for resolutions threw up more issues than solutions between the two.

[boonebgorges]

+1 to what hnla said - patch is looking great. I really like the new layout. We talked about the changes in this week's dev chat, and there was general agreement that this is a big improvement, and that we should go ahead with it for 2.2.

On that note, a couple nitty-gritty comments on the patch:

• There is some whitespace funkiness, like double-spaces between ! and bp_is_action_variable(). Have a second look at this.
• Our general pattern for checking nonces in "action" functions like bp_messages_action_mark_read() is: check wp_verify_nonce(), and if it fails, return false out of the function; if it passes, go ahead and do the logic. In other words, don't bother with bp_core_add_message() and the redirect if the nonce check fails.
• In bp_messages_action_bulk_manage(), there's something kinda funny about looping through each thread, and bailing within the loop if messages_check_thread_access() fails. It means that you could end up successfully performing the bulk action on *some* of the threads, but then display a message saying that there was a problem and not process the rest. It means a little extra looping, but I'd suggest doing a loop up front and checking for thread access *first*, and only process the action once the entire loop has been validated.
• Can we take this chance to move some of the table styling into the stylesheet? I'm thinking of the <td> widths.

[hnla]

Can we take this chance to move some of the table styling into the stylesheet? I'm thinking of the <td> widths.

Better can we just remove them ! quite right style attributes have long been deprecated, but also in our case we aren't actually adding explicit column widths to other tables and especially notifications, instead we are letting the table/cells work out the necessary widths as tables do for themselves quite happily - if anything I would prefer col/colgroup tags used if we needed to be explicit on how many columns and their widths a table had.

The only other attr I would consider for the tables are scopes, we should scope the columns, we can't, I think, scope the rows sadly.

[lakrisgubben]

@boonebgorges, @hnla: Thanks for the great feedback!

Latest patch removes the width on the <td>´s and add's scope to the <th>´s. It adds classes to 'from/to' and 'thread-count'.

It also loops through the messages in bulk_management and checks access and bails before doing the actual bulk management.

It removes some funky whitespace.

Our general pattern for checking nonces in "action" functions like bp_messages_action_mark_read() is: check wp_verify_nonce(), and if it fails, return false out of the function; if it passes, go ahead and do the logic. In other words, don't bother with bp_core_add_message() and the redirect if the nonce check fails.

I did this on the bp_messages_action_bulk_manage(). Wasn't sure if you meant on other places as well Boone.

[hnla]

.4 looks great, nice job!

[boonebgorges]

I did this on the bp_messages_action_bulk_manage().

Looks perfect there. Can you give the same treatment to the _mark_read() and _mark_unread() functions too?

[lakrisgubben]

Replying to boonebgorges:

Looks perfect there. Can you give the same treatment to the _mark_read() and _mark_unread() functions too?

done in latest patch :)

[boonebgorges]

Awesome, thanks!

I meant to ask in my last comment, but I'll ask now: Do we really need the new template functions to create the link markup? It's unfortunate that you've had to use inconsistent naming conventions in the functions you've proposed (_link_formatted() in some places, _link() in others), and it makes me wonder whether we shouldn't just build these links in the template itself. This is especially true since these functions are just wrappers - they don't have any internal logic or accept any arguments. What do you think?

[lakrisgubben]

It's unfortunate that you've had to use inconsistent naming conventions in the functions you've proposed (_link_formatted() in some places, _link() in others),

Agreed, that that wasn't perfect, but I guess you saw that it was because there already existed a (kind of poorly) named function for getting the delete-url. :/

This is especially true since these functions are just wrappers - they don't have any internal logic or accept any arguments. What do you think?

I'm not sure that having all that in the templates would be so nice (even though there's not supermuch logic in them, except for the read/unread link). Generally I really like how it's done in the notificatons list. The less code in the template-files the better, but that might just be my personal opinion. :) And even though I can't right now see any other place where these template-tags would be used, you never know in the future. ;)

Just out of curiosity, why wasn't this approach taken in notifications-loop?

I'm however fine with making changes to it if you'd prefer that!

Just so that we're on the same page, you're suggesting something like this in messages-loop.php

<td class="thread-options">
<?php if ( bp_has_unread_messages() { ?>
<a class="read" href="<?php bp_mark_message_read_link();?>">read</a>
<?php } else { ?>
<a class="unread" href="<?php bp_mark_message_unread_link();?>">unread</a>
<?php } ?>
 | 
<a class="delete" href="<?php bp_delete_message_link(); ?>">delete</a>
</td>

[boonebgorges]

I'm not sure that having all that in the templates would be so nice

Aesthetically, it's nice for the templates to be simple. But requiring that theme authors build anchor tag markup is not onerous, and it provides the ultimate flexibility for people who want to use completely different types of markup.

Another way to look at it: if we decide not to include these wrappers today, we can always revisit the decision in a version or two. If we decide to introduce them today, we're stuck with them forever.

Could you do me a favor and do a quick review of other components to see what we're doing there? You've already done so with notifications, but that's a bit of an outlier because it was just built. If we're providing these wrappers for all components, let's bite the bullet and do it here too. If notifications is the only one, let's not.

[lakrisgubben]

Could you do me a favor and do a quick review of other components to see what we're doing there? You've already done so with notifications, but that's a bit of an outlier because it was just built. If we're providing these wrappers for all components, let's bite the bullet and do it here too. If notifications is the only one, let's not.

Not really sure where to look since it only notifications and messages that has these kinds of list-tables.. But in the group-invites and friends-requests loops on a single member page we don't use these kind of wrappers. So I guess we shouldn't do it here either.

So, should I just remove all of the template tags that has to do with single-message mark read/unread except for those that output just the url? (Which should just mimic the bp_message_thread_delete_link() function?)

[boonebgorges]

But in the group-invites and friends-requests loops on a single member page we don't use these kind of wrappers. So I guess we shouldn't do it here either.

Agreed. Thanks for looking.

should I just remove all of the template tags that has to do with single-message mark read/unread except for those that output just the url?

Yup. Thanks :)

[lakrisgubben]

Replying to boonebgorges:

should I just remove all of the template tags that has to do with single-message mark read/unread except for those that output just the url?

Yup. Thanks :)

Fixed in latest patch.

[boonebgorges]

In 9161:

Improvements to the messages thread interface.

• Introduce noscript support for bulk actions.
• Revise structure and markup to better match the Notifications table.
• Add "Read/Unread" and "Delete" links for each message thread.
• Remove unnecessary "Select Read/Unread" dropdown.
• Introduce a number of new template functions in support of the new interface.

Props lakrisgubben.
See #6005.

[boonebgorges]

I cleaned up the latest patch (internationalization in particular needed a once-over in a couple places) and committed it in [9161].

I did decide to put a small amount of styling on the thread-info table column, because without it, that column would get really weirdly narrow if all the available excerpts had short excerpts, with the difference made up for with a weirdly wide From column. hnla and lakrisgubben, if you have suggestions about how to improve this, they'd be welcome.

Leaving the ticket open for now because there are bound to be some issues with this fairly large number of changes. Testing and fixes are welcome. Thanks for your work!

[boonebgorges]

Haven't heard any follow up or bug reports, so let's close this ticket. Further issues can be discussed in separate tickets. lakrisgubben - thanks very much for your work here.

[hnla]

@Boone Sorry meant to comment, however your solution acceptable so wasn't too concerned to. The table cell behaviour you describe is normal as such; a cell will adapt to the content it needs to display if I simplify the info cells to one character then the subject one will naturally widen as we have a fairly uniform long string that wants to display on one line, table judges what cells need what widths if I select one of my rows and then start adding characters to 'info' cell all cells will start to re-calculate for best fit widths; tables are one of the smarter constructs. Personally I wouldn't have been concerned with the wider cell seeing it as natural, but it's all good! Not seen or noticed any other issues of concern with this ticket.