From 19b7ef3888cdb92cf6aaa5482eff796ec40e6aef Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Sat, 4 Jul 2020 13:37:43 +0530 Subject: [PATCH] list_render: Use simplebar container to track scroll event. We have changed our all instances of list_render to use simplebar and thus, we will now use simplebar container to track scroll event for all the lists created by list_render. This fixes the bug of new subscribers not rendering on scrolling at the end of subscriber list in stream settings and similar bug in some other lists also. This commit also removes scroll_util.get_list_scrolling_container function as this is no longer used. Fixes #15637. --- frontend_tests/node_tests/list_render.js | 62 ++++++++++++++++-------- frontend_tests/node_tests/scroll_util.js | 18 ------- static/js/attachments_ui.js | 1 + static/js/dropdown_list_widget.js | 1 + static/js/list_render.js | 6 ++- static/js/muting_ui.js | 1 + static/js/recent_topics.js | 1 + static/js/scroll_util.js | 27 ----------- static/js/settings_emoji.js | 1 + static/js/settings_exports.js | 1 + static/js/settings_invites.js | 1 + static/js/settings_linkifiers.js | 1 + static/js/settings_streams.js | 1 + static/js/settings_users.js | 3 ++ static/js/stream_edit.js | 1 + 15 files changed, 60 insertions(+), 66 deletions(-) diff --git a/frontend_tests/node_tests/list_render.js b/frontend_tests/node_tests/list_render.js index 145e86ea58..93c30890a5 100644 --- a/frontend_tests/node_tests/list_render.js +++ b/frontend_tests/node_tests/list_render.js @@ -1,4 +1,3 @@ -zrequire('scroll_util'); zrequire('list_render'); // We need these stubs to get by instanceof checks. @@ -10,6 +9,7 @@ function Element() { return { }; } set_global('Element', Element); +set_global('ui', {}); // We only need very simple jQuery wrappers for when the // "real" code wraps html or sets up click handlers. @@ -54,14 +54,8 @@ function make_container() { return container; } -function make_scroll_container(container) { +function make_scroll_container() { const scroll_container = {}; - scroll_container.is = () => false; - scroll_container.length = () => 1; - scroll_container.css = (prop) => { - assert.equal(prop, 'max-height'); - return 100; - }; scroll_container.cleared = false; @@ -79,8 +73,6 @@ function make_scroll_container(container) { scroll_container.cleared = true; }; - container.parent = () => scroll_container; - return scroll_container; } @@ -147,16 +139,23 @@ function div(item) { run_test('scrolling', () => { const container = make_container(); - const scroll_container = make_scroll_container(container); + const scroll_container = make_scroll_container(); const items = []; + let get_scroll_element_called = false; + ui.get_scroll_element = (element) => { + get_scroll_element_called = true; + return element; + }; + for (let i = 0; i < 200; i += 1) { items.push('item ' + i); } const opts = { modifier: (item) => item, + simplebar_container: scroll_container, }; container.html = (html) => { assert.equal(html, ''); }; @@ -166,6 +165,7 @@ run_test('scrolling', () => { container.appended_data.html(), items.slice(0, 80).join(''), ); + assert.equal(get_scroll_element_called, true); // Set up our fake geometry so it forces a scroll action. scroll_container.scrollTop = 180; @@ -183,7 +183,7 @@ run_test('scrolling', () => { run_test('filtering', () => { const container = make_container(); - make_scroll_container(container); + const scroll_container = make_scroll_container(); const search_input = make_search_input(); @@ -202,6 +202,7 @@ run_test('filtering', () => { predicate: (item, value) => item.includes(value), }, modifier: (item) => div(item), + simplebar_container: scroll_container, }; container.html = (html) => { assert.equal(html, ''); }; @@ -246,12 +247,13 @@ run_test('filtering', () => { run_test('no filtering', () => { const container = make_container(); - make_scroll_container(container); + const scroll_container = make_scroll_container(); container.html = () => {}; // Opts does not require a filter key. const opts = { modifier: (item) => div(item), + simplebar_container: scroll_container, }; const widget = list_render.create(container, ['apple', 'banana'], opts); widget.render(); @@ -321,7 +323,7 @@ run_test('wire up filter element', () => { ]; const container = make_container(); - make_scroll_container(container); + const scroll_container = make_scroll_container(); const filter_element = make_filter_element(); // We don't care about what gets drawn initially. @@ -333,6 +335,7 @@ run_test('wire up filter element', () => { element: filter_element, }, modifier: (s) => '(' + s + ')', + simplebar_container: scroll_container, }; list_render.create(container, lst, opts); @@ -345,7 +348,7 @@ run_test('wire up filter element', () => { run_test('sorting', () => { const container = make_container(); - make_scroll_container(container); + const scroll_container = make_scroll_container(); const sort_container = make_sort_container(); let cleared; @@ -369,6 +372,7 @@ run_test('sorting', () => { filter: { predicate: () => true, }, + simplebar_container: scroll_container, }; function html_for(people) { @@ -476,7 +480,7 @@ run_test('sorting', () => { run_test('custom sort', () => { const container = make_container(); - make_scroll_container(container); + const scroll_container = make_scroll_container(); container.html = () => {}; const n42 = {x: 6, y: 7}; @@ -501,6 +505,7 @@ run_test('custom sort', () => { x_value: sort_by_x, }, init_sort: sort_by_product, + simplebar_container: scroll_container, }); assert.deepEqual( @@ -530,7 +535,7 @@ run_test('custom sort', () => { run_test('clear_event_handlers', () => { const container = make_container(); - const scroll_container = make_scroll_container(container); + const scroll_container = make_scroll_container(); const sort_container = make_sort_container(); const filter_element = make_filter_element(); @@ -546,6 +551,7 @@ run_test('clear_event_handlers', () => { element: filter_element, predicate: () => true, }, + simplebar_container: scroll_container, }; // Create it the first time. @@ -565,15 +571,22 @@ run_test('errors', () => { // We don't care about actual data for this test. const list = 'stub'; const container = make_container(); - make_scroll_container(container); + const scroll_container = make_scroll_container(); blueslip.expect('error', 'Need opts to create widget.'); list_render.create(container, list); blueslip.reset(); + blueslip.expect('error', 'simplebar_container is missing.'); + list_render.create(container, list, { + modifier: 'hello world', + }); + blueslip.reset(); + blueslip.expect('error', 'get_item should be a function'); list_render.create(container, list, { get_item: 'not a function', + simplebar_container: scroll_container, }); blueslip.reset(); @@ -582,6 +595,7 @@ run_test('errors', () => { filter: { predicate: 'wrong type', }, + simplebar_container: scroll_container, }); blueslip.reset(); @@ -591,6 +605,7 @@ run_test('errors', () => { filterer: () => true, predicate: () => true, }, + simplebar_container: scroll_container, }); blueslip.reset(); @@ -598,6 +613,7 @@ run_test('errors', () => { list_render.create(container, list, { filter: { }, + simplebar_container: scroll_container, }); blueslip.reset(); @@ -605,6 +621,7 @@ run_test('errors', () => { blueslip.expect('error', 'List item is not a string: 999'); list_render.create(container, list, { modifier: () => 999, + simplebar_container: scroll_container, }); blueslip.reset(); }); @@ -633,7 +650,7 @@ run_test('sort helpers', () => { run_test('replace_list_data w/filter update', () => { const container = make_container(); - make_scroll_container(container); + const scroll_container = make_scroll_container(); container.html = () => {}; const list = [1, 2, 3, 4]; @@ -648,6 +665,7 @@ run_test('replace_list_data w/filter update', () => { num_updates += 1; }, }, + simplebar_container: scroll_container, }); assert.equal(num_updates, 0); @@ -722,7 +740,7 @@ run_test('opts.get_item', () => { run_test('render item', () => { const container = make_container(); - const scroll_container = make_scroll_container(container); + const scroll_container = make_scroll_container(); const INITIAL_RENDER_COUNT = 80; // Keep this in sync with the actual code. container.html = () => {}; let called = false; @@ -758,6 +776,7 @@ run_test('render item', () => { modifier: (item) => `${item.text}\n`, get_item: get_item, html_selector: (item) => `tr[data-item='${item}']`, + simplebar_container: scroll_container, }); const item = INITIAL_RENDER_COUNT - 1; @@ -786,6 +805,7 @@ run_test('render item', () => { modifier: (item) => `${item.text}\n`, get_item: get_item, html_selector: 'hello world', + simplebar_container: scroll_container, }); blueslip.reset(); @@ -797,6 +817,7 @@ run_test('render item', () => { get_item_called = true; return item; }, + simplebar_container: scroll_container, }); get_item_called = false; widget_2.render_item(item); @@ -809,6 +830,7 @@ run_test('render item', () => { modifier: (item) => rendering_item ? undefined : `${item}\n`, get_item: get_item, html_selector: (item) => `tr[data-item='${item}']`, + simplebar_container: scroll_container, }); // Once we have initially rendered the widget, change the // behavior of the modifier function. diff --git a/frontend_tests/node_tests/scroll_util.js b/frontend_tests/node_tests/scroll_util.js index 782025203a..591ef8af2d 100644 --- a/frontend_tests/node_tests/scroll_util.js +++ b/frontend_tests/node_tests/scroll_util.js @@ -89,21 +89,3 @@ run_test('scroll_element_into_container', () => { scroll_util.scroll_element_into_container(elem2, container); assert.equal(container.scrollTop(), 250 - 100 + 3 + 15); }); - -run_test('get_list_scrolling_container error', () => { - const body = { - length: 1, - is: (sel) => { - assert.equal(sel, 'body, html'); - return true; - }, - }; - - blueslip.expect( - 'error', - "Please wrap lists in an element with " + - "'max-height' attribute.", - ); - - scroll_util.get_list_scrolling_container(body); -}); diff --git a/static/js/attachments_ui.js b/static/js/attachments_ui.js index 6f00820685..75483e22b8 100644 --- a/static/js/attachments_ui.js +++ b/static/js/attachments_ui.js @@ -96,6 +96,7 @@ function render_attachments_ui() { sort_fields: { mentioned_in: sort_mentioned_in, }, + simplebar_container: $('#attachments-settings .progressive-table-wrapper'), }); ui.reset_scrollbar(uploaded_files_table.closest(".progressive-table-wrapper")); diff --git a/static/js/dropdown_list_widget.js b/static/js/dropdown_list_widget.js index bf7862f6f0..aa7e2d0159 100644 --- a/static/js/dropdown_list_widget.js +++ b/static/js/dropdown_list_widget.js @@ -78,6 +78,7 @@ const DropdownListWidget = function (opts) { return item.name.toLowerCase().includes(value); }, }, + simplebar_container: $(`#${opts.container_id} .dropdown-list-wrapper`), }); $(`#${opts.container_id} .dropdown-search`).click((e) => { e.stopPropagation(); diff --git a/static/js/list_render.js b/static/js/list_render.js index 1246ea7477..014b9323e1 100644 --- a/static/js/list_render.js +++ b/static/js/list_render.js @@ -17,6 +17,10 @@ exports.validate_opts = (opts) => { blueslip.error('html_selector should be a function.'); return false; } + if (!opts.simplebar_container) { + blueslip.error('simplebar_container is missing.'); + return false; + } return true; }; @@ -264,7 +268,7 @@ exports.create = function ($container, list, opts) { }; widget.set_up_event_handlers = function () { - meta.scroll_container = scroll_util.get_list_scrolling_container($container); + meta.scroll_container = ui.get_scroll_element(opts.simplebar_container); // on scroll of the nearest scrolling container, if it hits the bottom // of the container then fetch a new block of items and render them. diff --git a/static/js/muting_ui.js b/static/js/muting_ui.js index 6eabf500ea..9bfac827d3 100644 --- a/static/js/muting_ui.js +++ b/static/js/muting_ui.js @@ -90,6 +90,7 @@ exports.set_up_muted_topics_ui = function () { }, }, parent_container: $('#muted-topic-settings'), + simplebar_container: $('#muted-topic-settings .progressive-table-wrapper'), }); }; diff --git a/static/js/recent_topics.js b/static/js/recent_topics.js index 4c722fd446..9f1820467e 100644 --- a/static/js/recent_topics.js +++ b/static/js/recent_topics.js @@ -419,6 +419,7 @@ exports.complete_rerender = function () { topic_sort: topic_sort, }, html_selector: get_topic_row, + simplebar_container: $('#recent_topics_table .table_fix_head'), }); revive_current_focus(); }; diff --git a/static/js/scroll_util.js b/static/js/scroll_util.js index 341c57c9b8..37cdd9f236 100644 --- a/static/js/scroll_util.js +++ b/static/js/scroll_util.js @@ -1,30 +1,3 @@ -exports.get_list_scrolling_container = function (container) { - /* - This is used for list widgets that don't - have SimpleBar (in contrast to, say, - ui.get_scroll_element(). - */ - - let nearestScrollingContainer = container; - - while (nearestScrollingContainer.length) { - if (nearestScrollingContainer.is("body, html")) { - blueslip.error( - "Please wrap lists in an element with " + - "'max-height' attribute."); - break; - } - - if (nearestScrollingContainer.css("max-height") !== "none") { - break; - } - - nearestScrollingContainer = nearestScrollingContainer.parent(); - } - - return nearestScrollingContainer; -}; - exports.scroll_delta = function (opts) { const elem_top = opts.elem_top; const container_height = opts.container_height; diff --git a/static/js/settings_emoji.js b/static/js/settings_emoji.js index cbdeb28a0a..1bfd930000 100644 --- a/static/js/settings_emoji.js +++ b/static/js/settings_emoji.js @@ -106,6 +106,7 @@ exports.populate_emoji = function (emoji_data) { author_full_name: sort_author_full_name, }, init_sort: ['alphabetic', 'name'], + simplebar_container: $('#emoji-settings .progressive-table-wrapper'), }); loading.destroy_indicator($('#admin_page_emoji_loading_indicator')); diff --git a/static/js/settings_exports.js b/static/js/settings_exports.js index 1113771f85..7f90a2f798 100644 --- a/static/js/settings_exports.js +++ b/static/js/settings_exports.js @@ -72,6 +72,7 @@ exports.populate_exports_table = function (exports) { sort_fields: { user: sort_user, }, + simplebar_container: $('#data-exports .progressive-table-wrapper'), }); const spinner = $('.export_row .export_url_spinner'); diff --git a/static/js/settings_invites.js b/static/js/settings_invites.js index 13762e9458..7690b4853a 100644 --- a/static/js/settings_invites.js +++ b/static/js/settings_invites.js @@ -68,6 +68,7 @@ function populate_invites(invites_data) { sort_fields: { invitee: sort_invitee, }, + simplebar_container: $('#admin-invites-list .progressive-table-wrapper'), }); loading.destroy_indicator($('#admin_page_invites_loading_indicator')); diff --git a/static/js/settings_linkifiers.js b/static/js/settings_linkifiers.js index a12325b495..48aee7a56b 100644 --- a/static/js/settings_linkifiers.js +++ b/static/js/settings_linkifiers.js @@ -65,6 +65,7 @@ exports.populate_filters = function (filters_data) { pattern: sort_pattern, url: sort_url, }, + simplebar_container: $('#filter-settings .progressive-table-wrapper'), }); loading.destroy_indicator($('#admin_page_filters_loading_indicator')); diff --git a/static/js/settings_streams.js b/static/js/settings_streams.js index 77521a676c..fc771b892b 100644 --- a/static/js/settings_streams.js +++ b/static/js/settings_streams.js @@ -42,6 +42,7 @@ exports.build_default_stream_table = function () { }, parent_container: $("#admin-default-streams-list").expectOne(), init_sort: ['alphabetic', 'name'], + simplebar_container: $('#admin-default-streams-list .progressive-table-wrapper'), }); loading.destroy_indicator($('#admin_page_default_streams_loading_indicator')); diff --git a/static/js/settings_users.js b/static/js/settings_users.js index cfbb9315e5..023230729d 100644 --- a/static/js/settings_users.js +++ b/static/js/settings_users.js @@ -271,6 +271,7 @@ section.bots.create_table = () => { email: sort_bot_email, bot_owner: sort_bot_owner, }, + simplebar_container: $('#admin-bot-list .progressive-table-wrapper'), }); loading.destroy_indicator($('#admin_page_bots_loading_indicator')); @@ -298,6 +299,7 @@ section.active.create_table = (active_users) => { last_active: sort_last_active, role: sort_role, }, + simplebar_container: $('#admin-user-list .progressive-table-wrapper'), }); loading.destroy_indicator($('#admin_page_users_loading_indicator')); @@ -324,6 +326,7 @@ section.deactivated.create_table = (deactivated_users) => { email: sort_email, role: sort_role, }, + simplebar_container: $('#admin-deactivated-users-list .progressive-table-wrapper'), }); loading.destroy_indicator($('#admin_page_deactivated_users_loading_indicator')); diff --git a/static/js/stream_edit.js b/static/js/stream_edit.js index f128baca4d..b7de6f84ac 100644 --- a/static/js/stream_edit.js +++ b/static/js/stream_edit.js @@ -320,6 +320,7 @@ function show_subscription_settings(sub_row) { } }, }, + simplebar_container: $('.subscriber_list_container'), }); user_pill.set_up_typeahead_on_pills(sub_settings.find('.input'),