Make timerow persistent (WhatsApp like implementation).

[ishammahajan]

On a recent discussion on chat.zulip.org it was determined that a
good way to go about showing the date consistently in the WebView
was to just copy the sticky timerow element from WhatsApp. That
implementation keeps a timerow which is always on top of the screen,
and as the other timerows whiz by it, its content gets updated.

I think this is the way to go, since it is easily understandable, is a good design, and make it harder to make errors on modification of code (if we require to do it at a later point in time).

(this gif is slightly sped up)

[borisyankov]

I would have called this 'sticky' instead of 'persistent'

[gnprice]

Awesome, thanks @ishammahajan ! This looked like a tricky CSS problem -- glad you were able to make it work. 🎉

Comments from playing with the UI:

• I like it!

• The date pill gets covered up by recipient headers as we scroll past them. I think it'd feel nicer to have the sticky date pill show in front of a scrolling recipient header, instead of vice versa.

  • In particular, with this PR's current version, when there are a lot of different topics interleaved so there are lots of recipient bars, if you scroll quickly it feels quite choppy -- the date pill's text basically flickers in and out.
  • On the other hand, I think we also want the sticky recipient header to cover up any scrolling date pill -- just like it currently does in this PR. It might be tricky to combine both of these properties; but if you can, that'd be great. 🙂
  • I suspect we'll then also want the date pill to be a bit more visually distinct from the recipient header for when they overlap. Either a border, or a contrasting background color.

• Now that we have the date in these sticky pills, let's remove the date from the recipient headers. Two reasons:

  • It feels pretty redundant showing them both -- especially because they're almost right next to each other.

  • Sometimes the date in the recipient header is wrong! 😮 This is actually a longstanding issue and it's pretty clear once you think about exactly how the recipient headers work... but although I'd never quite put my finger on the wrongness of it before, it becomes super conspicuous once this PR makes the real dates visible.

    Specifically, if a topic started on one date and continued to a later date without interruption, then when we're looking at the later messages, the sticky recipient header will be the one from the earlier date -- which may be much older than any message that's actually on the screen. Example:

    

• Small polish detail: the sticky date pill feels slightly out of sync with the scrolling ones. In particular if there's e.g. a "May 20" date pill between some messages and you scroll so it's just slightly higher than the sticky date pill, the sticky one will keep saying "May 19" or "May 17" or whatever even while the later "May 20" pill is slightly above it.

  • Instead, for perfect behavior the sticky pill should change to match a scrolling one at exactly the point where the scrolling pill is in the same spot.

• The time that the sticky pill hangs out after I stop scrolling feels a little long to me. Maybe make it a second shorter?

[gnprice]

And comments on the code:

bbbd107 timerow: Change design to pill type.

Looks good!

0ff5ed6 timerow: Add persistent timerow element.

+const privateTheme = `
+#timerow-persistent {
+  top: 0.5em;
+}
+`;

• Hmm, it's a little hard to understand what's going on here. What does "private" have to do with the position of the #timerow-persistent element?

  I think what's going on is that this is where you want to put it when there are no recipient headers -- this puts it right by the top, and when there are recipient headers you want it instead down below the sticky recipient header.

  Assuming that's right 😉 , one good way to express this would be: give the boolean a name like hasRecipientHeaders. And then have a line like top: ${hasRecipientHeaders ? "2.2em" : "0.5em" }.

• The name timerow was never quite on point, because they're dates not times... but now this #timerow-persistent isn't even a row. 😛 How about "date pill"?

  E.g. rename .timerow-content to .date-pill... and then this one can be #date-pill-sticky.

e7a9b2b timerow: Add functionality to react to scroll events.

• Cool, I see this getFirstVisibleMessage function is following the pattern of visibleMessageIds.

  That logic is pretty subtle, though. It's easy to get subtly wrong, either now or in future changes -- which makes it something I'd really like to keep in just one place, so each bug only has to get found and fixed once.

  It's OK to rewire how some of the data flows in order to make that natural. Note that we're already calling the existing function visibleMessageIds every time we call this new function.

+     msg-human-date="${humanDate(new Date(timestamp * 1000))}"

• Custom attributes we make up should always start with data-. This is a standard namespace reserved for applications' use.

