-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use html
as scroll container for recent view.
#30134
Conversation
html
as scroll container from recent view.html
as scroll container for recent view.
42bf7aa
to
6de22ad
Compare
html
as scroll container for recent view.html
as scroll container for recent view.
Works for me in light manual testing! |
The bug is hard to reproduce, but I was able to reproduce with zulip#30134 and used the same strategy to fix it here.
The bug is hard to reproduce, but I was able to reproduce with zulip#30134 and used the same strategy to fix it here.
The bug is hard to reproduce, but I was able to reproduce with zulip#30134 and used the same strategy to fix it here.
The bug is hard to reproduce, but I was able to reproduce with #30134 and used the same strategy to fix it here.
Pushed to resolve conflicts. |
Test-deploying this on chat.zulip.org. |
0e3bc2d
to
b9997d1
Compare
web/src/list_widget.ts
Outdated
// TypeEventHandlers<HTMLElement, any, any, any> which is confusing. | ||
// | ||
// @ts-expect-error Maybe JQuery<Window>.TypeEventHandlers is not defined? | ||
meta.$scroll_event_handler.off("scroll.list_widget_container"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you start a #frontend thread about this? Seems like something is wrong here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, seems like it'll be OK to address in a quick follow-up, so not going to block merging on this.
web/src/list_widget.ts
Outdated
@@ -261,6 +262,13 @@ export function create<Key, Item = Key>( | |||
old_widget.clear_event_handlers(); | |||
} | |||
|
|||
// When `html` is the scroll container, scroll event is fired on `window`. | |||
// TODO: Add an online reference for this since it's not intuitive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the proposal in this comment, or the purpose of this block in general.
Also why is this called $scroll_event_handler
? To me that really sounds like a function, not a DOM element; how does this differ from $scroll_container
? Probably you just want a clear naming distinction between the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed $scroll_event_handler
to $scroll_listening_element
.
Modified the comment to be more clear:
// When `html` is is `$scroll_container`, scroll event is fired on `window/document`.
// This is because when a scroll event happens on the body or html element,
// the event is fired on the document object, not the body or html element itself.
// We still keep `html` as `$scroll_container` to use it's various methods as `HTMLElement`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified and moved this inside the conditional block:
if ($scroll_listening_element.is("html")) {
// When `$scroll_container` is the entire page (`html`),
// scroll events are fired on `window/document`, so we need to
// listen for scrolling events on that.
//
// We still keep `html` as `$scroll_container` to use
// its various methods as `HTMLElement`.
$scroll_listening_element = $(window);
}
const compose_max_height = $("html").css("--max-unexpanded-compose-height"); | ||
assert(typeof compose_max_height === "string"); | ||
const scroll_max = document.body.scrollHeight - Number.parseInt(compose_max_height, 10); | ||
return scroll_position + scroll_height >= (2 / 3) * scroll_max; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you tweak this function to document why --max-unexpanded-compose-height
is the right compose height to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this comment:
// `--max-unexpanded-compose-height` is just empty space below the last rendered row
// in recent view. We don't want user to see this empty space until there are no new
// rows to render when the user is scrolling to the bottom of the view. So, we render new
// rows when user has scrolled 2 / 3 of (the total scrollable height - the empty space).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, made some small edits and line-wrapped it.
This looks pretty good; posted a round of reviews and wanted to note that I think we can merge this after completing those. |
Updated based on feedback. |
} | ||
meta.$scroll_listening_element.on( | ||
"scroll.list_widget_container", | ||
function (this: HTMLElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If $scroll_listening_element
is JQuery<Window>
, then this
is not HTMLElement
.
Since we moved to use standard empty view handlebars everywhere, this became stale.
If recent view load more banner is at the center as a result of `document.elementFromPoint(topic_center_x, topic_center_y)`, there is no `tr` to focus, resulting in first row to be focused as the closest element as per previous logic. To fix it, we just focus the last row which is just above the load more banner and is visible.
Fixes zulip#17933, zulip#27517 Instead of `recent_view_table`, we make `html` as our scroll container. This fixes an important bug for us where filters sometimes disappear due to them scrolling under navbar which is unexpected. Since we are now using separate containers to display rows and filter (while includes table headers), where filters use sticky positioning, this bug will be fixed.
Merged after doing a few edits to the comments; we have two TS-related follow-ups to fix: |
@amanagr just a note on GitHub's commit message auto-closing feature: It does no fancy parsing at all -- it doesn't look at punctuatioin, and just looks for phrases like " |
discussion: https://chat.zulip.org/#narrow/stream/9-issues/topic/recent.20conversations.20loses.20filter.20checkboxes
Things I tested:
Combined feed
-> recent view correctly sets focus to first row.