ErinCall
commented
8 years ago Oh, I meant to mention, I've tried bouncing the slack client a couple of times.
Closed ErinCall closed 8 years ago
ErinCall
commented
8 years ago Oh, I meant to mention, I've tried bouncing the slack client a couple of times.
bhuga
commented
8 years ago I've noticed this too; it's something Slack changed. Thanks for writing it down and forcing me to think about it; I'll poke at it tonight.
bhuga
commented
8 years ago Dropping some notes.
TS.client.msg_pane used to do scrolling after it rendered, but now it does scrolling[1] as part of a 212 line monstrosity of a method, rebuildMsgs. So the tl;dr is that it's scrolling to the bottom of rooms before our hacks are applied. This method is a real mess, and I can of course run something right after or before it, but there's something like 12 branches for where it might want to scroll to based on history visibility, message unreadness, and so forth, and I'm not sure how to replicate the decision with the new room size.
Efforts continue.
rebuildMsgs: function(call_again, reason) {
var model_ob = TS.shared.getActiveModelOb();
if (!model_ob) {
return
}
var all_unknowns = [];
if (typeof call_again === "undefined") call_again = true;
var edit_state;
if (TS.msg_edit.editing && TS.msg_edit.editing_in_msg_pane) edit_state = TS.msg_edit.pauseEditing();
TS.log(5, "rebuilding msgs for " + TS.model.active_cid);
TS.model.ui.msgs_are_auto_scrolling = false;
if (TS.pri) TS.log(999, "rebuildMsgs(), #" + model_ob.name + ", reason: " + (reason || "not specified"));
if (TS.pri) TS.log(999, "rebuildMsgs(), #" + model_ob.name + ": calling TS.client.msg_pane.clearUnreadDivider()");
TS.client.msg_pane.clearUnreadDivider();
var start_mark = "start_message_render";
TS.metrics.mark(start_mark);
var msgs;
var last_scroll_top = -1;
var html = "";
TS.client.msg_pane.last_render_time = new Date;
TS.client.msg_pane.last_rendered_msg = null;
TS.client.msg_pane.last_in_stream_msg = null;
last_scroll_top = model_ob.scroll_top;
msgs = model_ob.msgs;
var msg;
var prev_msg;
var reset_scroll_for_pending_history;
if (TS.replies && TS.replies.isReplyHidingEnabled()) {
TS.replies.rollUpReplies(msgs)
}
if (TS.boot_data && TS.boot_data.feature_js_raf_queue) {
TS.client.ui.$msgs_div.find(".day_divider").remove()
} else {
TS.client.ui.$msgs_div.empty()
}
if (model_ob._cached_html) {
TS.info("using _cached_html");
html = model_ob._cached_html;
model_ob._cached_html = null
} else if (!msgs) {
html = "-"
} else {
if (!msgs.length) {
html = ""
}
var jl_rolled_up_msgs = [];
var count = 0;
for (var i = msgs.length - 1; i > -1; i--) {
if (!msg || !TS.utility.msgs.isMsgHidden(msg)) prev_msg = msg;
msg = msgs[i];
if (TS.utility.msgs.isMsgHidden(msg)) continue;
var rollup = TS.utility.msgs.msgRollUpWorker(i, msg, msgs, jl_rolled_up_msgs);
if (rollup == "continue") {
msg = prev_msg;
continue
} else if (rollup == "swap") {
msg = jl_rolled_up_msgs[0];
jl_rolled_up_msgs.length = 0
}
count++;
var any_unknowns;
if (TS.boot_data.feature_shared_channels_ui && TS.boot_data.page_needs_enterprise && call_again) {
any_unknowns = TS.utility.msgs.allUnknownUsersInMessage(msg);
if (any_unknowns.length) {
any_unknowns.forEach(function(id) {
if (all_unknowns.indexOf(id) < 0) all_unknowns.push(id)
})
}
}
var not_same_day_as_prev_msg = _ifNotSameDayAsLastMsg(msg, prev_msg);
if (not_same_day_as_prev_msg) {
if (prev_msg) html += "</div></div>";
html += '<div class="day_container">';
html += TS.templates.messages_day_divider({
ts: msg.ts
});
html += '<div class="day_msgs" data-date="' + TS.utility.date.toCalendarDate(msg.ts) + '" data-ts="' + msg.ts + '">'
}
html += TS.templates.builders.buildMsgHTML({
msg: msg,
model_ob: model_ob,
prev_msg: prev_msg,
container_id: "msgs_div",
enable_slack_action_links: true
});
if (i === 0) {
html += "</div></div>"
}
if (!msg.rsp_id) {
TS.client.msg_pane.last_in_stream_msg = msg;
if (!TS.utility.msgs.isMsgHidden(msg)) TS.client.msg_pane.last_rendered_msg = msg
}
}
var min_count = 20;
if (!TS.isPartiallyBooted() && msgs.length && count < min_count) {
if (TS.pri) TS.log(888, "rebuildMsgs(): loading more history because count is < " + min_count);
if (TS.boot_data.feature_perf_defer_initial_msg_check && TS.model.prefs.start_scroll_at_oldest && !model_ob.is_im) {
if (TS.pri) TS.log(888, "rebuildMsgs(): loading scrollback, AND, resetting _has_auto_scrolled so we scroll to oldest again when more history loads.");
reset_scroll_for_pending_history = true
}
TS.client.ui.doLoadScrollBackHistory(true)
}
}
TS.client.ui.$msgs_div.html(html);
TS.metrics.measure("message_render", start_mark);
TS.metrics.clearMarks(start_mark);
TS.utility.makeSureAllLinksHaveTargets(TS.client.ui.$msgs_div);
TS.client.msg_pane.assignLastReadMsgDiv(model_ob);
TS.client.msg_pane.insertUnreadDivider();
TS.client.msg_pane.updateEndMarker();
if (TS.boot_data.feature_perf_defer_initial_msg_check) {
TS.client.ui.checkInlineImgsAndIframesMain()
}
TS.client.msg_pane.padOutMsgsScroller();
if (last_scroll_top == -1 || last_scroll_top === undefined || TS.model.prefs && TS.model.prefs.start_scroll_at_oldest && model_ob.unread_cnt) {
var start_at_oldest = TS.model.prefs && TS.model.prefs.start_scroll_at_oldest;
if (TS.boot_data.feature_perf_defer_initial_msg_check && start_at_oldest && model_ob.unread_cnt && TS.client.msg_pane.new_msgs_bar_showing && !model_ob._has_auto_scrolled) {
if (TS.pri) TS.log(888, "rebuildMsgs(): resetting has_auto_scrolled on #" + model_ob.name + " and forcing first unread to scroll into view, because new msgs bar is still up and unread_cnt = " + model_ob.unread_cnt);
last_scroll_top = -1;
_resetHasAutoScrolled(model_ob)
}
if (!TS.boot_data.feature_perf_defer_initial_msg_check || !model_ob._has_auto_scrolled || !TS.client.ui.isUserAttentionOnChat()) {
if (TS.pri) TS.log(888, "rebuildMsgs(): insta-scrolling to bottom(false) and then first unread into view because of last_scroll_top = -1 or undefined (actual: " + last_scroll_top + ") OR (pref to scroll AND unread), where unread_cnt = " + model_ob.unread_cnt); /* ###### this is the scroll branch that we run for rooms where we're "at the end". At this point, scrollHeight is too small on the messages div. */
TS.utility.queueRAF(function instaScrollMsgsToBottomRAF() {
TS.client.ui.instaScrollMsgsToBottom(false)
});
if (TS.model.prefs && TS.model.prefs.start_scroll_at_oldest) {
TS.client.ui.scrollMsgsSoFirstUnreadMsgIsInView(function() {})
}
if (TS.boot_data.feature_perf_defer_initial_msg_check && !model_ob.is_im && !reset_scroll_for_pending_history) {
(function(local_model_ob) {
TS.utility.rAF(function() {
var current_model_ob = TS.shared.getActiveModelOb();
if (!current_model_ob || current_model_ob.id !== local_model_ob.id) return;
if (TS.model.prefs && TS.model.prefs.start_scroll_at_oldest) {
TS.client.ui.scrollMsgsSoFirstUnreadMsgIsInView(function() {})
}
if (local_model_ob.msgs.length) {
if (TS.pri) TS.log(888, "rebuildMsgs(): marking has_auto_scrolled because !is_im and has msgs");
local_model_ob._has_auto_scrolled = true
} else {
if (TS.pri) TS.log(888, "rebuildMsgs(): NOT marking has_auto_scrolled because no msgs yet");
local_model_ob._has_auto_scrolled = false
}
local_model_ob = null
})
})(model_ob)
}
}
} else {
if (TS.pri) TS.log(888, "rebuildMsgs(): insta-scrolling to last_scroll_top of " + last_scroll_top);
TS.client.ui.instaScrollMsgsToPosition(last_scroll_top, false)
}
if (reset_scroll_for_pending_history && model_ob._has_auto_scrolled) {
if (TS.pri) TS.log(888, "rebuildMsgs(): resetting scroll state due to pending history call");
last_scroll_top = -1;
_resetHasAutoScrolled(model_ob)
}
if (!TS.boot_data.feature_perf_defer_initial_msg_check) {
TS.utility.queueRAF(TS.client.ui.checkInlineImgsAndIframesMain)
}
if (edit_state) TS.msg_edit.resumeEditing(edit_state);
if (TS.boot_data.feature_message_replies && msgs && !TS.replies.isReplyHidingEnabled()) {
TS.replies.getThreadsForMissingParents(model_ob, msgs)
}
if (TS.boot_data.feature_shared_channels_ui && TS.boot_data.page_needs_enterprise && call_again) {
if (all_unknowns.length) {
var or_clause = {
type: "or",
clauses: []
};
all_unknowns.forEach(function(id) {
or_clause.clauses.push({
type: "id",
value: id
})
});
var calling_args = {
query: JSON.stringify(or_clause)
};
var current_model_ob_id = model_ob.id;
TS.api.callImmediately("search.enterprise", calling_args).then(function(response) {
if (!response.data.items.length) return null;
response.data.items.map(function(member) {
var full_member = TS.members.getMemberById(member.id);
if (!full_member) {
if (member.team_id !== TS.model.team.id) {
member.is_primary_owner = false;
member.is_owner = false;
member.is_admin = false
}
full_member = TS.members.upsertMember(member).member
}
return full_member
}).filter(function(member) {
if (member.is_self || member.is_slackbot) {
return false
}
return true
});
if (current_model_ob_id === TS.shared.getActiveModelOb().id) TS.client.msg_pane.rebuildMsgs(false, "search.enterprise called");
return null
}).catch(function(error) {
TS.error(error.data.error + " error occured while searching");
TS.generic_dialog.alert("Sorry! Something went wrong. Please try again.");
throw error
})
}
}
html = null;
msgs = null;
model_ob = null
},[1] Perhaps it always did, and now for some reason our css makes rooms bigger than the native? This feels unlikely to me, but we did just mess with image sizing, so...
bhuga
commented
8 years ago A little more, since people are bothered by this:
I hooked into the metrics call after the HTML is updated and added our hacks. But adding the hacks doesn't change the height of the messages area; I'm thinking the browser hasn't done a repaint yet. I "fixed" it by adding a 100ms delay to the scrolling, but it's jerky, by that time slack has set the scroll_top variable of the room's model object. I guess what we want to happen is:
The second part is the hardest part (though the first one is a trick, too).
bhuga
commented
8 years ago More notes.
The problem is that slack enabled a feature for our team (TS.boot_data.feature_attachments_makeover). This changed how attachments are drawn. In particular it hardcodes a height, and the scroller now seems to demand this.
I have found the template that renders inline attachments, and have experimented with gsubbing out the hardcoded css and replacing it with other stuff. If there's a hardcoded height, things work. If there's not, the scrolling bug.
So we have an image in there and we are telling CSS the width stuff it will need to know to pick a height, so why does it need a height? I think because when the template renders, there is not yet an image src. For some reason there is this thing called 'hidden_by_default', set as !!TS.client (true), which sets data-real-src instead of src. By the time I inspect it, this has been copied to src.Further investigations ongoing.
cc @zerowidth who asked about this today
bhuga
commented
8 years ago I managed to get the src to come out of my hacked-over-template thing, but that didn't quite work either. I really think I'm going to need to repaint after building. All of this work has given me a plan though:
I'll try that tomorrow. That's enough for today.
bhuga
commented
8 years ago A few more hours into this one. It's a real doozy :frowning:
The function that causes the scroller div height change is TS.client.ui.checkInlineImgsAndIframesMain. For some reason, slack inserts all media with a hidden field and then this function marks them not-hidden after the scrolling happens. With a hardcoded height this works fine, but that's what we want to avoid.
I've tried a few things, including removing the hidden-ness from inline images when the template is rendered. This feels right, but results in a nondeterministic height when changing rooms, always fixed by the time the room is done loading. Paint randomness, or something?
bhuga
commented
8 years ago https://github.com/bhuga/slacks-hacks/pull/11 takes a stab at fixing this. I send my custom built signal in an override of slack's function that checks inline images and un-hides them. This seems to solve the problem, but sometimes things still don't scroll when new messages come in that are higher than slack's grace-pixels (50). No solution there yet.
bhuga
commented
8 years ago I had to revert that. After the window was open a few minutes, there was weirdness where rooms would jump way, way up.
https://github.com/bhuga/slacks-hacks/pull/12 is another attempt. It's a lot less code so I think the potential impact is more limited. We'll see.
bhuga
commented
8 years ago This hasn't come back 😅
When I switch to one of a select few channels, the view appears scrolled up a little bit from the bottom of the messages. Here's a gif:
I don't see this happen with the vanilla Slack client.