• It'd probably be better to keep this information in a JS object (indexed by message ID), rather than as custom attributes in the DOM. Basically DOM attributes are good for info that needs to be visible to CSS, and for an ID like data-msg-id to index into other data... and for anything else, prefer JS objects.

  • One reason is performance; generally stuff in the DOM is not as well optimized as within the JS engine.
  • Also structuring things as JS objects tends to give more flexibility.
  • Perhaps an example of that last point: if this data comes in the form of a JS object, then all we need in handlePersistentTimestamp is a message ID... which is what visibleMessageIds already returns. 🙂 And it's not a coincidence that that's what it returns -- a message ID is a much more specific and structured thing than a DOM element, suitable for the Math.min and Math.max inside visibleMessageIds and again in sendScrollMessage, and for sending directly to the outside with sendMessage.

+const handlePersistentTimestamp = () => {
+  const firstVisibleMessage = getFirstVisibleMessage();
+
+  if (firstVisibleMessage) {

• Style nit: this causes the whole main line of the function's logic to get indented. Instead, use an early return:

const handlePersistentTimestamp = () => {
  const firstVisibleMessage = getFirstVisibleMessage();
  if (!firstVisibleMessage) {
    return;
  }

That way it's the short "nothing interesting here" cases that get indented, while the main narrative flow of the function is all aligned at constant indentation.

Same thing on the if (persistentTimestamp && replaceableDate) { condition... except that in that case, I think if it fails that's a bug. That's an even stronger reason to use the early-return pattern; it also should be made explicit, either with a one-line comment or by throwing an error instead of just returning.

+      if (dateTimeout) {
+        clearTimeout(dateTimeout);
+      }

• No need for the conditional. See clearTimeout docs on MDN.

[gnprice]

Oh, another small UI issue I just noticed:

• When there are no messages (including at the loading animation), a small gray pill appears. I think this is probably the sticky date pill with no date.

[gnprice]

Oh, an important bit of workflow:

• Please be sure to link to any related issues in the PR thread. In this case, we have the issue this is meant to address filed as It's really hard to figure out the date of the messages you're looking at #1336 . (My mentioning it just now takes care of this part; just next time, best to do so up front in the original PR description.)

• Separately: when an issue is going to be fixed by a given change, be sure to say "Fixes: #" in the commit message.

Together those make sure (a) the previous discussion is easy to find from the PR thread, (b) the PR is easy to find from the issue thread, (c) the issue gets properly closed when we merge the fix.

[ishammahajan]

Thanks for the detailed review!

• I suspect we'll then also want the date pill to be a bit more visually distinct from the recipient header for when they overlap. Either a border, or a contrasting background color.

That makes sense! What do you feel about adding a box-shadow over the container?

• Now that we have the date in these sticky pills, let's remove the date from the recipient headers.

Sure, that sounds good! (FWIW, b49c49d solves the problem of non replacing dates, and that solution is basically the same as this one! -- get the first visible message and replace it's date with the date present in the recipient header)

• the sticky date pill feels slightly out of sync with the scrolling ones

I agree, but again the issue comes because of the stickiness situation. I'll try to fix it though, have some ideas.

• The time that the sticky pill hangs out after I stop scrolling feels a little long to me.

I agree. Checked WhatsApps time right now, seems like it's about a second.

• When there are no messages (including at the loading animation), a small gray pill appears. I think this is probably the sticky date pill with no date.

Hmm. Didn't notice that, I thought the pill would completely disappear since it has no contents. Appears as if padding takes its own space even without content. Fixed though.

• when an issue is going to be fixed by a given change, be sure to say "Fixes: #" in the commit message.

I originally hadn't marked this because this was only a potential solution and not definitely going to be implemented. I didn't want to flood the given issue with lots of references (which I have done before due to multiple fixes and solution of issues, see #3385). My original plan was to only add that fixes line in the final push, so as to keep things clean.

As for the comments on the code, I have fixed most of them, but changes to the code now (based on mentioned changes) make it so that combining both getFirstVisibleMessage and visibleMessageIds is very tricky. I have slightly changed the logic by which we get our date, not requiring JS objects to be passed. Due to this another function had to be introduced, called walkToTimerowAbove. This was easily merged with the existing walkToMessage already present in the last commit. Sorry about the delay, it took me an embarrassingly long amount of time to understand why the getFirstVisibleMessage refused to work correctly. Found that it was because of const minOverlap = 20;.

[ishammahajan]

@gnprice I think now that #3491 has been implemented, this might be the next thing to focus on! :)

In the revision made today I just changed the design of the date-pill-sticky to match the same one we used for the timestamp in #3491. The normal date-pills also have the same design, except that they don't have the box-shadow. It looked too janky if so (in the case of header-->timerow-->message(which contains the timestamp) vertical order of elements).

Without the shadow

With the shadow

Let me know what you think about this. Also, as you might have noticed in the previous screenshot (second one) the shadow of the date-pill has been 'aimed' straight down as opposed to the timestamp where the shadow has been aimed slightly to the left.

[gnprice]

Thanks @ishammahajan !

Things seen in the UI from playing with it:

• Let's leave the sticky date-pill hidden when you first go to the narrow -- it feels visually distracting (and is gone fast enough I think people usually won't be able to read it). Can always start scrolling to get that info.

• I notice that you found a way to make the order of what floats above what work out! Sticky date pill above scrolling recipient headers, sticky recipient header above scrolling date pills, etc.

  More precisely, I didn't notice anything about this for a while, until I specifically looked for it (after being reminded by this thread) -- which seems like exactly the way it should be. 😉

• You mentioned this, but just observing that it's still true -- I think perhaps a little more so than before:

  the sticky date pill feels slightly out of sync with the scrolling ones

  Scrolling down, the sticky pill doesn't change until the scrolling one has gone past it by slightly more than the pills' height.

• The animation where the sticky pill moves in, and especially where it moves back out, feels a bit overly eye-catching. I'm not sure what to do about this -- I think a key cause of it is just that it has to move so far, to get to its spot below the sticky recipient header.

  ... Oh, here's one aspect that should be more straightforward to fix: when there isn't a recipient header, let's have it stick closer to the top -- probably exactly as far from the top in that case as it is from the bottom of the recipient header when there is one. Quite apart from the animation, it looks funny having it so far down, with space above it.

• It feels slightly off that the box-shadow is different for the date pill and the time pill. I don't have a strong view on what it should be -- and this is subtle, so I'm OK merging with the inconsistency and sorting it out later -- but it'd be good to find a consistent choice to use.

[gnprice]

And comments on the code:

when an issue is going to be fixed by a given change, be sure to say "Fixes: #" in the commit message.

I originally hadn't marked this because this was only a potential solution and not definitely going to be implemented. I didn't want to flood the given issue with lots of references (which I have done before due to multiple fixes and solution of issues, see #3385). My original plan was to only add that fixes line in the final push, so as to keep things clean.

Yeah, it's kind of an awkward UI bug in GitHub that making such references can flood an issue thread like that (though #3385 is a pretty mild example 😉, with only 5 in a row in the longest spot.)

I find the references extremely helpful when reading the Git log, though -- and it's really easy to forget a last-minute change like that when it isn't in the actual draft branch. So despite that GitHub UI issue, please do include the reference in your draft commits; we'll just live with a little noise on the issue threads. 🙂

(You could also try out a circumlocution like "Fixes issue 3385" -- that'll ensure the information is there even if you forget to edit it to the #3385 syntax later, but will prevent showing up in the issue thread.)

afcf160 timerow: Change design to pill type.

• Hmm, I see some em units here. ... Oh huh, some of them are in master! Looks like Add interactive timestamp to brief messages. #3491 was incompletely rebased -- the early versions used em because that's what we were using all over at that time, and then we rebased it but didn't change those to rem and/or px.

  I'll go fix the ones in master; then please rebase and fix the ones in the PR branch too. Generally things like padding should be in px.

b3de5af timerow: Add persistent timerow element.

• Similarly, let's convert padding, top, etc., to px. (Or rem if that seems hard in some cases -- though I think any of those may be a sign of something else we should convert to px.)

• Let's put the new div in htmlBody, rather than renderMessagesAsHtml -- it feels more like the things that are there like the older/newer spinners, the loading-messages animation, and the typing indicator.

• Can we simplify the CSS by adding class="date-pill" (well, modulo the rename) to the sticky date-pill element too?

164c657 timerow: Refactor - Change inaccurate names.

• Looks good!

• A small further improvement would be to rebase this to before timerow-persistent even gets added -- so that that commit uses the helpful name date-pill-sticky from the start.

4fd047a timerow: Add functionality to react to scroll events.

Sorry about the delay, it took me an embarrassingly long amount of time to understand why the getFirstVisibleMessage refused to work correctly. Found that it was because of const minOverlap = 20;.

• Ah, I see!

  I recommend you take this as an example of code that wasn't as clear as it should be -- that was in fact misleading. That sure can cause it to take a long time to understand... which is why we try hard to avoid writing code that way.

  I'd written this function and called it isVisible... and then secretly its semantics weren't actually to see if an element was visible (the thing the name says!), but rather whether it was I guess "thoroughly visible" in a particular sense. There's a reason we wanted those semantics, it's fine to have them... but the name said something else, and there wasn't any doc comment or anything else in the interface to indicate that there was this asterisk or wrinkle. So, sorry about that.

  And with this change, now there is a doc comment for it!

• OK, and then I wrote the point above before I'd seen this bit, but: getFirstVisibleMessage is a perfect example of a function with a high risk of causing the exact same kind of problem for the next person as isVisible did for you. 😛 (Where "the next person" might even be your own future self!)

  In particular: (a) it says "message" but can return a header-class element that isn't a message; (b) there's some other subtle distinction going on vs. visibleMessageIds that there's no hint of in the name or jsdoc...

  I see, I think the point is that you're avoiding counting a message if it's "visible" only far enough to be occluded by a recipient header?

  That is reasonable! It occurs to me that that is probably also a thing that the mark-as-read path (which uses visibleMessageIds) should be doing. :-) The recipient header is typically 32px high... so we're marking a message as read when only 20px of it pokes down into the viewport, and it's still 12px short of even beginning to be actually visible on screen.

  So that's an opportunity to unify these -- as well as something to fix in master.

• Hmm, if you're getting the date values by walking up to find a scrolling date pill anyway...

  how about just starting from someVisibleMessage() and walking straight up to find it?

  You'll want the first scrolling date-pill with getBoundingClientRect above a certain spot, corresponding to where the sticky date-pill goes when fully shown.

  That may simplify some of this.

79b7cba js: Consolidate walkTo functions into one.

• Cool, good to consolidate these. Smaller style points:

• Best to add the generality first, then add the use of it. That way there isn't an intermediate version where the code is duplicated, and also the commit that adds the use of it becomes clearer.

• elementType is really a class -- className would be a good name.

• Nit: spelling is "finicky".

[gnprice]

• Hmm, I see some em units here. ... Oh huh, some of them are in master! Looks like Add interactive timestamp to brief messages. #3491 was incompletely rebased -- the early versions used em because that's what we were using all over at that time, and then we rebased it but didn't change those to rem and/or px.
  I'll go fix the ones in master; then please rebase and fix the ones in the PR branch too. Generally things like padding should be in px.

Done!

1312e6a6b msglist css: Fix a few straggler `em` uses to `rem`.
b19845371 time pills: Reorder CSS properties a bit.
507b5e469 time pills: Define width on pill, not container.
a937bd688 time pills: Make the container's left edge irrelevant.
2fde24a59 time pills: Cancel out a translate-15% transform.
e16094a22 time pills: Simplify transform to a nice round 100%.
3dc7fee83 time pills: Convert most uses of rem to px.
3f8ad8345 time pills: Snap layout to our 4px grid.
994d0131b time pills: Allow width to vary with content.

The first commit converted all em to rem, which is an acceptable baseline. Converting to px was trickier 😉 , but that's what the remaining commits there accomplished!

[ishammahajan]

• Let's leave the sticky date-pill hidden when you first go to the narrow

The thing is, we always send a scroll message to the webview whenever we open a narrow, so this is unavoidable, the most I can do in this case is perhaps add a flag which checks for first load. Will that be acceptable?

• I notice that you found a way to make the order of what floats above what work out!

Wait, I don't understand. I did figure it out, and as I mentioned in chat, that change is in another branch of mine, timerow-persistent_v3. Were you looking at that, perhaps?

• You mentioned this, but just observing that it's still true -- I think perhaps a little more so than before:

  the sticky date pill feels slightly out of sync with the scrolling ones

  Scrolling down, the sticky pill doesn't change until the scrolling one has gone past it by slightly more than the pills' height.

Again, this seems to be the outdated version of the branch, since this issue has since been fixed. It transitions smoothly on my device/emulator -- at the precise moment when sticky date pill crosses the static pills.

• The animation where the sticky pill moves in, and especially where it moves back out, feels a bit overly eye-catching. I'm not sure what to do about this -- I think a key cause of it is just that it has to move so far, to get to its spot below the sticky recipient header.

Perhaps the branch timerow-persistent_v3 will amend things? In that case there will appear to be less of a transition since the sticky date pill will go under the recipient headers.

• when there isn't a recipient header, let's have it stick closer to the top

...It already does on my machine. Just like the 'sync' with static date pills feature, you seem to have a different behaviour from what I observe. Perhaps there is a bug at play here?

[gnprice]

Scrolling down, the sticky pill doesn't change until the scrolling one has gone past it by slightly more than the pills' height.

Again, this seems to be the outdated version of the branch, since this issue has since been fixed. It transitions smoothly on my device/emulator -- at the precise moment when sticky date pill crosses the static pills.

[...]

• when there isn't a recipient header, let's have it stick closer to the top

...It already does on my machine. Just like the 'sync' with static date pills feature, you seem to have a different behaviour from what I observe. Perhaps there is a bug at play here?

Hmm! This was based on commit 79b7cba -- here's what I have on my machine (as set by using ../zulip/tools/reset-to-pull-request 3496 yesterday to look at the code from this PR):

$ git k ..pr/3496
* 79b7cba2e (pr/3496) js: Consolidate `walkTo` functions into one.
* 4fd047a8f timerow: Add functionality to react to scroll events.
* 164c65795 timerow: Refactor - Change inaccurate names.
* b3de5af48 timerow: Add persistent timerow element.
* afcf16063 timerow: Change design to pill type.
o 8a1099fcc (pr/3491) msglist: Add slide-in timestamp labels for each message.

(That git k is another of my aliases. The tracking branches pr/NNN are a feature of our reset-to-pull-request -- you opt into it with git config zulip.prPseudoRemote pr, which sets a config variable locally for your current repo.)

And that commit is what's currently on the PR branch. (You can see that in the thread, above -- search for "pushed".)

Perhaps you're looking at a version on your machine that you intended to push here but haven't?

[gnprice]

Also to double-check I just went back to this PR's code (at 79b7cba), and restarted the Metro server (the one from react-native start) and rebuilt the app with react-native run-android.

I still get this behavior just as I described yesterday:

• when there isn't a recipient header, let's have it stick closer to the top

...It already does on my machine. Just like the 'sync' with static date pills feature, you seem to have a different behaviour from what I observe. Perhaps there is a bug at play here?

For this one, though:

Scrolling down, the sticky pill doesn't change until the scrolling one has gone past it by slightly more than the pills' height.

Again, this seems to be the outdated version of the branch, since this issue has since been fixed. It transitions smoothly on my device/emulator -- at the precise moment when sticky date pill crosses the static pills.

I find that it varies! Sometimes it's perfectly in sync -- sometimes it's off by more than the height of a pill.

There seems to be a pattern where when scrolled near the very bottom, it's in sync, and at some point as I scroll up it suddenly becomes out of sync. Not sure if that's real or a red herring from random variation.

That's from browsing a stream narrow; some things that happen in between where it's in and out of sync include an image, and an emoji reaction. Could be that one of those is somehow involved?

Other variables to consider when trying to repro/debug would be whether the scrolling pill comes just after a recipient header, or a message (of various kinds), etc.

[gnprice]

• The animation where the sticky pill moves in, and especially where it moves back out, feels a bit overly eye-catching. I'm not sure what to do about this -- I think a key cause of it is just that it has to move so far, to get to its spot below the sticky recipient header.

Perhaps the branch timerow-persistent_v3 will amend things? In that case there will appear to be less of a transition since the sticky date pill will go under the recipient headers.

OK, went and tried that branch. Specifically at 3cd7c72 .

(Workflow note: I recommend sending a PR for any branch you'd like me to try out. Makes it slightly easier to get the code, with ../zulip/tools/reset-to-pull-request... but more importantly, the list of PRs is something I regularly check to see what I should look at and reply to. So if something isn't there, I'm very likely to forget it existed -- as I did in this case! 😉 If you don't consider the branch ready for merge, that's fine; just say so in the PR and/or mark it as draft.)

I think I now understand the confusion on this bit:

• I notice that you found a way to make the order of what floats above what work out!

Wait, I don't understand. I did figure it out, and as I mentioned in chat, that change is in another branch of mine, timerow-persistent_v3. Were you looking at that, perhaps?

The difference I see in your "v3" branch is that the sticky date pill is (effectively) covered by the sticky recipient header -- whereas in this PR, the sticky date pill "floats over" the sticky recipient header.

Rereading my comments above, I think I didn't express a view on which of those two should occlude the other; though there were a lot of moving parts there to talk about. In any case, good to have prototypes to play with for both possibilities!

I agree, this helps with making the transition less eye-catching.

There's one gotcha in it which is a bit subtle, and which I don't have a solution for off the top of my head:

• The sticky recipient header slides under the next scrolling one (as that next one becomes the new sticky one.)
• So when a scrolling header is nearly at the top, you see it occluding part of the sticky header...
• and then if the date pill transitions at that point, it passes over part of the header -- but gets clipped abruptly at a random-looking spot in the middle of the topmost-looking other object, i.e. the scrolling header.
• Secretly it's getting clipped by this line that coincides with the bottom edge of the sticky header. But that edge is hidden by the scrolling header! It's as if the date pill is sliding under the sticky header... which is itself sliding under the scrolling header, which is itself shown under the date pill. A bit like an Escher drawing. 💫

Like I said, this effect is subtle -- it's only visible for a few frames, and only within fairly narrow windows of how things happen to be aligned by the scroll position. So, could be acceptable.

[gnprice]

• Let's leave the sticky date-pill hidden when you first go to the narrow

The thing is, we always send a scroll message to the webview whenever we open a narrow, so this is unavoidable, the most I can do in this case is perhaps add a flag which checks for first load. Will that be acceptable?

Ah, just noticed this question in here.

This is something where the code is a bit confusing -- we might perhaps give a better name to sendScrollMessage.

At first load, we call a function of ours named sendScrollMessageIfListShort, which might call sendScrollMessage.

But that function's job isn't really about scrolling. What it does is send a message from the webview (all of js.js is running inside the webview), to the MessageList code on the outside... that we call type: 'scroll' but the meaning we really give to it is: "mark the following messages as read".

The reason we might call that function on narrow is to mark the messages that are visible as read; it doesn't actually do any scrolling.

(We call it a "scroll" message because the most common reason we send one is that the user scrolled.)

So, for this new logic we don't need to put it in sendScrollMessage -- it's not really related to marking messages as read, after all. Let's call it only when the user actually scrolls.

[ishammahajan]

This is something where the code is a bit confusing -- we might perhaps give a better name to sendScrollMessage.

The code I call (handleStickyDatePill) is called from handleScrollEvent, which fires when a user scrolls the screen - window.addEventListener('scroll', handleScrollEvent);. I don't make use of sendScrollMessage at all.

For the issue, I think the browser itself sends a scroll message when a user opens up the webview. To confirm this theory I inserted the hide css class in the date-pill-sticky element, and opened the WebView. The browser threw an error No date-pill-sticky element! which is the error in handleStickyDatePill that fires if the element is absent from the tree (or if the function cannot access it for whatever reason.

Perhaps you're looking at a version on your machine that you intended to push here but haven't?

That's plausible (I cannot confirm since I have rebased since I pushed).

I find that it varies! Sometimes it's perfectly in sync -- sometimes it's off by more than the height of a pill.

I can't get it to vary for whatever reason. Here's the video, maybe you're doing something different to get it to reproduce? Would be interesting to see.
https://imgur.com/WCCsKJq

I still get this behavior just as I described yesterday:

• when there isn't a recipient header, let's have it stick closer to the top

...It already does on my machine. Just like the 'sync' with static date pills feature, you seem to have a different behaviour from what I observe. Perhaps there is a bug at play here?

The video above also contains a group chat where it's visible that the date pill is closer to the app bar than it would be in a stream.

Hmm! This was based on commit 79b7cba -- here's what I have on my machine (as set by using ../zulip/tools/reset-to-pull-request 3496 yesterday to look at the code from this PR):

Ah, my bad. In the git client I use it's easy to get confused/make the wrong click

v1, v2 and v3 are adjacent. I've made that mistake once or twice, which is why I said that 😄 .

[gnprice]

This is something where the code is a bit confusing -- we might perhaps give a better name to sendScrollMessage.

The code I call (handleStickyDatePill) is called from handleScrollEvent, which fires when a user scrolls the screen - window.addEventListener('scroll', handleScrollEvent);. I don't make use of sendScrollMessage at all.

Ah indeed. Sorry, I must have not been looking at your actual code when I added that reply, and just pattern-matched a guess.

For the issue, I think the browser itself sends a scroll message when a user opens up the webview. To confirm this theory I inserted the hide css class in the date-pill-sticky element, and opened the WebView. The browser threw an error No date-pill-sticky element! which is the error in handleStickyDatePill that fires if the element is absent from the tree (or if the function cannot access it for whatever reason.

Hmm, so -- I see, in the PR itself you don't have hide initially set on the sticky date pill:

+  pieces.push('<div id="date-pill-sticky"></div>');

and the CSS is set up so that without hide it's visible.

I'm confused about why an error happened. The function normally works fine if the pill is hidden, right? That's the normal case when you start scrolling after nothing's happened for a couple of seconds.

On the main point: It doesn't make a lot of sense to me that the browser would just routinely issue a scroll event at startup. But! We do have a number of calls to window.scroll and window.scrollBy in our code... IOW, we spontaneously cause scroll events in a variety of circumstances.

If you search for window.scroll (so, matching both those methods), and then trace up through callers, you'll find it happens

• when the viewport is resized -- this can include the keyboard being opened or closed, or the unreads banner appearing and disappearing
• inside handleUpdateEventContent -- i.e., when the React side sends an updated HTML blob, e.g. because a new message arrived
• inside handleInitialLoad... aha, which we call on first load!
• possibly other places, I haven't tried to check this is comprehensive

So the handleInitialLoad explains what we've been seeing.

We'll want to avoid the date-pill spontaneously showing up in all three of the above cases, though (and possibly others if there are others.)

Hmmm. I'm not actually sure what's a great way to do that. Options include

• Try to somehow identify scroll events that come from one of those, and ignore them here.
  • Maybe set a flag when we do one of these, and set a timer to clear it again in like 50ms? Then even a "natural" scroll in that interval will be ignored, but that's probably OK.
• Try to somehow identify scroll events that come from the user naturally. Listen for touches? And then somehow notice when the touch turns into a scroll?

You might look around at docs, the web, etc., and play around to see if you can find a good solution. I think the flag-plus-timer solution I described should be acceptable, if you don't find a better one.

[gnprice]

Rereading this thread, and trying to summarize the status:

• I made a number of code comments up here. Most are small.
• There's the issue where it can be slightly out of sync, which apparently is tricky to reproduce. We'll want to track this down, but I think it'll be fine to merge with this issue as is, and leave it as a followup.
• There's the issue where it shows up spontaneously -- most conspicuously when the narrow is first opened. Discussed in the last couple of comments above, and I suggested a hack solution that I think should work well enough.
• A couple of other UI comments are in this comment.

I think this is getting close to merging! Looking forward to seeing the next version when I'm back from my upcoming vacation.

[ishammahajan]

If you search for window.scroll (so, matching both those methods), and then trace up through callers, you'll find it happens

* when the viewport is resized -- this can include the keyboard being opened or closed, or the unreads banner appearing and disappearing

* inside `handleUpdateEventContent` -- i.e., when the React side sends an updated HTML blob, e.g. because a new message arrived

* inside `handleInitialLoad`... aha, which we call on first load!

* possibly other places, I haven't tried to check this is comprehensive

So the handleInitialLoad explains what we've been seeing.

Ahh, that makes a lot of sense. Thanks for the investigation!
Turns out, the MDN docs list a perfect event for our application, touchmove. Happily, we already use this!

The touchmove event is fired when one or more touch points are moved along the touch surface.