Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config option to disable scroll into view for targets of boosted links and forms #1459

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

xhaggi
Copy link
Contributor

@xhaggi xhaggi commented May 26, 2023

This adds a configuration option to enable/disable scrolling up after swapping a boosted link or form.

Related discussion #407

@David-Guillot you have requested something like that in #407 (comment)

@Telroshan Telroshan added the enhancement New feature or request label Sep 20, 2023
@Telroshan
Copy link
Collaborator

Hey it's me, the annoying semantic guy
I'm just wondering if boostedShowTop is the right choice of name here ; I know the swap spec uses the keyword "show" internally, but the user never gets to see that. I don't find that name super intuitive at the first glance to guess what it's doing.
Why not use the term scroll instead of show here, such as boostedScrollTop ?

Though looking at the existing config, maybe a name such as scrollBoostedTop or scrollBoostedElementsTop would be more consistent? (as we have refreshOnHistoryMiss, includeIndicatorStyles, allowEval for example, where the verb is the first word of the config property)
I'm suggesting this one here because swapSpec["show"] = "top" resolves to a scrollIntoView call

Let me know your thoughts on this

@sjc5
Copy link
Contributor

sjc5 commented Sep 20, 2023

I would love to see this get merged! One great thing about hx-boost is that you can, if you choose, basically just always target the full document, and get what you might call an "automatic" SPA user experience on top of a totally traditional MPA developer experience. Although not totally in line with HTMX's primary use case of using smaller, granular html snippets, I think this is also a really valid use case for a lot of sites. This is what we're doing by default with Hwy, for example. Unfortunately, the default scroll-to-top behavior makes some form interactions a little goofy, and not being able to turn that off is odd.

At risk of overthinking and delaying merging, I'd love to see this take options:

scrollTopAfterBoosted: true | false | "anchor-only" | "form-only"

@Telroshan Telroshan added the under discussion Issues that are being considered but require further alignment label Sep 23, 2023
@xhaggi
Copy link
Contributor Author

xhaggi commented Nov 2, 2023

I don't have a strong opinion on the name I've chosen, so I'll adapt it to your suggestion. Will do it tomorrow.

@Telroshan
Copy link
Collaborator

And I'm not a native speaker so let me invoke @alexpetros or @1cg as my choices of naming have already proven not to be the best ones in the past 😆

@1cg
Copy link
Contributor

1cg commented Nov 2, 2023

should we allow for more than just a global override, or is global good enough? (Thank you for your work on this @xhaggi )

@sjc5
Copy link
Contributor

sjc5 commented Nov 2, 2023

should we allow for more than just a global override, or is global good enough? (Thank you for your work on this @xhaggi )

I think global override is a good start. But yes, I think it would be great to also be able to override on a per-form / per-anchor basis. The main use case for me, just for the sake of being explicit, is to be able to return form validation results in what might be a long form, without scroll state being lost.

@alexpetros
Copy link
Collaborator

I think we should use scrollIntoView, because that aligns with the JS name. I also don't know why we would limit this to boosted links. I'd suggest that we do htmx.scrollIntoView which by default scrolls the non-OOB element into view on all swaps.

We also probably will want a granular way to enable/disable that (and add it to OOB swaps), but that's no reason to hold up the global config.

@sjc5
Copy link
Contributor

sjc5 commented Nov 2, 2023

I think we should use scrollIntoView, because that aligns with the JS name. I also don't know why we would limit this to boosted links. I'd suggest that we do htmx.scrollIntoView which by default scrolls the non-OOB element into view on all swaps.

We also probably will want a granular way to enable/disable that (and add it to OOB swaps), but that's no reason to hold up the global config.

Based on some background discussions I read earlier (sorry don't have the links handy), the limitation to boosted forms and anchors is to mimic default browser behavior (though the default smooth scroll violates this). Personally I just want a way to set it to no scrolling at all, to avoid the UI slingshotting effect when you submit a form (but still be able to do simple progressive enhancement), so other than that have no strong opinion :)

@alexpetros
Copy link
Collaborator

I would love to see this get merged! One great thing about hx-boost is that you can, if you choose, basically just always target the full document, and get what you might call an "automatic" SPA user experience on top of a totally traditional MPA developer experience

Unrelated, but isn't this just a regular link?

@sjc5
Copy link
Contributor

sjc5 commented Nov 2, 2023

I would love to see this get merged! One great thing about hx-boost is that you can, if you choose, basically just always target the full document, and get what you might call an "automatic" SPA user experience on top of a totally traditional MPA developer experience

Unrelated, but isn't this just a regular link?

Yep, regular anchor tag with hx-boost on for automatic client routing. Astro just recently released their client routing solution to get this same effect.

@1cg
Copy link
Contributor

1cg commented Nov 2, 2023

ok, so https://htmx.org/attributes/hx-swap/ is where you can specify scrolling behavior

by default it does nothing, unless we are taking about a boosted link or form, so this global really only applies to those and the name should probably reflect that

you can override the behavior of a boosted form w/ something like this:

<body hx-boost="true">
  ...
  <form hx-swap="innerHTML scroll:none" ... />
    ...
  </form>
  ...
</body>

(not tested)

@sjc5
Copy link
Contributor

sjc5 commented Nov 2, 2023

ok, so https://htmx.org/attributes/hx-swap/ is where you can specify scrolling behavior

by default it does nothing, unless we are taking about a boosted link or form, so this global really only applies to those and the name should probably reflect that

you can override the behavior of a boosted form w/ something like this:

<body hx-boost="true">
  ...
  <form hx-swap="innerHTML scroll:none" ... />
    ...
  </form>
  ...
</body>

(not tested)

Unless I'm missing something, I think it's really important to inherit the global hx-swap setting. Sometimes that's going to be more complex, refer to extensions, etc.

If you've set hx-boost at a parent level, you can just use traditional form attributes.

<form method="POST" action="/whatever" scrollTopOnResponse="false">

Having to repeat something like "morph:innerHTML" everywhere isn't really necessary

@1cg
Copy link
Contributor

1cg commented Nov 2, 2023

You can just modify the scrolling behavior:

<form hx-swap="scroll:none" ... />

I'm not saying we don't need this global config, I'm just thinking out loud about how widely it matters with respect to the name.

Maybe htmx.config.scrollTopIntoViewOnBoost?

@alexpetros
Copy link
Collaborator

alexpetros commented Nov 2, 2023

by default it does nothing, unless we are taking about a boosted link or form, so this global really only applies to those and the name should probably reflect that

I understand now, and agree—forgot that boost has special scrolling behavior. I think htmx.config.scrollTopIntoViewOnBoost is also the right name.

@sjc5
Copy link
Contributor

sjc5 commented Nov 2, 2023

You can just modify the scrolling behavior:

<form hx-swap="scroll:none" ... />

I'm not saying we don't need this global config, I'm just thinking out loud about how widely it matters with respect to the name.

Maybe htmx.config.scrollTopIntoViewOnBoost?

There may be a bug because I've never been able to get that syntax to work. It still scrolls to top for me on a boosted form, with hx-boost set on the body.

@1cg
Copy link
Contributor

1cg commented Nov 2, 2023 via email

@sjc5
Copy link
Contributor

sjc5 commented Nov 2, 2023

OK, that's a bug for sure. Do you mind debugging it?

Yep, happy to. Thanks for hanging in there with me :)

