Skip to content
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

webview js: New algorithm for identifying messages in view #3040

Merged
merged 4 commits into from
Nov 1, 2018

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Oct 11, 2018

TODO items before merge:

  • I think Document.prototype.elementsFromPoint (note plural) exists in an iOS 9 webview... but I'm not highly confident of that, so I'd like some manual testing that everything seems to work. @jackrzhang began work on this.
  • There's a spot where I need an assert, and haven't sorted out how to express that; suggestions welcome. (@borisyankov ?)
  • The last commit isn't really finished; I think it's not quite right in some cases.

The most important part of this branch is the first commit, which fixes #2988 ; and I'm pretty confident in it now apart from those first two points above. So I hope to get those resolved and get it merged soon.


The algorithm we have in getStartAndEndNodes for finding what
messages are visible on the screen (and therefore we should mark as
read) has some problems. Among them:

  • Most egregiously, if there are no messages at the bottom of the
    viewport, we fail to find anything at all! This happens in
    particular when you narrow to a conversation that only has a few
    messages and fits easily on the screen. This is Messages in a short thread don't get marked as read #2988.

  • When the bottom of the screen has a date separator, after which
    offscreen is a recipient bar, after which further offscreen is a
    message... we'll treat seeing that date separator as if it meant
    seeing the message.

  • When the top of the screen has a floating recipient bar, we'll
    treat that as seeing the message immediately after the recipient
    bar, as well as everything since then... which may extend many
    screenfuls into the past.

Fix all of those with an algorithm reworked more or less from scratch,
adapted from the algorithm used in the webapp.

  • Start from one visible message, and work up and down to the edges
    of the screen. The webapp does this with the selected message (the
    one in the blue box); we need to work a bit harder, which is
    someVisibleMessage and its helper midMessagePeer. Fixes Messages in a short thread don't get marked as read #2988.

  • Because we're walking up and down as needed rather than counting on
    finding a message at a single fixed point, we can stop using the
    hack of finding data-msg-id on things that aren't messages, like
    date separators; we just pass over such things to focus on messages.

  • To get at an element laid out in sequence with messages, and be
    able to look past something drawn over them like a floating
    recipient bar, we use elementsFromPoint to get the whole sequence
    of elements behind the one returned by elementFromPoint.

Copy link
Contributor

@jackrzhang jackrzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this out on simulators running iOS 9.0 and iOS 9.3 - Document.prototype.elementsFromPoint doesn't exist in an iOS 9 webview:

visible-messages-ios-9 0

Here's the error message, which is a bit difficult to discern above:
image

Another interesting symptom is that specifically on the 'Stream' screen (the second icon from the left on the bottom navigation bar), I can't scroll past the top or bottom edge of the view, and there's no bouncing / snap-back animation like you can observe on the screen for private messages.

@borisyankov
Copy link
Contributor

Good catch. It seems the plural version (elementsFromPoint) is not well supported:
https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/elementsFromPoint
It is not indicating anything that too correctly for Safari as it says just true but obviously this was added at some point and not Safari 1.0 :)
Apparently, we might have a problem with older Chrome versions too.

@borisyankov
Copy link
Contributor

borisyankov commented Oct 23, 2018

We should pick one of the polyfills:
https://github.com/jfkhoury/elementsFromPoint
https://gist.github.com/oslego/7265412
https://gist.github.com/iddan/54d5d9e58311b0495a91bf06de661380

None of them have tests nor look anything special.
But the code is also not too tricky.

@borisyankov
Copy link
Contributor

Another interesting symptom is that specifically on the 'Stream' screen (the second icon from the left on the bottom navigation bar), I can't scroll past the top or bottom edge of the view, and there's no bouncing / snap-back animation like you can observe on the screen for private messages.

