message_list: Don't split message groups for a date divider.

This adds date dividers within a single message group when the only
reason we had previously been splitting apart two message groups is a
change of date.  The overall effect is a cleaner message list user
experience.

The downside of this change would be that the recipient bars no longer
will always show a new date for date changes; to fix that, we rewrite
how the floating recipient bars both set the date field on the
floating recipient bar itself, as well as ensure that non-floating
recipient bars don't show duplicate dates.

In a future design update where we modify how message recipient bars
look, we may very well be able to simplify this logic by removing some
of the dynamic nature of the recipient bar calculations.  But this is
a good implementation of what remains.

Tweaked significantly by tabbott from Steve Howell's original, both to
extract these changes from a larger PR as well as to modify the
first_visible_message logic to handle some tricky corner cases.

Fixes #10171.

frontend_tests/node_tests/message_list_view.js

@@ -76,7 +76,6 @@ run_test('merge_message_groups', () => {
         return {
             message_containers: messages,
             message_group_id: _.uniqueId('test_message_group_'),
-            group_date_divider_html: 'some html',
         };
     }
 
@@ -181,6 +180,35 @@ run_test('merge_message_groups', () => {
         assert.deepEqual(result.rerender_messages, []);
     }());
 
+    (function test_append_message_different_subject_and_days() {
+
+        var message1 = build_message_context({timestamp: 1000});
+        var message_group1 = build_message_group([
+            message1,
+        ]);
+
+        var message2 = build_message_context({topic: 'Test subject 2',
+                                              timestamp: 900000});
+        var message_group2 = build_message_group([
+            message2,
+        ]);
+
+        var list = build_list([message_group1]);
+        var result = list.merge_message_groups([message_group2], 'bottom');
+
+        assert_message_groups_list_equal(
+            list._message_groups,
+            [message_group1, message_group2]);
+        assert_message_groups_list_equal(result.append_groups, [message_group2]);
+        assert.deepEqual(result.prepend_groups, []);
+        assert.deepEqual(result.rerender_groups, []);
+        assert.deepEqual(result.append_messages, []);
+        assert.deepEqual(result.rerender_messages, []);
+        assert.equal(
+            message_group2.group_date_divider_html,
+            '900000000 - 1000000');
+    }());
+
     (function test_append_message_different_day() {
 
         var message1 = build_message_context({timestamp: 1000});
@@ -196,15 +224,13 @@ run_test('merge_message_groups', () => {
         var list = build_list([message_group1]);
         var result = list.merge_message_groups([message_group2], 'bottom');
 
-        assert(message_group2.group_date_divider_html);
+        assert_message_groups_list_equal(list._message_groups, [message_group1]);
-        assert_message_groups_list_equal(
+        assert.deepEqual(result.append_groups, []);
-            list._message_groups,
-            [message_group1, message_group2]);
-        assert_message_groups_list_equal(result.append_groups, [message_group2]);
         assert.deepEqual(result.prepend_groups, []);
         assert.deepEqual(result.rerender_groups, []);
-        assert.deepEqual(result.append_messages, []);
+        assert.deepEqual(result.append_messages, [message2]);
-        assert.deepEqual(result.rerender_messages, []);
+        assert.deepEqual(result.rerender_messages, [message1]);
+        assert(list._message_groups[0].message_containers[1].want_date_divider);
     }());
 
     (function test_append_message_historical() {
@@ -311,6 +337,36 @@ run_test('merge_message_groups', () => {
         assert.deepEqual(result.rerender_messages, []);
     }());
 
+    (function test_prepend_message_different_subject_and_day() {
+
+        var message1 = build_message_context({timestamp: 900000});
+        var message_group1 = build_message_group([
+            message1,
+        ]);
+
+        var message2 = build_message_context({topic: 'Test Subject 2',
+                                              timestamp: 1000});
+        var message_group2 = build_message_group([
+            message2,
+        ]);
+
+        var list = build_list([message_group1]);
+        var result = list.merge_message_groups([message_group2], 'top');
+
+        // We should have a group date divider between the recipient blocks.
+        assert.equal(
+            message_group1.group_date_divider_html,
+            '900000000 - 1000000');
+        assert_message_groups_list_equal(
+            list._message_groups,
+            [message_group2, message_group1]);
+        assert.deepEqual(result.append_groups, []);
+        assert_message_groups_list_equal(result.prepend_groups, [message_group2]);
+        assert.deepEqual(result.rerender_groups, [message_group1]);
+        assert.deepEqual(result.append_messages, []);
+        assert.deepEqual(result.rerender_messages, []);
+    }());
+
     (function test_prepend_message_different_day() {
 
         var message1 = build_message_context({timestamp: 900000});
@@ -326,15 +382,16 @@ run_test('merge_message_groups', () => {
         var list = build_list([message_group1]);
         var result = list.merge_message_groups([message_group2], 'top');
 
+        // We should have a group date divider within the single recipient block.
         assert.equal(
-            message_group1.group_date_divider_html,
+            message_group2.message_containers[1].date_divider_html,
             '900000000 - 1000000');
         assert_message_groups_list_equal(
             list._message_groups,
-            [message_group2, message_group1]);
+            [message_group2]);
         assert.deepEqual(result.append_groups, []);
-        assert_message_groups_list_equal(result.prepend_groups, [message_group2]);
+        assert.deepEqual(result.prepend_groups, []);
-        assert_message_groups_list_equal(result.rerender_groups, [message_group1]);
+        assert_message_groups_list_equal(result.rerender_groups, [message_group2]);
         assert.deepEqual(result.append_messages, []);
         assert.deepEqual(result.rerender_messages, []);
     }());

static/js/floating_recipient_bar.js

@@ -5,17 +5,230 @@ var exports = {};
 
 var is_floating_recipient_bar_showing = false;
 
+function top_offset(elem) {
+    return elem.offset().top - $('#tab_bar').safeOuterHeight();
+}
+
+exports.first_visible_message = function (bar) {
+    // The first truly visible message would be computed using the
+    // bottom of the floating recipient bar; but we want the date from
+    // the first visible message were the floating recipient bar not
+    // displayed, which will always be the first messages whose bottom
+    // overlaps the floating recipient bar's space (since you ).
+
+    var messages = bar.children('.message_row');
+    var frb_bottom = exports.frb_bottom();
+    var frb_top = frb_bottom - 25;
+    var result;
+
+    for (var i = 0; i < messages.length; i += 1) {
+        // The details of this comparison function are sensitive, since we're
+        // balancing between three possible bugs:
+        //
+        // * If we compare against the bottom of the floating
+        //   recipient bar, we end up with a bug where if the floating
+        //   recipient bar is just above a normal recipient bar while
+        //   overlapping a series of 1-line messages, there might be 2
+        //   messages occluded by the recipient bar, and we want the
+        //   second one, not the first.
+        //
+        // * If we compare the message bottom against the top of the
+        //   floating recipient bar, and the floating recipient bar is
+        //   over a "Yesterday/Today" message date row, we might
+        //   confusingly have the floating recipient bar display
+        //   e.g. "Yesterday" even though all messages in view were
+        //   actually sent "Today".
+        //
+        // * If the the floating recipient bar is over a
+        //   between-message groups date separator or similar widget,
+        //   there might be no message overlap with the floating
+        //   recipient bar.
+        //
+        // Careful testing of these two corner cases with
+        // message_viewport.scrollTop() to set precise scrolling
+        // positions determines the value for date_bar_height_offset.
+
+        var message = $(messages[i]);
+        var message_bottom = top_offset(message) + message.safeOuterHeight();
+        var date_bar_height_offset = 10;
+
+        if (message_bottom > frb_top) {
+            result = message;
+        }
+
+        // Important: This will break if we ever have things that are
+        // not message rows inside a recipient_row block.
+        message = message.next('.message_row');
+        if (message.length > 0 && result) {
+            // Before returning a result, we check whether the next
+            // message's top is actually below the bottom of the
+            // floating recipient bar; this is different from the
+            // bottom of our current message because there may be a
+            // between-messages date separator row in between.
+            if (top_offset(message) < frb_bottom - date_bar_height_offset) {
+                result = message;
+            }
+        }
+        if (result) {
+            return result;
+        }
+    }
+
+    // If none of the messages are visible, just take the last message.
+    return $(messages[messages.length - 1]);
+};
+
+exports.get_date = function (elem) {
+    var message_row = exports.first_visible_message(elem);
+
+    if (!message_row || !message_row.length) {
+        return;
+    }
+
+    var msg_id = rows.id(message_row);
+
+    if (msg_id === undefined) {
+        return;
+    }
+
+    var message = message_store.get(msg_id);
+
+    if (!message) {
+        return;
+    }
+
+    var time = new XDate(message.timestamp * 1000);
+    var today = new XDate();
+    var rendered_date = timerender.render_date(time, undefined, today)[0].outerHTML;
+
+    return rendered_date;
+};
+
 exports.frb_bottom = function () {
     var bar = $("#floating_recipient_bar");
-    var bar_top = bar.offset().top;
+    var bar_top = top_offset(bar);
     var bar_bottom = bar_top + bar.safeOuterHeight();
 
     return bar_bottom;
 };
 
-exports.obscured_recipient_bar = function () {
+exports.relevant_recipient_bars = function () {
-    // Find the recipient bar that is closed to being onscreen
+    var elems = [];
-    // but above the "top".
+
+    // This line of code does a reverse traversal
+    // from the selected message, which should be
+    // in the visible part of the feed, but is sometimes
+    // not exactly where we want.  The value we get
+    // may be be too far up in the feed, but we can
+    // deal with that later.
+    var first_elem = exports.candidate_recipient_bar();
+
+    if (!first_elem) {
+        first_elem = $('.focused_table').find('.recipient_row').first();
+    }
+
+    if (first_elem.length === 0) {
+        return [];
+    }
+
+    elems.push(first_elem);
+
+    var max_offset = top_offset($('#compose'));
+    var header_height = first_elem.find('.message_header').safeOuterHeight();
+
+    // It's okay to overestimate header_height a bit, as we don't
+    // really need an FRB for a section that barely shows.
+    header_height += 10;
+
+    function next(elem) {
+        return elem.next('.recipient_row');
+    }
+
+    // Now start the forward traversal of recipient bars.
+    // We'll stop when we go below the fold.
+    var elem = next(first_elem);
+
+    while (elem.length) {
+
+        if (top_offset(elem) < header_height) {
+            // If we are close to the top, then the prior
+            // elements we found are no longer relevant,
+            // because either the selected item we started
+            // with in our reverse traversal was too high,
+            // or there's simply not enough room to draw
+            // a recipient bar without it being ugly.
+            elems = [];
+        }
+
+        if (top_offset(elem) > max_offset) {
+            // Out of sight, out of mind!
+            // (The element is below the fold, so we stop the
+            // traversal.)
+            break;
+        }
+
+        elems.push(elem);
+        elem = next(elem);
+    }
+
+    if (elems.length === 0) {
+        blueslip.warn('Unexpected situation--maybe viewport height is very short.');
+        return [];
+    }
+
+    var items = _.map(elems, function (elem, i) {
+        var date_html;
+        var need_frb;
+
+        if (i === 0) {
+            date_html = exports.get_date(elem);
+            need_frb = top_offset(elem) < 0;
+        } else {
+            date_html = elem.find('.recipient_row_date').html();
+            need_frb = false;
+        }
+
+        var date_text = $(date_html).text();
+
+        // Add title here to facilitate troubleshooting.
+        var title = elem.find('.message_label_clickable').last().attr('title');
+
+        var item = {
+            elem: elem,
+            title: title,
+            date_html: date_html,
+            date_text: date_text,
+            need_frb: need_frb,
+        };
+
+        return item;
+    });
+
+    items[0].show_date = true;
+
+    for (var i = 1; i < items.length; i += 1) {
+        items[i].show_date = items[i].date_text !== items[i - 1].date_text;
+    }
+
+    _.each(items, function (item) {
+        if (!item.need_frb) {
+            delete item.date_html;
+        }
+    });
+
+    return items;
+};
+
+exports.candidate_recipient_bar = function () {
+    // Find a recipient bar that is close to being onscreen
+    // but above the "top".  This function is guaranteed to
+    // return **some** recipient bar that is above the fold,
+    // if there is one, but it may not be the optimal one if
+    // our pointer is messed up.  Starting with the pointer
+    // is just an optimization here, and our caller will do
+    // a forward traversal and clean up as necessary.
+    // In most cases we find the bottom-most of recipient
+    // bars that is still above the fold.
 
     // Start with the pointer's current location.
     var selected_row = current_msg_list.selected_row();
@@ -29,14 +242,14 @@ exports.obscured_recipient_bar = function () {
         return;
     }
 
-    var floating_recipient_bar_bottom = exports.frb_bottom();
-
     while (candidate.length) {
-        if (candidate.is(".recipient_row")) {
+
-            if (candidate.offset().top < floating_recipient_bar_bottom) {
+        if (candidate.hasClass("recipient_row") && top_offset(candidate) < 0) {
-                return candidate;
+            return candidate;
-            }
         }
+        // We cannot use .prev(".recipient_row") here, because that
+        // returns nothing if the previous element is not a recipient
+        // row, rather than finding the first recipient_row.
         candidate = candidate.prev();
     }
 };
@@ -49,10 +262,14 @@ function show_floating_recipient_bar() {
 }
 
 var old_source;
-function replace_floating_recipient_bar(source_recipient_bar) {
+function replace_floating_recipient_bar(source_info) {
+
+    var source_recipient_bar = source_info.elem;
+
     var new_label;
     var other_label;
     var header;
+
     if (source_recipient_bar !== old_source) {
         if (source_recipient_bar.children(".message_header_stream").length !== 0) {
             new_label = $("#current_label_stream");
@@ -71,6 +288,11 @@ function replace_floating_recipient_bar(source_recipient_bar) {
         new_label.toggleClass('message-fade', source_recipient_bar.hasClass('message-fade'));
         old_source = source_recipient_bar;
     }
+
+    var rendered_date = source_info.date_html || '';
+
+    $('#floating_recipient_bar').find('.recipient_row_date').html(rendered_date);
+
     show_floating_recipient_bar();
 }
 
@@ -81,39 +303,28 @@ exports.hide = function () {
     }
 };
 
+exports.de_clutter_dates = function (items) {
+    _.each(items, function (item) {
+        item.elem.find('.recipient_row_date').toggle(item.show_date);
+    });
+};
+
 exports.update = function () {
-    // .temp-show-date might be forcing the display of a recipient_row_date if
+    var items = exports.relevant_recipient_bars();
-    // the floating_recipient_bar is just beginning to overlap the
-    // top-most recipient_bar. remove all instances of .temp-show-date and
-    // re-apply it if we continue to detect overlap
-    $('.temp-show-date').removeClass('temp-show-date');
 
-    var floating_recipient_bar_bottom = exports.frb_bottom();
+    if (!items || items.length === 0) {
-
-    var source_recipient_bar = exports.obscured_recipient_bar();
-
-    if (!source_recipient_bar) {
         exports.hide();
         return;
     }
 
-    // We now know what the floating stream/topic bar should say.
+    exports.de_clutter_dates(items);
-    // Do we show it?
 
-    // Hide if the bottom of our floating stream/topic label is not
+    if (!items[0].need_frb) {
-    // lower than the bottom of source_recipient_bar (since that means we're
-    // covering up a label that already exists).
-    var header_height = $(source_recipient_bar).find('.message_header').safeOuterHeight();
-    if (floating_recipient_bar_bottom <=
-        source_recipient_bar.offset().top + header_height) {
-        // hide floating_recipient_bar and use .temp-show-date to force display
-        // of the recipient_row_date belonging to the current recipient_bar
-        $('.recipient_row_date', source_recipient_bar).addClass('temp-show-date');
         exports.hide();
         return;
     }
 
-    replace_floating_recipient_bar(source_recipient_bar);
+    replace_floating_recipient_bar(items[0]);
 };
 
 

static/js/message_list_view.js

@@ -107,6 +107,31 @@ function clear_group_date_divider(group) {
     group.group_date_divider_html = undefined;
 }
 
+function clear_message_date_divider(msg) {
+    // see update_message_date_divider for how
+    // these get set
+    msg.want_date_divider = false;
+    msg.date_divider_html = undefined;
+}
+
+function update_message_date_divider(opts) {
+    var prev_msg_container = opts.prev_msg_container;
+    var curr_msg_container = opts.curr_msg_container;
+
+    if (!prev_msg_container || same_day(curr_msg_container, prev_msg_container)) {
+        clear_message_date_divider(curr_msg_container);
+        return;
+    }
+
+    var prev_time = new XDate(prev_msg_container.msg.timestamp * 1000);
+    var curr_time = new XDate(curr_msg_container.msg.timestamp * 1000);
+    var today = new XDate();
+
+    curr_msg_container.want_date_divider = true;
+    curr_msg_container.date_divider_html =
+        timerender.render_date(curr_time, prev_time, today)[0].outerHTML;
+}
+
 function set_timestr(message_container) {
     var time = new XDate(message_container.msg.timestamp * 1000);
     message_container.timestr = timerender.stringify_time(time);
@@ -239,15 +264,19 @@ MessageListView.prototype = {
             message_container.include_footer    = false;
 
             if (same_recipient(prev, message_container) && self.collapse_messages &&
-                prev.msg.historical === message_container.msg.historical &&
+                prev.msg.historical === message_container.msg.historical) {
-                same_day(prev, message_container)) {
                 add_message_container_to_group(message_container);
+                update_message_date_divider({
+                    prev_msg_container: prev,
+                    curr_msg_container: message_container,
+                });
             } else {
                 finish_group();
                 current_group = start_group();
                 add_message_container_to_group(message_container);
 
                 update_group_date_divider(current_group, message_container, prev);
+                clear_message_date_divider(message_container);
 
                 message_container.include_recipient = true;
                 message_container.subscribed = false;
@@ -279,6 +308,7 @@ MessageListView.prototype = {
             message_container.include_sender = true;
             if (!message_container.include_recipient &&
                 !prev.status_message &&
+                same_day(prev, message_container) &&
                 same_sender(prev, message_container)) {
                 message_container.include_sender = false;
             }
@@ -307,10 +337,10 @@ MessageListView.prototype = {
 
     join_message_groups: function (first_group, second_group) {
         // join_message_groups will combine groups if they have the
-        // same_recipient on the same_day and the view supports collapsing
+        // same_recipient and the view supports collapsing, otherwise
-        // otherwise it may add a subscription_marker if required.
+        // it may add a subscription_marker if required.  It returns
-        // It returns true if the two groups were joined in to one and
+        // true if the two groups were joined in to one and the
-        // the second_group should be ignored.
+        // second_group should be ignored.
         if (first_group === undefined || second_group === undefined) {
             return false;
         }
@@ -319,9 +349,9 @@ MessageListView.prototype = {
 
         // Join two groups into one.
         if (this.collapse_messages && same_recipient(last_msg_container, first_msg_container) &&
-            same_day(last_msg_container, first_msg_container) &&
             last_msg_container.msg.historical === first_msg_container.msg.historical) {
             if (!last_msg_container.status_message && !first_msg_container.msg.is_me_message
+                && same_day(last_msg_container, first_msg_container)
                 && same_sender(last_msg_container, first_msg_container)) {
                 first_msg_container.include_sender = false;
             }
@@ -382,6 +412,14 @@ MessageListView.prototype = {
         }
 
         var was_joined = this.join_message_groups(first_group, second_group);
+        if (was_joined) {
+            update_message_date_divider({
+                prev_msg_container: prev_msg_container,
+                curr_msg_container: curr_msg_container,
+            });
+        } else {
+            clear_message_date_divider(curr_msg_container);
+        }
 
         if (where === 'top') {
             if (was_joined) {

static/styles/zulip.scss

@@ -845,14 +845,6 @@ td.pointer {
     display: none;
 }
 
-/*
-   used to override .hide-date when floating_recipient_bar just begins to
-   overlap the top-most recipient_bar
-*/
-.recipient_row_date.temp-show-date {
-    display: block !important;
-}
-
 .floating_recipient .recipient_row_date.hide-date {
     display: block;
 }
@@ -1063,7 +1055,7 @@ td.pointer {
     border-right: 1px solid hsl(0, 0%, 88%);
 }
 
-.mention .messagebox {
+.mention .messagebox-content {
     background-color: hsl(8, 94%, 94%);
 }
 

static/templates/single_message.handlebars

@@ -4,6 +4,9 @@
     <div class="unread_marker"><div class="unread-marker-fill"></div></div>
     <div class="messagebox{{^include_sender}} prev_is_same_sender{{/include_sender}}{{^msg/is_stream}} private-message{{/msg/is_stream}} {{#if next_is_same_sender}}next_is_same_sender{{/if}}"
       style="box-shadow: inset 2px 0px 0px 0px {{#if msg/is_stream}}{{background_color}}{{else}}#444444{{/if}}, -1px 0px 0px 0px {{#if msg/is_stream}}{{background_color}}{{else}}#444444{{/if}};">
+        {{#if want_date_divider}}
+        <div class="date_row no-select">{{{date_divider_html}}}</div>
+        {{/if}}
         <div class="messagebox-border">
             <div class="messagebox-content">
                 <div class="message_top_line">