Page MenuHomePhabricator

Scroll the chat column to the bottom when images load

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



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

rP Phabricator
Lint Passed
No Test Coverage
Build Status
Buildable 5020
Build 5038: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

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.
This revision is now accepted and ready to land.Mar 28 2015, 3:33 PM
This revision was automatically updated to reflect the committed changes.

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. :/


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.


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.