755116 - Add a way to disable global history on a docshell (while leaving session history active)

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Shane Caraveo (:mixedpuppy)]

moved from https://github.com/mozilla/socialapi-dev/issues/29

Rev 73ecfd1 enables history in the sidebar panel to work around a problem with one of our partners. Apparently, with history disabled, window.history.replaceState exists but throws an exception. Our partner is expecting that if it is disabled window.history or window.history.replaceState should be undefined. We should confirm that, confirm it is a bug in Firefox and re-disable once fixed.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Mark, Can you provide more clarity on this bug?  I don't see any problems with any of our providers if we disable history.

Hi Boris, do you know what the correct behavior of history.replaceState should be when history is disabled in a browser element?  From my testing, you can check for its existence and it appears to exist when history is disabled, calling it causes an exception.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

The problem is with our amigo.  They are attempting to use existing libraries which handle window.history.replaceState not existing, but don't handle it existing but failing.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Hi Boris, do you know what the correct behavior of history.replaceState should be when
> history is disabled in a browser element?

The spec doesn't have a concept of history not existing, so correct behavior is whatever we feel like.  The question is what would break consumers least.

We could just make history.replaceState silently do nothing when history is disabled, but chances are that would still break whatever content this is.

We could make there not be a function, maybe, but that might still break scripts once everyone supports replaceState...

So maybe the only sane solution is to leave history enabled in the sidebar?

Comment 4

[Shane Caraveo (:mixedpuppy)]

The content is expecting history.replaceState to be undefined if it is not available.  I think (but not sure) that is a safe fallback for any content, whether in our special use case or simply regular web content.

The problem with history in our sidebar is that will show up in the urlbar.  The content in the sidebars will be specialized content designed for browser features, not regular web content.  I think it would be best that it is not a part of regular browsing history.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I think (but not sure) that is a safe fallback for any content

For now, because there are browsers out there without replaceState support so content has to worry about that case.  Once all browsers support replaceState, content is likely to just start assuming it exists.

> The problem with history in our sidebar is that will show up in the urlbar. 

That's just broken, imo.  The only reason it doesn't do that with session history disabled is this line in nsDocShell::OnNewURI in the "no session history" case:

            updateGHistory = false; // XXX Why global history too?

How about we leave that, but also add a way to disable global history explicitly on the docshell, so you can set up a session history but not affect the global history?  Should be pretty easy...

Comment 6

[Mark Hammond [:markh] [:mhammond]]

I asked our partner about the possibility of changing their libraries.  FWIW, the response was:

"""
The scope of changing these libraries is pretty large, but the most
concerning issue is that any number of [our] devs could come along and break
our sidebar by making a call to window.history anywhere in [our] libraries
that are loaded/used by the sidebar and expecting that if window.history
is available, replaceState should be callable. That said, let me check
with the folks who actually wrote that piece of logic for feedback and get
back to you on this.
"""

Comment 7

[Boris Zbarsky [:bzbarsky]]

Right.  Hence comment 5.

If that sounds like a reasonable plan from your side, let me know!

Comment 8

[Mark Hammond [:markh] [:mhammond]]

That sounds fine to me - all we need is global history disabled.

Comment 9

[Justin Lebar (not reading bugmail)]

Over to core -> document navigation.  I have no idea where this social code lives, but presumably if we give you a method on docshell, that's good enough?

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'd think so, if right now they can either set up a session history or not....

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, and Justin, if you plan to patch, let me know.  If you'd prefer to review, likewise let me know, and then I'll patch.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Justin Lebar [:jlebar] from comment #9)
> Over to core -> document navigation.  I have no idea where this social code
> lives, but presumably if we give you a method on docshell, that's good
> enough?

That would be enough, although I guess it would be nice if there was a XUL attribute 'disableglobalhistory' or similar to match the existing 'disablehistory' attribute which we were using before our partner hit the problem.

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to Boris Zbarsky (:bz) from comment #11)
> If you'd prefer to
> review, likewise let me know, and then I'll patch.

It sounds like you've already looked at the code; please, be my guest.  :)

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK.  Patch coming up tomorrow.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Oh, we even had an API for this already.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: This should do the trick

Mark, let me know if this doesn't do what you want?

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Thanks!  FWIW, something has changed on the partner's side so their code no longer seems to hit the problem.  However, I did manage to hack some code up that called the history stuff in the same environment and it works fine with the 'disableglobalhistory' attribute...

But looking at the other users of this attribute in mozilla-central, I wonder if we shouldn't take a simpler approach.  IIUC, 'disablehistory' currently has 2 main effects:
1) global history isn't enabled.
2) history related functions throw exceptions due to sessionHistory not having been set.

Most existing users just want (1) - nobody relies on (2) - they just never call the history functions.  So instead of adding yet another attribute, I wonder if 'disablehistory' shouldn't simply act as the new 'disableglobalhistory'?  This would mean there is no way of indicating no session history is available, but is there really a use-case for that?

Comment 18

[Boris Zbarsky [:bzbarsky]]

There are other user-visible effects of disabling session history.  Specifically, the fact that the user can't use the UI to traverse the session history.

Comment 19

[Boris Zbarsky [:bzbarsky]]

In any case, this is clearly not a document navigation issue; this is all pure toolkit code...

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 625257 [details] [diff] [review]
This should do the trick

Gavin, you seem to be a good candidate to review this patch from bz.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 625257 [details] [diff] [review]
This should do the trick

This interaction between disablehistory/disableglobalhistory is a little odd, but I'm not sure it's worth complicating things further by making more invasive changes. Let's just make sure https://developer.mozilla.org/en/XUL/browser is updated accordingly.

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Pushed to inbound as rev 94922:201257e86edf

Comment 23

[Mark Hammond [:markh] [:mhammond]]

Added docs to MDN for disableglobalhistory and opened bug 758511 about the docs for disablehistory being totally wrong (it is for a different element) and me not knowing how to resolve that.

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/201257e86edf