Scroll to expose expanded items in location list

[raksooo]

This PR adds automatic scroll when expanding countries and cities in the location list to make as much of the content as possible fit into the scroll view.

Git checklist:

• Describe the change in CHANGELOG.md under the [Unreleased] header.
• Check that commits follow the Mullvad coding guidelines

---

This change is 

[pronebird]

Reviewed 3 of 8 files at r1.
Reviewable status: 3 of 8 files reviewed, 1 unresolved discussion (waiting on @raksooo)

---

gui/src/renderer/components/Accordion.tsx, line 95 at r1 (raw file):

  private onWillExpand() {
    if (this.contentRef.current) {
      this.props.onWillExpand?.(this.contentRef.current.offsetHeight);

Any reason why you don't want to reuse this.getContentHeight() instead?

[pronebird]

Reviewed 1 of 8 files at r1.
Reviewable status: 4 of 8 files reviewed, 2 unresolved discussions (waiting on @raksooo)

---

gui/src/renderer/components/Accordion.tsx, line 69 at r1 (raw file):

    if (!this.state.mountChildren) {
      this.setState({ mountChildren: true }, () => {
        this.onWillExpand();

These two lines can be moved into a closure or a function because it's a code duplication:

this.onWillExpand();
this.setState({ containerHeight: this.getContentHeight() });

[pronebird]

Reviewed 1 of 8 files at r1.
Reviewable status: 5 of 8 files reviewed, 3 unresolved discussions (waiting on @raksooo)

---

gui/src/renderer/components/CustomScrollbars.tsx, line 90 at r1 (raw file):

  }

  public scrollIntoView(elementDimensions: DOMRect) {

Dimensions normally refer to size based on my experience using various APIs, although here we have position + size expressed by DOMRect, so maybe it argument name should reflect that.

In DOM API terms "clientRect" would be more appropriate and easier to understand (hint: getBoundingClientRect). In that case it becomes clearer that the expected coordinates must be viewport relative.

[pronebird]

Reviewable status: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @raksooo)

---

gui/src/renderer/components/CustomScrollbars.tsx, line 93 at r1 (raw file):
Same nitpick here, as per google the definition of "dimension" is:

a measurable extent of a particular kind, such as length, breadth, depth, or height.

So if it's a rect, call it a rect or frame -- it's common.

[raksooo]

Reviewable status: 0 of 8 files reviewed, 4 unresolved discussions (waiting on @pronebird)

---

gui/src/renderer/components/Accordion.tsx, line 69 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

These two lines can be moved into a closure or a function because it's a code duplication:

this.onWillExpand();
this.setState({ containerHeight: this.getContentHeight() });

Done.

---

gui/src/renderer/components/Accordion.tsx, line 95 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Any reason why you don't want to reuse this.getContentHeight() instead?

this.getContentHeight() did return the value suffixed with "px", I've refactored it into two functions to make it reusable.

---

gui/src/renderer/components/CustomScrollbars.tsx, line 90 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Dimensions normally refer to size based on my experience using various APIs, although here we have position + size expressed by DOMRect, so maybe it argument name should reflect that.

In DOM API terms "clientRect" would be more appropriate and easier to understand (hint: getBoundingClientRect). In that case it becomes clearer that the expected coordinates must be viewport relative.

I agree 👍 I've changed it to buttonRect/locationRect/clientRect from CityRow/CountryRow to CustomScrollbars.

---

gui/src/renderer/components/CustomScrollbars.tsx, line 93 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Same nitpick here, as per google the definition of "dimension" is:

a measurable extent of a particular kind, such as length, breadth, depth, or height.

So if it's a rect, call it a rect or frame -- it's common.

Done.

[pronebird]

Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @pronebird and @raksooo)

---

gui/src/renderer/components/CustomScrollbars.tsx, line 96 at r1 (raw file):

      // The element position needs to be relative to the parent, not the document
      elementDimensions.y -= scrollableDimensions.top;
      const bottomOverflow = elementDimensions.bottom - scrollableDimensions.height;

A more generic solution would be to find a nearest scroll position that would reveal the element when it's above or below the visible scroll area.

Min/max for scroll area would be:

scrollRect = scroll.getClientBoundingRect()

minScrollY = 0
maxScrollY = scroll.offsetHeight - scroll.scrollHeight

Then given the DOMRect relative to the viewport, get the rect relative to the scroll area (horizontal coordinates omitted):

elementTop = elementRect.top - scrollRect.top
elementBottom = elementRect.bottom - scrollRect.top

Then I think it gets easier to compute the scroll coordinate:

var currentScrollTop = scroll.scrollTop
var currentScrollBottom = scroll.scrollTop + scroll.offsetHeight

var nextScrollTop = elementTop

if nextScrollTop < currentScrollTop {
  nextScrollTop = max(elementTop, minScrollY)
}
else if elementBottom > currentScrollBottom {
  nextScrollTop = min(elementBottom, maxScrollY)
}

scroll.scrollTo({
  top: nextScrollTop,
  behaviour: 'smooth'
})

[pronebird]

Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @pronebird and @raksooo)

---

gui/src/renderer/components/CustomScrollbars.tsx, line 96 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

A more generic solution would be to find a nearest scroll position that would reveal the element when it's above or below the visible scroll area.

Min/max for scroll area would be:

scrollRect = scroll.getClientBoundingRect()

minScrollY = 0
maxScrollY = scroll.offsetHeight - scroll.scrollHeight

Then given the DOMRect relative to the viewport, get the rect relative to the scroll area (horizontal coordinates omitted):

elementTop = elementRect.top - scrollRect.top
elementBottom = elementRect.bottom - scrollRect.top

Then I think it gets easier to compute the scroll coordinate:

var currentScrollTop = scroll.scrollTop
var currentScrollBottom = scroll.scrollTop + scroll.offsetHeight

var nextScrollTop = elementTop

if nextScrollTop < currentScrollTop {
  nextScrollTop = max(elementTop, minScrollY)
}
else if elementBottom > currentScrollBottom {
  nextScrollTop = min(elementBottom, maxScrollY)
}

scroll.scrollTo({
  top: nextScrollTop,
  behaviour: 'smooth'
})

Typo:

maxScrollY = scroll.offsetHeight - scroll.scrollHeight

Really meant to be the other way around:

maxScrollY = scroll.scrollHeight - scroll.offsetHeight

[pronebird]

Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions (waiting on @pronebird and @raksooo)

---

gui/src/renderer/components/Accordion.tsx, line 69 at r1 (raw file):

Previously, raksooo (Oskar Nyberg) wrote…

Done.

Seems like a heavy artillery to use Promises here, but if you like them why not.

[raksooo]

Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions (waiting on @pronebird)

---

gui/src/renderer/components/Accordion.tsx, line 69 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Seems like a heavy artillery to use Promises here, but if you like them why not.

I like the await syntax but agree that it's a bit overkill here. I've changed it to a callback.

---

gui/src/renderer/components/CustomScrollbars.tsx, line 96 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Typo:

maxScrollY = scroll.offsetHeight - scroll.scrollHeight

Really meant to be the other way around:

maxScrollY = scroll.scrollHeight - scroll.offsetHeight

This has been updated after discussing this further on slack.

[pronebird]

Reviewed 8 of 8 files at r2, 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[pronebird]

Reviewed 4 of 6 files at r4.
Reviewable status: 6 of 8 files reviewed, all discussions resolved (waiting on @pronebird)

---

gui/src/renderer/components/SelectLocation.tsx, line 47 at r4 (raw file):

export default class SelectLocation extends Component<IProps> {
  private scrollView = React.createRef<CustomScrollbars>();
  private heightAdditionRef = React.createRef<SpacePreAllocationView>();

Nit: maybe reflect it in the variable name too.

[pronebird]

Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @pronebird and @raksooo)

---

gui/src/renderer/components/SelectLocation.tsx, line 237 at r4 (raw file):

}

interface IHeightAdditionProps {

Nit: here too.

[pronebird]

Reviewed 2 of 6 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @raksooo)

[raksooo]

Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on @pronebird)

---

gui/src/renderer/components/SelectLocation.tsx, line 47 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: maybe reflect it in the variable name too.

I was a bit too quick when changing the name. Fixed.

---

gui/src/renderer/components/SelectLocation.tsx, line 237 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: here too.

Done.

[pronebird]

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

[pronebird]

Reviewed 7 of 7 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved