Page MenuHomePhabricator

mw.viewport.isElementInViewport broken when page scrolled
Closed, ResolvedPublic

Description

As soon as the page scrolls somewhere, the function will not work properly.

See gif for example: https://i.imgur.com/kTgNwUn.gif

  1. Go to page
  2. Scroll down
  3. Load mw.viewport
  4. Select element that is clearly in viewport in Elements pane
  5. Write in console mw.viewport.isElementInViewport($0)
  6. See a very nice false false

Seems to me looking at the source that the implementation assumes that getBoundingClientRect gives you something like jquery's .offset when in reality that function already gives you the cooridnates relative to the viewport, not to the top of the page.

Here's an example implementation that works with getBoundingClientRect (only taking the height into account though): https://github.com/joakin/loot-ui/blob/master/lib/client/components/lazy-images/index.js#L41-L50

Found trying to remove MobileFrontend's specific version in favor of core's one. (https://gerrit.wikimedia.org/r/#/c/275876/)

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 10 2016, 12:00 PM

Change 276451 had a related patch set uploaded (by Jhernandez):
Fix mw.viewport.isElementInViewport with scrolled page

https://gerrit.wikimedia.org/r/276451

Thanks @Jhernandez. The function used to use jQuery's .offset originally, so I must not have been diligent enough in my testing after switching to getBoundingClientRect.

jhobs triaged this task as High priority.Mar 10 2016, 6:03 PM

Change 276451 merged by jenkins-bot:
Fix mw.viewport.isElementInViewport with scrolled page

https://gerrit.wikimedia.org/r/276451