That is not related for sure, we are using a FlatList there, but it is a rather curious thing. I'll look into that as I wouldn't expect the overscroll bouncing` not to work.

@gnprice gnprice force-pushed the visible-messages branch 2 times, most recently from 18b27fa to 648ee81 Compare October 30, 2018 01:16
@gnprice
Copy link
Member Author

gnprice commented Oct 30, 2018

Thanks @jackrzhang for that testing!

I've just pushed a revised version, based in part on pairing with @borisyankov . This has a fallback for the case where Document#elementsFromPoint is missing.

I haven't yet actually gone back and tested this exact version on iOS 9, though -- that's still to do.

Also in this version: a description of the browser versions we aim to support.

Some followups, beyond the commits here that are needed for fixing #2988 :

  • The last two, WIP commits here, which do some followup cleanups.
  • Our HTML is actually invoking "quirks mode". We should add a <!DOCTYPE html>. This will change the behavior of documentBody.clientHeight, of some scrolling nuances, and possibly other things; so we'll need to change some JS code at the same time, and test well. Not related to this PR, really, but I discovered it while looking things up for it.
  • We don't actually work on the Chrome 37 that shipped with Android L! Nor the Chrome 30 on K. ... The supported-version cutoffs I wrote here say we don't care, though. The Chrome 44 shipped with M works OK, though with minor degradation.

@@ -624,7 +617,7 @@ const handleLongPress = (target: Element) => {
sendMessage({
type: 'longPress',
target: target.matches('.header') ? 'header' : 'message',
messageId: getMessageIdFromNode(target),
messageId: ancestorMessageId(target),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be curious to see if that will not throw the exception in ancestorMessageId because of the if (!messagePeerAncestor) { clause.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Moved this code over to #3097 .

This is more complicated than OS versions because on the one hand,
Android WebViews update independently of the OS (hooray!); but on
the other hand, they don't quite consistently update to the latest
version either (dang.)
(Pairing with @borisyankov.)

We use this in a couple of spots already; we'll use it in
a few more soon.

This isn't actually necessary by the supported-version cutoffs listed
above.  But we'd already written it before we gathered the data to
produce those cutoffs, and it's cheap to keep around.
The algorithm we have in `getStartAndEndNodes` for finding what
messages are visible on the screen (and therefore we should mark as
read) has some problems.  Among them:

 * Most egregiously, if there are no messages at the bottom of the
   viewport, we fail to find anything at all!  This happens in
   particular when you narrow to a conversation that only has a few
   messages and fits easily on the screen.  This is zulip#2988.

 * When the bottom of the screen has a date separator, after which
   offscreen is a recipient bar, after which further offscreen is a
   message... we'll treat seeing that date separator as if it meant
   seeing the message.

 * When the *top* of the screen has a floating recipient bar, we'll
   treat that as seeing the message immediately after the recipient
   bar, as well as everything since then... which may extend many
   screenfuls into the past.

Fix all of those with an algorithm reworked more or less from scratch,
adapted from the algorithm used in the webapp (message_viewport.js).

 * Start from one visible message, and work up and down to the edges
   of the screen.  The webapp does this with the selected message (the
   one in the blue box); we need to work a bit harder, which is
   `someVisibleMessage` and its helper `midMessagePeer`.  Fixes zulip#2988.

 * Because we're walking up and down as needed rather than counting on
   finding a message at a single fixed point, we can stop using the
   hack of finding `data-msg-id` on things that aren't messages, like
   date separators; we just pass over such things to focus on messages.

 * To get at an element laid out in sequence with messages, and be
   able to look past something drawn over them like a floating
   recipient bar, we use `elementsFromPoint` to get the whole sequence
   of elements behind the one returned by `elementFromPoint`.

We leave `getMessageNode` and `getMessageIdFromNode` around for now
because they have a couple of other callers, which are best dealt with
in separate commits.
@gnprice gnprice merged commit 733c2f9 into zulip:master Nov 1, 2018
@gnprice
Copy link
Member Author

gnprice commented Nov 1, 2018

And merged. Thanks @jackrzhang for the testing and @borisyankov for the review!

I added a bit more text to the supported-versions comment, to describe this fact:

We don't actually work on the Chrome 37 that shipped with Android L! Nor the Chrome 30 on K. ... The supported-version cutoffs I wrote here say we don't care, though.

Those followups mentioned above are still open.

@gnprice
Copy link
Member Author

gnprice commented Nov 1, 2018

OK -- and filed #3097 and #3098 for those followups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Messages in a short thread don't get marked as read
3 participants