Scroll the chat column to the bottom when images load
ClosedPublic
Actions

Authored by epriestley on Mar 28 2015, 3:21 PM.

Details

Reviewers

btrahan
chad

Maniphest Tasks

T7558: Conpherence doesn't scroll to bottom in Durable Column regularly

Commits

Restricted Diffusion Commit
rPd9d0daecd7fd: Scroll the chat column to the bottom when images load

Summary

Fixes T7558. This might not be 100% perfect but should solve most of the issue.

I briefly looked at things like MutationObserver (some fancy next-gen browser junk) but couldn't immediately get it working.

Other methods for handling this kind of thing involve polling, complicated polyfills, etc. We could give MutationObserver a more serious effort if this is too leaky.

Test Plan

In a thread with some images, reloaded the page and saw the scrollbar stay at the bottom.
Tested with and without USB devices attached.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley updated this revision to Diff 29302.Mar 28 2015, 3:21 PM

epriestley retitled this revision from to Scroll the chat column to the bottom when images load.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added reviewers: chad, btrahan.

epriestley added a task: T7558: Conpherence doesn't scroll to bottom in Durable Column regularly.

Herald added a subscriber: epriestley. · View Herald TranscriptMar 28 2015, 3:21 PM

epriestley edited the test plan for this revision. (Show Details)Mar 28 2015, 3:23 PM

noidea

This revision is now accepted and ready to land.Mar 28 2015, 3:33 PM

Closed by commit rPd9d0daecd7fd: Scroll the chat column to the bottom when images load (authored by epriestley, committed by epriestley). · Explain WhyMar 28 2015, 3:35 PM

This revision was automatically updated to reflect the committed changes.

chad awarded a token.Mar 28 2015, 4:15 PM

Was playing with this trying to fix the bug and not sure about the math here? (see inline) I don't really know what's going on though. :/

webroot/rsrc/externals/javelin/lib/Scrollbar.js
316	What's the 64 here? I assume the "near the bottom" piece I thought scrollTop when scrolled near the bottom would more or less be the height? Basically not sure about visible either.

epriestley added inline comments.Apr 16 2015, 6:24 PM

webroot/rsrc/externals/javelin/lib/Scrollbar.js
316	Yeah, "64" is "near the bottom". Here's what it's trying to do: "visible" is how many pixels tall the scrollable container is. Basically the height of the scrollbar itself in pixels. "scrollTop" is the position of the top of the scrollbar. +------------+ \| text text \| \| text text \| <--- Content, height = 25 \| text text \| \| text text \| \| text text \| \| text text \| \| text text \| \| text text \| \| text text \| +=================+ <--- Viewport, scrollTop = 11 ("viewport position") \| \| text text \| ^\| \| \| text text \| .\| \| \| text text \| .\| \| \| text text \| .\| \| \| text text \| .\| \| \| text text \| .\| <--- Scrollbar, visible = 8 ("scrollbar height") \| \| text text \| .\| \| \| text text \| v\| +=================+ <--- (scrollTop + visible) = 19 ("bottom of viewport") \| text text \| \| text text \| \| text text \| \| text text \| +------------+ `scrollTop + visible` is the position of the top of the scrollable window plus the height of the window itself, giving us the bottom of the scrollable window. If the content has height 500 and the viewport has height 100, "scrollTop" will never be close to 500. The largest value it can ever have is 400, since then you're at the bottom of the document and the last 100 px of document content are visible.

Thanks! Very useful ascii diagram. :D

I think part of the issue is that the "load" event isn't that good/well-timed (possibly?). It seems like browsers are drawing the image, then calling our stuff, then we're adjusting the position (maybe?). This causes jumps (I think?). I'm not 100% sure this is what's happening and the code could definitely be buggy, but it feels like it might be the issue. It also doesn't really fully explain why the jumps end up in the wrong place instead of just being glitchy.

My plan of attack here would be something like:

Fiddle with things a bit and see if that's actually the issue / if I can figure out anything to do about it.
Likely make this behavior explicit opt-in for JX.Scrollbar since that feels like the easiest fix for T7838.
Possibly try to do MutationObserver again, but that felt like it might involve a huge amount of work.
If none of that makes headway, start rendering placeholder images and doing the load in JS. So the logic would become:
- We render out a placeholder <div />.
- We do the image load in JS.
- When the image loads, we can measure it.
- We swap it into the document with exactly the correct dimensions and scroll the thing at the same time.
- Hopefully that works?
- Although it's kind of a lot of work, this isn't completely silly/futile because T4190 probably needs to do something similar anyway.