577720 - Use history.replaceState() so that the URL after processing a bug isn't process_bug.cgi, post_bug.cgi, or attachment.cgi

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement, P1)

Description

[Justin Lebar (not reading bugmail)]

As discussed at http://etherpad.mozilla.com:9000/kwpLzwwtbz , we'd like URL after processing a bug not to be process_bug.cgi but to be the regular bug URL.

We could use history.replaceState() to accomplish this pretty easily.

Comment 1

[Max Kanat-Alexander]

Note that this function doesn't exist until Firefox 4 according to the docs.

However, it's possible the YUI History Manager could do it now.

Comment 2

[Gervase Markham [:gerv]]

The fact that bug submission leads to process_bug.cgi rather than show_bug.cgi is a long-standing issue (bug 427913, bug 365078). It's not simple to solve.

Basically, this bug's suggestion to fix it involves adding something like the following code to the output of process_bug.cgi:

<script>
if (history.replaceState) {
  history.replaceState(null, null, "show_bug.cgi?id=[% bug.id %]")
}
</script>

This fixes the problem in Firefox, at very low cost and with no need to rearchitecture.

Gerv

Comment 3

[Justin Lebar (not reading bugmail)]

(In reply to comment #1)
> Note that this function doesn't exist until Firefox 4 according to the docs.

FWIW, the latest versions of Chrome and Safari have both shipped push/replaceState.

Comment 4

[Max Kanat-Alexander]

  Okay. So this does sound like a great and simple way to handle this problem.

  Is this part of the HTML5 spec still in flux? Aren't there security concerns about modifying the URL in the title bar without modifying the actual page being displayed?

Comment 5

[Gervase Markham [:gerv]]

I suspect if two browsers have implemented it, it's stable enough for us to use.

The security concerns are addressed by the fact that this call raises an exception if the same-origin policy is violated. That is to say, you can only change the URL to other URLs in the same domain.

(You could do something similar by redirecting to "#foo" in older browsers. This just extends a bit the parts of the URL you can hack.)

Gerv

Comment 6

[Guy Pyrzak]

Make sure when this is implemented that it also handled attachments as well as bug saves. Obviously this won't work for people who have JS turned off. 

Do we know how this works with bookmarks etc?

Comment 7

[Max Kanat-Alexander]

(In reply to comment #5)
> I suspect if two browsers have implemented it, it's stable enough for us to
> use.

  Well, I wouldn't be sure about that until it's in a CR, based on the degree to which other aspects of HTML5 have fluxed.

> The security concerns are addressed by the fact that this call raises an
> exception if the same-origin policy is violated. That is to say, you can only
> change the URL to other URLs in the same domain.

  Okay. :-) I just wanted to be sure that there wasn't going to be some future security restriction that prevented us from doing what we want to do.

Comment 8

[Max Kanat-Alexander]

  BTW, If somebody does this relatively soon, and it's really just those three lines of code that could go into show-header, then I'd be happy to take it for 4.0.

Comment 9

[Guy Pyrzak]

Attachment: v1

Comment 10

[Gervase Markham [:gerv]]

Comment on attachment 456633 [details] [diff] [review]
v1

That's wrong in several ways :-)

First, it should be matching process_bug.cgi, not show_bug.cgi.

Then, how do you know that $javascript is semicolon-terminated? I ask particularly because it's no longer semicolon-terminated after you've finished with it! So how are you relying on everyone else to do the right thing?

Lastly, we need to do it for a number of different URLs, as Max indicates. 

I'm not quite sure why Max said it should be in the header, as it's specific to particular CGIs. Wouldn't it make more sense to have it in the main template served for that CGI? OK, we'd need a few copies of it with different target URLs, but it's only one line of code.

Did you check that '' is a valid value for the first parameter? It wasn't immediately clear to me from the docs...

$title will be "Bug processed"; it would be much better if the title set were the same title that the bug normally has...

Gerv

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> First, it should be matching process_bug.cgi, not show_bug.cgi.

  No, actually he's correct there. This is in show-header, which sets up header.html.tmpl for any time that edit.html.tmpl is going to be included.

> Then, how do you know that $javascript is semicolon-terminated?

  That's not necessary because it doesn't already exist at this point.

> Lastly, we need to do it for a number of different URLs, as Max indicates. 

  show-header handles all those, so it's OK.

> Did you check that '' is a valid value for the first parameter? It wasn't
> immediately clear to me from the docs...

  This is something we should confirm, and probably the MDC docs should be clarified if it's not clear.

> $title will be "Bug processed"; it would be much better if the title set were
> the same title that the bug normally has...

  I think that might be better as a more general change to the templates.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 456633 [details] [diff] [review]
v1

>=== modified file 'template/en/default/bug/show-header.html.tmpl'

>+  [% javascript =  "$javascript window.history.replaceState('','$title', 'show_bug.cgi?id=$bug.bug_id' )" %]

  Are you using $javascript because calling templates use it? Perhaps you should prefix your code to $javascript instead, if so. (I see what Gerv was talking about now, sorry Gerv.)

  You need to do FILTER js on $title before inserting it into the string, or there could be script injections.

  Also, this needs to only be done "if window.history.replaceState". Also, are you sure it's "window" that you want? Theoretically we could be in a frame, no?

Comment 13

[Guy Pyrzak]

Attachment: v2

Comment 14

[Guy Pyrzak]

fixed the concerns as well as made sure it wasn't too long in length.

Comment 15

[Guy Pyrzak]

Attachment: V3

in a block now

Comment 16

[Justin Lebar (not reading bugmail)]

(In reply to comment #6)
> Do we know how this works with bookmarks etc?

If you can't bookmark the page after calling replaceState, or if the URL of the bookmark doesn't reflect the URL you set by calling replaceState, that's a bug.

(In reply to comment #11)
> Did you check that '' is a valid value for the first parameter? It wasn't
> immediately clear to me from the docs...

Any object which can be passed to JSON.stringify works in Firefox.  The spec's requirement is even looser.  So certainly '' works.  Also null should work.

Comment 17

[Frédéric Buclin]

Bah, could you please dupe this bug when you know it's a dupe? See comment 2. There are 24 people missing this discussion because you keep discussing in this bug instead of the original ones.

Comment 18

[Justin Lebar (not reading bugmail)]

(In reply to comment #17)
> Bah, could you please dupe this bug when you know it's a dupe? See comment 2.
> There are 24 people missing this discussion because you keep discussing in this
> bug instead of the original ones.

I commented in both of those bugs directing them here.  I don't think this is strictly a dupe, since it only solves the problem on browsers which support this API.

Comment 19

[Chris Lawson (gone)]

(In reply to comment #1)
> Note that this function doesn't exist until Firefox 4 according to the docs.
> 
> However, it's possible the YUI History Manager could do it now.

Will this work in any Gecko browsers prior to Firefox 4 (which, AIUI, is now coming from Gecko 1.9.3+; please correct me if I'm wrong there)?

If not, we still need a proper solution for bug 427913.

Comment 20

[Justin Lebar (not reading bugmail)]

(In reply to comment #19)
> Will this work in any Gecko browsers prior to Firefox 4?
No.

Comment 21

[Max Kanat-Alexander]

Comment on attachment 456651 [details] [diff] [review]
V3

>=== modified file 'template/en/default/bug/show-header.html.tmpl'
>+    if( history && history.replaceState ) {
>+      var null_state = {};
>+      history.replaceState( null_state , 

  I'm pretty sure you can just literally pass "null" for that first argument.

>+                          "show_bug.cgi?id=[% bug.bug_id FILTER js %]" );

  I think you also need to wrap the whole block in an "IF bug.defined" because otherwise, when updating a bug where there is no next item in the list, I get a URL like:

  https://landfill.bugzilla.org/mkanat/show_bug.cgi?id=

Comment 22

[Justin Lebar (not reading bugmail)]

I just realized that even with this change, if you submit a change to a bug and then refresh the page, you'll probably still get a warning that you're refreshing a POST'ed page.

This might be a bit confusing, since it will appear from the URL that we didn't POST to the page.  But it's probably not a big deal.

Comment 23

[Guy Pyrzak]

You do get a warning. It is confusing, I don't know anything about the whole "replaceState" capability so if you can tell me how to fix it I'd be happy to include it in the patch.

The best part about this addition is that bookmarking works and refresh doesn't become totally ridiculous.

At least it doesn't try to resubmit your changes.

Comment 24

[Justin Lebar (not reading bugmail)]

(In reply to comment #23)
> You do get a warning. It is confusing, I don't know anything about the whole
> "replaceState" capability so if you can tell me how to fix it I'd be happy to
> include it in the patch.
push/replaceState doesn't help you get around the warning.  My understanding is that you get the warning when you refresh a document that was POST'ed to.  Using replaceState to change the URL doesn't change the fact that you POST'ed to process_bug.cgi.

> At least it doesn't try to resubmit your changes.
It doesn't?  Isn't that what the warning is about?  Or does Bugzilla have a way to make the "Save Changes" POST idempotent?

Comment 25

[Max Kanat-Alexander]

  I'd say that that POST issue is a bug (or deficiency) in the History API or the browser, probably, unless there is some good and reasonable way to work around it.

Comment 26

[Justin Lebar (not reading bugmail)]

(In reply to comment #25)
>   I'd say that that POST issue is a bug (or deficiency) in the History API or
> the browser, probably, unless there is some good and reasonable way to work
> around it.

Yeah, the more I think about it, I think this might be a spec bug.  I'll bring it up with WhatWG.

Comment 27

[Guy Pyrzak]

Attachment: v4

Comment 28

[Justin Lebar (not reading bugmail)]

See also bug 580069, which I'm going to check in soon (though not for FF4b4).  This should fix the refresh problems.

Comment 29

[Justin Lebar (not reading bugmail)]

Note also that the title parameter to replaceState is completely ignored (per bug 544535), so if you want to change document.title, you'll have to set it explicitly.  I imagine you do want to change the title here so it's not "Bug N was processed".

Comment 30

[Max Kanat-Alexander]

Comment on attachment 465855 [details] [diff] [review]
v4

>+        history.replaceState( "null", 

  Why are you passing the string "null" instead of an actual null?

>+                           "[% title FILTER js %]",

  It sounds like the API of pushState may be changing too rapidly for us to use it at the moment, if the title argument is going away or being ignored. I suppose we have to decide what to do here about the title, and I'd like to see some decision from the WhatWG about it.

Comment 31

[Justin Lebar (not reading bugmail)]

I'd either pass the empty string as the title or pass what you want document.title to be as the title.

Since Chrome and Safari have both shipped replaceState and sites are already using it, the API isn't going to change.  I think it's absolutely stable enough to use.

There's been some talk about using the title parameter to change document.title, but I don't think it's going to happen at this point.  Even if it does, passing the either empty string or the title you actually want will be OK.

Comment 32

[Max Kanat-Alexander]

@jlebar Okay, I read over the bit about the title parameter, and I think it's unfortunate, because instead of fixing that problem, browser implementors are effectively pushing it off onto every single consumer of replaceState, no? What are we supposed to do with the problem of title and back/forward?

Comment 33

[Justin Lebar (not reading bugmail)]

I'd love to talk about the title parameter, but I don't think this bug is the place to do it.  How about bug 585653?

Comment 34

[Guy Pyrzak]

Attachment: 4.1

feel free to not approve etc but I think this has what you want.

Comment 35

[Max Kanat-Alexander]

Comment on attachment 465896 [details] [diff] [review]
4.1

Yeah, the upside is way better than the downside, here. :-)

Comment 36

[Justin Lebar (not reading bugmail)]

Don't you need to set document.title?

Comment 37

[Max Kanat-Alexander]

Well, not according to the spec.... ;-)

Comment 38

[Matt Brubeck (:mbrubeck)]

(In reply to comment #37)
>> Don't you need to set document.title?
> Well, not according to the spec.... ;-)

You can still set the title the old-fashioned way (document.title = "...";).

Comment 39

[Justin Lebar (not reading bugmail)]

(In reply to comment #37)
> Well, not according to the spec.... ;-)

No, actually.  See [1]: "The title is purely advisory. User agents might use the title in the user interface."  Ignoring it isn't optimal, but it's definitely compliant with the spec.  And certainly the spec doesn't suggest that the title argument is linked to document.title.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#dom-history-pushstate

Comment 40

[Max Kanat-Alexander]

Ah, okay. pyrzak, do you want to submit another patch that updates document.title also?

Comment 41

[Guy Pyrzak]

sure. why not.

Comment 42

[Guy Pyrzak]

Attachment: 4.1 with document.title = new Title

Comment 43

[Guy Pyrzak]

can't commit this until bug 588483 is fixed.

Comment 44

[Frédéric Buclin]

Comment on attachment 466358 [details] [diff] [review]
4.1 with document.title = new Title

>+    [% javascript =  BLOCK %]
>+      if( history && history.replaceState ) {
>+        history.replaceState( null, 
>+                           "[% title FILTER js %]",  
>+                           "show_bug.cgi?id=[% bug.bug_id FILTER js %]" );
>+        document.title = "[% title FILTER js %]";

The filtering of the title is wrong. HTML entities are all escaped, so you e.g. see – " instead of - and ".


>+      [% javascript FILTER none %]
>+    [% END %]

Is it intentional that [% javascript %] is *inside* [% javascript =  BLOCK %]? This looks wrong to me. Also, please fix the indentation.

Comment 45

[Guy Pyrzak]

Attachment: filtered fixed

Comment 46

[Max Kanat-Alexander]

Comment on attachment 480354 [details] [diff] [review]
filtered fixed

I'm pretty sure the second one shouldn't have an – in it, right?

Comment 47

[Guy Pyrzak]

Attachment: fixed escaped endash

Comment 48

[Max Kanat-Alexander]

Comment on attachment 480458 [details] [diff] [review]
fixed escaped endash

Nice!

The POST resubmission problem still exists, of course, but that's a browser-side issue.

Comment 49

[Guy Pyrzak]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                             
modified template/en/default/bug/show-header.html.tmpl
Committed revision 7514.

Comment 50

[Justin Lebar (not reading bugmail)]

(In reply to comment #48)
> The POST resubmission problem still exists, of course, but that's a
> browser-side issue.

We fixed that in bug 580069.  Hopefully WebKit will follow suit.

Comment 53

[Frédéric Buclin]

Note that this bug is responsible for bug 652748.

Comment 54

[Frédéric Buclin]

Added to relnotes in bug 713346.