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

Define that push/replaceState don't affect to :target handling #639

Closed
smaug---- opened this issue Feb 6, 2016 · 16 comments
Closed

Define that push/replaceState don't affect to :target handling #639

smaug---- opened this issue Feb 6, 2016 · 16 comments

Comments

@smaug----
Copy link

https://html.spec.whatwg.org/multipage/browsers.html#history-1

One could argue push/replaceState should affect to :target handling, but given that no browser seems to do that, we should probably just spec the current handling.

https://bugzilla.mozilla.org/show_bug.cgi?id=1246240
https://bugs.webkit.org/show_bug.cgi?id=83490
https://code.google.com/p/chromium/issues/detail?id=89165

@smaug----
Copy link
Author

We might also want to think of some backwards compatible change to push/replaceState which allow target handling (and also hashchange events?).

@beidson
Copy link

beidson commented Feb 8, 2016

While I agree this should have worked, I also agree that since no browser has ever done it, and all major browsers are in agreement on it not working, that this change is the right course of action.

We might also want to think of some backwards compatible change to push/replaceState which allow target handling (and also hashchange events?).

That would also be ideal.

@laughinghan
Copy link

laughinghan commented Apr 13, 2016

(@majido: smaug____ suggested in IRC that I ask you for feedback)

a safe breaking change?

From my understanding, the only reason to be wary of changing a behavior that every browser agrees on is the danger of breaking webpages, right? There's nothing intrinsically bad about changing behavior, the badness is that webpages that used to work may no longer work, which is intrinsically bad.

Frankly, it seem far-fetched to me that fixing this would break a webpage. There are only two ways that could happen, that I can think of:

  1. a :target style that affects layout that a webapp expects to be applied on initial page load but not when the app does .pushState()/.replaceState()
  2. javascript on the page reading a computed style from something that could be selected by a :target selector and making a decision based on the computed style, that is correct on initial page load but not when the app does .pushState()/.replaceState()

There are corpora available of stylesheets used on popular websites, right? We could analyze them to determine whether 1. is true, right?

I doubt it's feasible to automatically determine whether 2. happens, but it also seems even less likely than 1., so supposing that 1. turns out in fact to never happen, could we make this change in Nightlies of each browser, and then maybe Betas, and see if anyone complains?

I'd bet no one does, and the Web would simply be a better place.

backwards-compatibly fixing this