@alexpetros
Copy link
Collaborator

@sjc5 would you mind moving that investigation to a new issue?

@xhaggi are you available to change this to scrollIntoViewOnBoost?

@xhaggi
Copy link
Contributor Author

xhaggi commented Nov 3, 2023

First of all, scroll and show do different things when used as modifiers in hx-swap. show calls element.scrollIntoView and scroll calls element.scrollTop. For a boosted link or form, the swap modifier show is set to top by default.

You can just modify the scrolling behavior:
<form hx-swap="scroll:none" ... />

@1cg to change the default behavior, you have to use hx-swap="show:none" on boosted links and forms.

@alexpetros I don't really like scrollTopIntoViewOnBoost or scrollIntoViewOnBoost because neither explain what is really happening. What is being scrolled into the view? I like scrollTopOnBoost more.

@xhaggi xhaggi force-pushed the boosted-disable-default-show-top branch 2 times, most recently from 64fb23f to 6b827a0 Compare November 3, 2023 07:38
@xhaggi xhaggi changed the title config: new option to enable / disable scroll to top after swapping a boosted link or form New config option to enable / disable scroll to top after swapping a boosted link or form Nov 3, 2023
@xhaggi
Copy link
Contributor Author

xhaggi commented Nov 3, 2023

I have now decided to use scrollTopOnBoost and also added a note in the hx-swap documentation on how to disable the default behavior on a per-element basis

@alexpetros
Copy link
Collaborator

alexpetros commented Nov 3, 2023

@alexpetros I don't really like scrollTopIntoViewOnBoost or scrollIntoViewOnBoost because neither explain what is really happening. What is being scrolled into the view? I like scrollTopOnBoost more.

The reason to use scrollIntoViewOnBoost is that it mirrors the DOM API, which makes it easier to search the documentation for. If I'm like "can htmx do scrollIntoView that I read about on MDN," this makes it easier to find. I don't feel especially strongly about it vs scrollTopOnBoost, which is fine, but I do prefer scrollIntoViewOnBoost for this reason.

@1cg we just need a final decision on scrollTopOnBoost or scrollIntoViewOnBoost (or whatever you want the config to be called).

@xhaggi
Copy link
Contributor Author

xhaggi commented Nov 3, 2023

@alexpetros after a second thought the name scrollIntoViewOnBoost makes more sense, especially if you use hx-target on a boosted element. This will cause the target to scroll into the viewport. Without hx-target the body is used and it will scroll into the viewport, causing the page to scroll to the top. However, I think that probably very few users are aware of this. We should definitely make this clear in the documentation.

@xhaggi xhaggi force-pushed the boosted-disable-default-show-top branch from 6b827a0 to 609ac0f Compare November 3, 2023 17:47
@xhaggi xhaggi changed the title New config option to enable / disable scroll to top after swapping a boosted link or form Add config option to disable scroll into view for targets of boosted links or forms Nov 3, 2023
@xhaggi xhaggi changed the title Add config option to disable scroll into view for targets of boosted links or forms Add config option to disable scroll into view for targets of boosted links and forms Nov 3, 2023
@xhaggi xhaggi force-pushed the boosted-disable-default-show-top branch from 609ac0f to e2294fc Compare November 3, 2023 17:48
@xhaggi xhaggi force-pushed the boosted-disable-default-show-top branch from e2294fc to 8c8f8b8 Compare November 3, 2023 17:49
@xhaggi
Copy link
Contributor Author

xhaggi commented Nov 3, 2023

Force pushed. @alexpetros let me know if the documentation is clear enough.

@1cg 1cg merged commit f2d776c into bigskysoftware:dev Nov 3, 2023
1 check passed
@alexpetros
Copy link
Collaborator

@1cg beat me to punch and merged it, but the documentation is great :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request under discussion Issues that are being considered but require further alignment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants