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

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Creating/Changing Bugs
P1
enhancement
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Guy Pyrzak)

Tracking

unspecified
Bugzilla 4.2
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 7 obsolete attachments)

1.20 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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

8 years ago
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.
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
(Reporter)

Comment 3

8 years ago
(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

8 years ago
  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?
Severity: normal → enhancement
Priority: -- → P1
Summary: Use history.replaceState() so that the URL after processing a bug isn't process_bug.cgi → Use history.replaceState() so that the URL after processing a bug isn't process_bug.cgi or post_bug.cgi
Target Milestone: --- → Bugzilla 4.2
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
(Assignee)

Comment 6

8 years ago
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

8 years ago
(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.

Updated

8 years ago
Summary: Use history.replaceState() so that the URL after processing a bug isn't process_bug.cgi or post_bug.cgi → Use history.replaceState() so that the URL after processing a bug isn't process_bug.cgi, post_bug.cgi, or attachment.cgi

Comment 8

8 years ago
  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.
(Assignee)

Comment 9

8 years ago
Created attachment 456633 [details] [diff] [review]
v1
Attachment #456633 - Flags: review?(mkanat)
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

8 years ago
(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

8 years ago
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?
Attachment #456633 - Flags: review?(mkanat) → review-

Updated

8 years ago
Assignee: create-and-change → guy.pyrzak
(Assignee)

Comment 13

8 years ago
Created attachment 456648 [details] [diff] [review]
v2
Attachment #456633 - Attachment is obsolete: true
Attachment #456648 - Flags: review?(mkanat)
(Assignee)

Comment 14

8 years ago
fixed the concerns as well as made sure it wasn't too long in length.
(Assignee)

Comment 15

8 years ago
Created attachment 456651 [details] [diff] [review]
V3

in a block now
Attachment #456648 - Attachment is obsolete: true
Attachment #456651 - Flags: review?(mkanat)
Attachment #456648 - Flags: review?(mkanat)
(Reporter)

Comment 16

8 years ago
(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

8 years ago
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.
(Reporter)

Comment 18

8 years ago
(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

8 years ago
(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.
(Reporter)

Comment 20

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

Comment 21

8 years ago
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=
Attachment #456651 - Flags: review?(mkanat) → review-
(Reporter)

Comment 22

8 years ago
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.
(Assignee)

Comment 23

8 years ago
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.
(Reporter)

Comment 24

8 years ago
(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

8 years ago
  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.
(Reporter)

Comment 26

8 years ago
(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.
(Assignee)

Comment 27

7 years ago
Created attachment 465855 [details] [diff] [review]
v4
Attachment #456651 - Attachment is obsolete: true
Attachment #465855 - Flags: review?(mkanat)
(Reporter)

Comment 28

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

Comment 29

7 years ago
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

7 years ago
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.
Attachment #465855 - Flags: review?(mkanat) → review-
(Reporter)

Comment 31

7 years ago
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

7 years ago
@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?
(Reporter)

Comment 33

7 years ago
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?
(Assignee)

Comment 34

7 years ago
Created attachment 465896 [details] [diff] [review]
4.1

feel free to not approve etc but I think this has what you want.
Attachment #465896 - Flags: review?(mkanat)

Comment 35

7 years ago
Comment on attachment 465896 [details] [diff] [review]
4.1

Yeah, the upside is way better than the downside, here. :-)
Attachment #465896 - Flags: review?(mkanat) → review+

Updated

7 years ago
Flags: approval+
(Reporter)

Comment 36

7 years ago
Don't you need to set document.title?

Comment 37

7 years ago
Well, not according to the spec.... ;-)
(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 = "...";).
(Reporter)

Comment 39

7 years ago
(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

7 years ago
Ah, okay. pyrzak, do you want to submit another patch that updates document.title also?
(Assignee)

Comment 41

7 years ago
sure. why not.
(Assignee)

Comment 42

7 years ago
Created attachment 466358 [details] [diff] [review]
4.1 with document.title = new Title
Attachment #465855 - Attachment is obsolete: true
Attachment #465896 - Attachment is obsolete: true
Attachment #466358 - Flags: review?(mkanat)

Updated

7 years ago
Attachment #466358 - Flags: review?(mkanat) → review+
(Assignee)

Updated

7 years ago
Depends on: 588483
(Assignee)

Comment 43

7 years ago
can't commit this until bug 588483 is fixed.
(Assignee)

Updated

7 years ago
Flags: approval+

Updated

7 years ago
Flags: approval+

Updated

7 years ago
Keywords: 4xp, relnote

Updated

7 years ago
Keywords: 4xp

Comment 44

7 years ago
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 &ndash; &quot; 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.
Attachment #466358 - Flags: review-

Updated

7 years ago
Flags: approval+
(Assignee)

Comment 45

7 years ago
Created attachment 480354 [details] [diff] [review]
filtered fixed
Attachment #466358 - Attachment is obsolete: true
Attachment #480354 - Flags: review?(mkanat)
Attachment #480354 - Flags: review?(LpSolit)

Comment 46

7 years ago
Comment on attachment 480354 [details] [diff] [review]
filtered fixed

I'm pretty sure the second one shouldn't have an &ndash; in it, right?
Attachment #480354 - Flags: review?(mkanat)
Attachment #480354 - Flags: review?(LpSolit)
Attachment #480354 - Flags: review-
(Assignee)

Comment 47

7 years ago
Created attachment 480458 [details] [diff] [review]
fixed escaped endash
Attachment #480354 - Attachment is obsolete: true
Attachment #480458 - Flags: review?(mkanat)
Attachment #480458 - Flags: review?(LpSolit)

Comment 48

7 years ago
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.
Attachment #480458 - Flags: review?(mkanat)
Attachment #480458 - Flags: review?(LpSolit)
Attachment #480458 - Flags: review+

Updated

7 years ago
Flags: approval+
(Assignee)

Comment 49

7 years ago
Committing to: bzr+ssh://guy.pyrzak%40gmail.com@bzr.mozilla.org/bugzilla/trunk/                             
modified template/en/default/bug/show-header.html.tmpl
Committed revision 7514.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 50

7 years ago
(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.

Updated

7 years ago
Duplicate of this bug: 648152
Duplicate of this bug: 651797

Comment 53

7 years ago
Note that this bug is responsible for bug 652748.
Blocks: 652748
Blocks: 657222

Comment 54

6 years ago
Added to relnotes in bug 713346.
Keywords: relnote

Updated

5 years ago
Duplicate of this bug: 471794

Updated

3 years ago
Duplicate of this bug: 1148061
You need to log in before you can comment on or make changes to this bug.