web: behaviour: fix scroll on build history list #5392

aoldershaw · 2020-04-01T02:17:27Z

Existing Issue

Fixes #5383 .

Changes proposed in this pull request

Introduce scrollCoordsSimple function for dealing with simple cases of computing scroll position (that don't require knowing the coordinates of the source and parent elements)
Replace the deprecated DOM event mousewheel with wheel, which fixes scrolling on Firefox (which seems to have been broken for a while)

Note that this behaviour could be shoehorned in to the more complicated scrollCoords function, but there is a noticeable performance hit in scrolling due to the increased number of calls out to Elm.Browser.

Contributor Checklist

Updated release notes (located at https://github.com/concourse/concourse/tree/master/release-notes)

Reviewer Checklist

Code reviewed
Release notes reviewed
PR acceptance performed

cirocosta · 2020-04-01T13:51:46Z

Hey @aoldershaw ,

I was able to get scrolling working on Chrome after the fix, but not on Firefox 🤔 - do you think it'd be hard to adapt for it too? the fix you provided doesn't seem to deal with anything that's platform dependent, which makes me wonder if it even worked before on Firefox at all 🤔

thanks!

aoldershaw · 2020-04-01T14:21:52Z

Hey @cirocosta, thanks for testing in multiple browsers! Weird that it doesn't work in Firefox - this is essentially just reverting the scrolling behaviour to 495904d.

It also seems to not be the same scrolling issue as I set out to fix - on Chrome, you could scroll horizontally for a split second before having it forced back to the latest build, whereas on Firefox, it doesn't seem to move at all.

I'll take a look!

EDIT: can confirm that scrolling doesn't appear to work on 5.8.0 on FF

aoldershaw · 2020-04-01T15:34:10Z

@cirocosta turns out we were using a deprecated event onmousewheel when we should have been using onwheel - https://developer.mozilla.org/en-US/docs/Web/API/Element/wheel_event.

Seems to work on Chrome and Firefox now

jomsie · 2020-04-02T18:22:58Z

scrolling looks good on both chrome/safari (on osx), but firefox (on osx) is very slow scrolling... takes a whole scroll to go back 1 or 1.5 builds. (might be a firefox issue?)

still, slow scrolling beats no scrolling at all!

aoldershaw · 2020-04-02T18:34:08Z

scrolling looks good on both chrome/safari (on osx), but firefox (on osx) is very slow scrolling... takes a whole scroll to go back 1 or 1.5 builds. (might be a firefox issue?)

Huh, weird...I hadn't noticed since I was just using my trackpad, but I am experiencing the same with a mouse. The deltaY values seem to be on completely different scales between Chrome and Firefox...not sure how best to deal with that. We could try just checking the sign of deltaY and scale it to a fixed value, but that could be annoying trying to scroll a small amount.

EDIT: ah, it may be the case that the deltaMode differs (Firefox is measuring in terms of lines, rather than pixels - https://stackoverflow.com/questions/20110224/what-is-the-height-of-a-line-in-a-wheel-event-deltamode-dom-delta-line). I'll take a look

jomsie · 2020-04-02T18:41:29Z

scrolling looks good on both chrome/safari (on osx), but firefox (on osx) is very slow scrolling... takes a whole scroll to go back 1 or 1.5 builds. (might be a firefox issue?)

Huh, weird...I hadn't noticed since I was just using my trackpad, but I am experiencing the same with a mouse. The deltaY values seem to be on completely different scales between Chrome and Firefox...not sure how best to deal with that. We could try just checking the sign of deltaY and scale it to a fixed value, but that could be annoying trying to scroll a small amount.

EDIT: ah, it may be the case that the deltaMode differs (Firefox is measuring in terms of lines, rather than pixels - https://stackoverflow.com/questions/20110224/what-is-the-height-of-a-line-in-a-wheel-event-deltamode-dom-delta-line). I'll take a look

👀 look tomorrow

aoldershaw · 2020-04-06T23:37:25Z

Okay @jomsie, I've added some handling for deltaMode, which makes it feel more natural on Firefox. Let me know what you think!

#5383 This was broken by #5275 - in order to get ScrollToId to work, we needed to replace `getViewportOf` with `getElement`. However, due to elm/browser#86, the `viewport` and `scene` returned by `getElement` is of the document rather than the target. This could be fixed by using `.parentElem` instead of `.srcElem` in the `getX` and `getY` functions, since we manually update the `viewport` and `scene` to match the target for `.parentElem`. However, there is a noticable performance degradation needing to perform 3 browser `Tasks` (`getElement` * 2 + `getViewportOf` * 1) rather than performing just the one that's required for the other scroll behaviours. I introduced `scrollCoordsSimple` to handle this simple case, while leaving `scrollCoords` alone for the more complex case. Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>

#5383 onmousewheel is deprecated and non-standard, and was replaced with onwheel (source: https://developer.mozilla.org/en-US/docs/Web/API/Element/wheel_event). Despite it working on Chrome, it does not on Firefox Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>

#5383 Consider `deltaMode` in the `wheel` event for determining how much to scroll. Firefox is the only browser I've encountered that seems to generate events with DeltaModeLine (when scrolling with a mouse wheel, and not a trackpad). According to https://bugzilla.mozilla.org/show_bug.cgi?id=943034, the height of a line is based on the `font-size` of the scrollable element, which in this case is 20px. The page height is set to 800px, which is taken from https://github.com/facebookarchive/fixed-data-table/blob/3a9bf338b22406169e7261f85ddeda22ddce3b6f/src/vendor_upstream/dom/normalizeWheel.js#L23. Note that these values don't need to be exact - they should just feel natural. I've never actually seen a DeltaModePage event in practice, so I'm not sure if it will feel perfect, but oh well! Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>

jomsie · 2020-04-09T16:18:36Z

i'll review this after lunch!!!

jomsie · 2020-04-13T19:51:58Z

web/elm/src/StrictEvents.elm

@@ -28,6 +30,12 @@ type alias ScrollState =
    }


+type DeltaMode


the things firefox makes you do 😆

jomsie · 2020-04-13T20:50:55Z

Just some evidence!

safari:

chrome:

firefox:

jomsie · 2020-04-13T21:27:59Z

web/elm/src/Build/Header/Header.elm

@@ -394,12 +395,23 @@ update msg ( model, effects ) =
    case msg of
        ScrollBuilds event ->
            let
+                scrollFactor =
+                    case event.deltaMode of
+                        DeltaModePixel ->


Just a side comment, not super important, how'd you get the 20 / 800 for line / page?

Totally fair question! Tried to give some justification for it in the commit message I added that in.

tl;dr 20 for Line is based on the font-size of the scroll bar. The 800 for Page is totally arbitrary - I took it from some Facebook library

Note: I haven’t been able to produce a DeltaModePage event, so not sure if it feels natural, but it’s better than nothing!

jomsie

I'm diggin' this, unfortunate about all the firefox stuff, but seems great.

Just one comment that is purely for my curiosity:
https://github.com/concourse/concourse/pull/5392/files#r407731952

aoldershaw requested a review from a team April 1, 2020 02:17

aoldershaw force-pushed the issue/5383 branch from 5ee9ae0 to eb76b04 Compare April 1, 2020 02:21

aoldershaw force-pushed the issue/5383 branch from 55eff3d to a42c166 Compare April 1, 2020 18:00

jomsie self-assigned this Apr 2, 2020

Aidan Oldershaw added 5 commits April 8, 2020 19:30

add release note

6090352

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>

move release notes to latest.md

fda56e4

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>

aoldershaw force-pushed the issue/5383 branch from 04a8611 to fda56e4 Compare April 8, 2020 23:34

jomsie reviewed Apr 13, 2020

View reviewed changes

jomsie approved these changes Apr 13, 2020

View reviewed changes

aoldershaw merged commit 91f2ab7 into master Apr 14, 2020

aoldershaw deleted the issue/5383 branch April 14, 2020 13:28

jamieklassen mentioned this pull request Apr 22, 2020

Build scrolling broken #5490

Closed

jwntrs added the release/documented Documentation and release notes have been updated. label Apr 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

web: behaviour: fix scroll on build history list #5392

web: behaviour: fix scroll on build history list #5392

aoldershaw commented Apr 1, 2020 •

edited

cirocosta commented Apr 1, 2020 •

edited

aoldershaw commented Apr 1, 2020 •

edited

aoldershaw commented Apr 1, 2020

jomsie commented Apr 2, 2020

aoldershaw commented Apr 2, 2020 •

edited

jomsie commented Apr 2, 2020

aoldershaw commented Apr 6, 2020

jomsie commented Apr 9, 2020

jomsie Apr 13, 2020

jomsie commented Apr 13, 2020

jomsie Apr 13, 2020

aoldershaw Apr 13, 2020 •

edited

jomsie left a comment

web: behaviour: fix scroll on build history list #5392

web: behaviour: fix scroll on build history list #5392

Conversation

aoldershaw commented Apr 1, 2020 • edited

Existing Issue

Changes proposed in this pull request

Contributor Checklist

Reviewer Checklist

cirocosta commented Apr 1, 2020 • edited

aoldershaw commented Apr 1, 2020 • edited

aoldershaw commented Apr 1, 2020

jomsie commented Apr 2, 2020

aoldershaw commented Apr 2, 2020 • edited

jomsie commented Apr 2, 2020

aoldershaw commented Apr 6, 2020

jomsie commented Apr 9, 2020

jomsie Apr 13, 2020

Choose a reason for hiding this comment

jomsie commented Apr 13, 2020

jomsie Apr 13, 2020

Choose a reason for hiding this comment

aoldershaw Apr 13, 2020 • edited

Choose a reason for hiding this comment

jomsie left a comment

Choose a reason for hiding this comment

aoldershaw commented Apr 1, 2020 •

edited

cirocosta commented Apr 1, 2020 •

edited

aoldershaw commented Apr 1, 2020 •

edited

aoldershaw commented Apr 2, 2020 •

edited

aoldershaw Apr 13, 2020 •

edited