Scroll to current category broken as of 2.8.0.beta5

bug
1

In Discourse versions before 2.8.0.beta5, if you edit a topic and click the Category select drop down, the current category is both selected and scrolled down to in the list of categories, such that it is immediately visible at the top, and its subcategories (if any) are visible. The topic can then be easily moved into a more appropriate subcategory.

Working in 2.8.0.beta4 and earlier:

As of 2.8.0.beta5, the current category is no longer scrolled to when opening the Category select drop down:

I hope these short video demos are helpful in quickly showing what I’m trying to describe.

Note: in my examples I have the “fixed category positions” and " fixed category positions on create" options on, however the scroll to current behavior is missing with these options on or off.

I confirmed this in local development by installing 2.8.0.beta4 and observing the desired behavior, and then updating to 2.8.0.beta5 and observing the behavior no longer working.

I dug around in the code until I found that the following large commit involving select-kit.js is where the change took place (wait a moment and it should jump to the lines of code in question):

The following code was removed from the _scrollToRow() method, which gets called by the _scrollToCurrent() method:

if (rowContainer) {
  const collectionContainer = rowContainer.parentNode;

  collectionContainer.scrollTop =
    rowContainer.offsetTop - collectionContainer.offsetTop;
}

I confirmed that adding this code back to select-kit.js in 2.8.0.beta5 fixes the issue, though as it’s not clear to me why it was removed, I’m uncertain of other possible side effects of adding it back.

Thank you for taking a look at this, and I hope we can get this behavior working again in a future release.

4 Likes
3

Hi, thanks for this :+1:

I don’t recall exactly why I did this, but AFAICT right now, I think this is a mistake too.

I made this PR:

If you can try this code and tell me if you think that would be enough for you please?

4

Hi Joffrey, thanks for taking a look at this! I tested the latest changes, however it appears it didn’t work. As it has been merged, I confirmed on the tests-passed branch.

Of the two commits in the PR, the first one works (ab0fbf1), but is overridden by the second (92943ff).

The change on line 992 seems fine but doesn’t have the expected effect. It does seem to need the code block that was removed that adjusts the collectionContainer.scrollTop value.

Again thanks!

1 Like
5

Yes I wasn’t sure if this behavior would be enough. I can add this back tomorrow. Can you be more precise on what exactly you don’t like about this behavior now please?

6

Hi Joffrey,

Thanks for continuing to look into this. With the current code changes (I’ve made sure to pull the latest code, clear tmp, and restart the development server), it has not regained the behavior that was present in versions before 2.8.0.beta5. It still currently no longer scrolls to show the current category at the top of the visible options in the drop down.

The correct/desired behavior is what is shown in the first video in the original post, however it currently is still working as in the second video. Currently the scrollbar always starts fully at the top, regardless of where the current category is in the options.

The line rowContainer?.focus({ preventScroll }); seems to have no effect in this case, regardless of if preventScroll is true, false, or even if the whole line is removed.

The essential line that makes the behavior work (not present in the current release) is:

collectionContainer.scrollTop =
          rowContainer.offsetTop - collectionContainer.offsetTop;

Without this, it does not scroll to the correct position, since scrollTop is 0 by default. After this line runs, scrollTop equals the correct number of pixels it needs to scroll down to.

Hopefully this is helpful. Please let me know if I can help clarify further.

Thanks!

7

Yes it’s not at the top but it’s in the viewport, which I would have said is enough.

8

Hi Joffrey,

Thanks for the feedback. I added more categories to my demo and confirmed the current category does appear now in the viewport, definitely an improvement. It would still be ideal to have it appear at the top of the list, as it used to. This would ensure that more of the subcategories beneath it are immediately visible and the user can click to change to a subcategory without first scrolling down to reveal them. In my opinion, this is a stronger use case than revealing adjacent unrelated categories. Was it changed to be centered on purpose, or was it an accidental regression of the previous behavior? In any case, up to you - just offering my 2 cents.

Again, thanks and I appreciate your hard work on Discourse!