Okay so if you wanna be a real hard-ass about this (or I'm wrong and we find a counterexample), how do authors ask browsers to recompute :target styles without adding to history and/or causing the page to jump?

What if we made the fix a global, page-wide opt-in to minimize pain for web authors, like:

history.recomputeTargetSelectors = true;

Update: Looks like there's precedence for this kind of thing: history.scrollRestoration = 'manual', which has since been integrated into the HTML Living Standard and implemented in both Chrome and Firefox.

Other options include:

  • a 4th argument to .pushState()/.replaceState() (on every call? Blech)
  • updating the CSS selector:
h3:target-including-if-changed-by-pushState-this-should-probs-have-a-better-name {
  background: yellow;
}

Once upon a time we might've linked this to the next DOCTYPE version, but that's out of the question, right?

laughinghan added a commit to mathquill/mathquill.github.com that referenced this issue Apr 13, 2016
In the Beginning, "Community" in the navbar on the homepage was a normal
link, and clicking it would take you to the Community section and
highlight the heading, and the browser Back button would take you back
to your scroll position at the top of the page where the navbar was.

Then I added smooth-scrolling so that when you click "Community" in the
navbar it would smoothly scroll down to the Community heading, and
highlight it. Problem: if you then hit the browser Back button, the
heading highlight would vanish but you would stay in place, rather than
the expected behavior of going back to the scroll position at the top of
the page where you clicked on the navbar.

Now I'm finally fixing that by doing history.pushState():
  https://developer.mozilla.org/en-US/docs/Web/API/History_API
(falling back to the old technique if unavailable)

Of course the web platform sucks so we have to contend with:
  whatwg/html#639
@ghost
Copy link

ghost commented Apr 14, 2016

I agree with @laughinghan and vote for fixing it early than carrying the weight of the bugs until the end of the web.

@laughinghan
Copy link

laughinghan commented Apr 21, 2016

Inspired by a discussion with smaug____ in IRC, I've found a good reason to believe that virtually no webpages would be broken by the "breaking" change of updating :target selectors upon pushState: though they are currently not updated, if you however hit Back and then Fwd again, then :target rules do get applied (http://output.jsbin.com/quludi).

That means that the only way for a webapp to be broken by :target selectors being updated upon pushState, yet function correctly when you hit Back then Fwd, would be if the webapp's popstate/hashchange handlers had some kind of hysteresis that obscured the broken :target CSS. This seems highly unlikely to me. I have difficulty imagining why a popstate/hashchange handler would make a CSS change that depended on the sequence of Backs and Fwds to get to that history state, not to mention how it could obscure a :target rule that would've been broken if it had been applied upon the first .pushState().

@pimterry
Copy link

+1 for fixing this properly - it's very surprising behaviour.

Conveniently @laughinghan's back/fwd find does provide a nice workaround for the current behaviour, which might be of interest to everybody else trying to actually use :target with browser history, and ending up here. You can double-add to the history with pushState and go back immediately to get expected behaviour:

function pushHashAndFixTargetSelector(hash) {
  history.pushState({}, "", hash);

  // Do it again and go back immediately, to force :target to update.
  history.pushState({}, "", hash);
  history.back();
}

This gives the expected result. Only side-effect (I think) is that the user's forward button is no longer disabled, although if they hit it, it won't break anything - it's the exact same URL. (Yes, this is a little nasty - we should fix it in the spec properly).

More discussion on actually using this: https://medium.com/@pimterry/better-css-only-tabs-with-target-7886c88deb75.

@robinbastien
Copy link

@pimterry Ha, thanks! Great workaround. Would be nice if pushState updated :target by default :(

@laughinghan
Copy link

FYI @pimterry @robinbastien I believe you can avoid the odd side effect of the forward button being disabled by instead doing:

function pushHashAndFixTargetSelector(hash) {
  history.pushState({}, "", hash);

  // go back and then come forward again immediately, to force :target to update.
  history.back();
  var onpopstate = window.onpopstate;
  window.onpopstate = function() {
    history.forward();
    window.onpopstate = onpopstate;
  };
}

That's what I did in my original test http://output.jsbin.com/quludi

@asgh
Copy link

asgh commented Apr 25, 2017

When fixing this, please also consider what happens if you use pushState, followed by window.location.hash = 'foo'.

In Firefox this will update the :target, but in Webkit browsers the visible hash changes, but the :target is not affected until there is any kind of non-pushed change to the url.

@nornagon
Copy link

nornagon commented Feb 6, 2022

The CSS Selectors Level 4 spec now says:

Note: The current URL of a page can change as a result of user actions such as activating a link targetting a different fragment within the same page; or by use of the pushState API; as well as by the more obvious actions of navigating to a different page or following a redirect (which could be initiated by protocols such as HTTP, markup instructions such as , or scripting instructions ). UAs must ensure that :local-link, as well as the :target and :target-within pseudo-classes below, respond correctly to all such changes in state.

This seems to me like it is saying that :target should be updated when pushState is called, i.e. the opposite of what the top issue in this thread proposed.

@domenic
Copy link
Member

domenic commented Feb 6, 2022

Yeah, the CSS Selectors spec is just wrong; it's unclear why they say that. They should fix it.

The current HTML spec (which governs how :target behaves, not the CSS spec) already says that :target should not change in reaction to pushState/replaceState, since they don't update the target element. So this issue can be closed.

If you'd like a new pseudo-class that matches the current fragment, and doesn't match the target element (which is only updated in specific conditions), I opened w3c/csswg-drafts#6942 where developers can advocate for that.

@domenic domenic closed this as completed Feb 6, 2022
@nornagon
Copy link

nornagon commented Feb 7, 2022

Ah, disappointing. As currently specced, :target is more or less useless for single-page apps. Since navigating with pushState doesn't update the target, in-app navigations don't correctly highlight targeted elements, requiring apps to implement that functionality in JavaScript. The workarounds described in this issue all have the side-effect that they destroy the browser history in one way or another.

I'm not really sure why it's valuable to not update :target on pushState(), other than that it happens to be what browsers did. As mentioned in this thread, it seems unlikely to cause significant breakage if this behavior were changed.

@jerrygreen
Copy link

How come that history.back() respects :target, while history.pushState() and history.replaceState() don't?

@jerrygreen
Copy link

Btw regarding the workaround, instead of this chunk:

  var onpopstate = window.onpopstate;
  window.onpopstate = function() {
    history.forward();
    window.onpopstate = onpopstate;
  };

You can use addEventListener with "once" option:

  window.addEventListener('popstate', () => { history.forward() }, { once: true } )

Which makes the same but without this onpopstate replacement magic trick. I still don't like this workaround though, I would prefer that history.pushState() and history.replaceState() would respect the :target, by changing it. But I, instead, have to tell the browser to go to my page first, then to go history.back() and history.forward(). Unfortunate...

@Azarattum
Copy link

Unfortunately the workaround isn't viable, since it triggers scroll to an anchored element upon navigation in Safari (works fine in Chrome, though). The whole reason of using history.pushState() instead of just assigning to the location.hash was to avoid page scrolling.

Are there any methods to update the :target elements, but not scroll to them that work across browsers?

@Nintron27
Copy link

Unfortunately the workaround isn't viable, since it triggers scroll to an anchored element upon navigation in Safari (works fine in Chrome, though). The whole reason of using history.pushState() instead of just assigning to the location.hash was to avoid page scrolling.

Are there any methods to update the :target elements, but not scroll to them that work across browsers?

I found for iOS Safari (Atleast 17.2) that if you use an invalid fragment it doesn't scroll, so something like #i-havent-used-this-id-anywhere-on-the-page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests