674416 - Custom searches in query.cgi are lost when clicking the "Back" button in Firefox when viewing a buglist

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Frédéric Buclin]

Tested with Firefox 6 and Chrome 12:

In query.cgi, I set the custom searches (aka boolean charts) to:

1) assignee is equal to LpSolit
and
2) classification is not equal to foo

The buglist returns zero bugs, because I didn't pass my full email address. So I click the "Back" button in my browser and realize that my custom searches are gone. This of course should not happen.

Comment 1

[Max Kanat-Alexander]

Yes, I believe this is a dupe, actually. We have a comment from Brendan on the dupe explaining that Firefox has a bug that is preventing its bfcache from working on pages that lead to a multi-part response (like buglist.cgi). He has patched it and it should be in Firefox 7 I believe.

Comment 2

[Max Kanat-Alexander]

Chrome, on the other hand, does not have a bfcache. It would require some rather complex code to make this work on Chrome, but it could be possible with history.replaceState. But it is a pretty significant amount of work, I'm pretty sure.

Comment 3

[Max Kanat-Alexander]

Pyrzak, do you have any thoughts on how we could manage this? If possible, I was thinking that when we view buglist.cgi, if the previous page is query.cgi (or some other search page) we could insert the Edit Search URI as the "back" URI, and make the browser reload the page that way.

I'm trying to avoid duplicating the URI-parsing and initial-setup logic (the stuff that's currently in the templates) of Custom Search in JS.

Comment 4

[Max Kanat-Alexander]

So, I'm wondering if I can do this by setting window.location during onpopstate for modern browsers. I think it might be possible.

Comment 5

[Max Kanat-Alexander]

  I've been doing research and attempting to write various forms of a Proof Of Concept today for this. This turns out to be a very hard problem to solve (although it would be very easy to simply rely on all browsers' page/bf caches, if they all had one--sadly Chrome does not and we break Firefox's in a way that won't be fixed until FF7).

  The problems are this:

  * This must work both when the user goes Back from the buglist page, and 
    also when they come Forward after having gone Back from the query page.

  * Using the window.history API for this is unreliable at best, and totally
    broken at worst. window.onpopstate fires under different circumstances in
    different browsers, and although it would certainly work in some situations
    when going Back, there's no conceivable way to make it work when going
    Forward.

  * I could update the query.cgi URI using replaceState as we build the custom
    search UI on the client, but that would be somewhat complex, and I would
    then also have to implement a method of reading the URI when we go 
    back/forward and re-implement the entire logic of building the UI from
    query-string variables on the client side. It's very complex--possibly more
    complex than the whole patch implementing the Custom Search UI was in the
    first place.

  * One solution would be to disable building the UI in JS for browsers that
    lack a bfcache or where the bfcache is broken for our case, but (a) we
    can't detect that and (b) it would be a significantly worse user experience
    than generating the UI without reloading the page.

Comment 6

[Max Kanat-Alexander]

Okay, I do have one thought. If there's some way to use JS tell the browser that the current page should *not* be loaded from cache if it is ever loaded again in Back/Forward, but should instead be reloaded using its URL, we could use replaceState and build the URL, and then not have to parse it out.

Comment 7

[Max Kanat-Alexander]

Attachment: v1

Okay, here is the solution!! It only works in browsers that support history.replaceState, but that's Firefox 4 and later, and the current version of Chrome as well, which are the only two browsers that are actually going to be affected by this bug in any case (as everything else has a working page cache that will actually cache the DOM, even IE 6, I believe).

Basically, the solution is that every time somebody changes something in the Custom Search part of the form, we change the query string of the *current page* using history.replaceState to reflect that change, since query.cgi takes the same arguments as buglist.cgi! Then when the browser navigates through history, it reloads the page! Tested on Chrome and Firefox 4.

Comment 8

[Max Kanat-Alexander]

Attachment: v2

LpSolit pointed out (correctly) that the previous patch would cause bad errors on browsers that didn't support history.replaceState.

Comment 9

[:glob ✱]

Comment on attachment 557691 [details] [diff] [review]
v2

tested via:

searched for:
"assignee" "is equal to" "foo" + 
"attachment description" "is not equal to" "bar"
search, back

IE 8:

"assignee" "is equal to" "foo"  restored
"attachment description" "is not equal to" "bar" not restored

opera 11.590:

everything except "attachment description" restored

firefox 6.01 and 8.0a2:

working.

chrome:

working.


this needs to work on IE too, r-

Comment 10

[Max Kanat-Alexander]

Comment on attachment 557691 [details] [diff] [review]
v2

Cool, thanks for the testing.

Internet Explorer 8 does not support the HTML5 History API. As such, this patch will not affect any behavior in Internet Explorer 8. As far as I can think, there's no *reasonable* way to support this behavior in IE 8. There is a *stupid* way: I could update window.location.hash every time, which would create a new history entry for every single onchange event fired by the custom search UI.

Unfortunately, IE 9 presently represents only a small fraction of IE users. So for now, what I'd like to do is make this bug focus on the handling for HTML5-supporting browsers, and file another bug for IE 8 and below.

Comment 11

[:glob ✱]

(In reply to Max Kanat-Alexander from comment #10)
> Internet Explorer 8 does not support the HTML5 History API. As such, this
> patch will not affect any behavior in Internet Explorer 8.

note in comment 7 you also said that IE shouldn't be affected by this issue in the first place.

> So for now, what I'd like to do is make this bug focus on the handling for
> HTML5-supporting browsers, and file another bug for IE 8 and below.

filed as bug 686422

Comment 12

[:glob ✱]

Comment on attachment 557691 [details] [diff] [review]
v2

r=glob

Comment 13

[Max Kanat-Alexander]

> note in comment 7 you also said that IE shouldn't be affected by this issue
> in the first place.

  Yeah. :-( I thought it had a bfcache.

Comment 14

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified js/custom-search.js
modified template/en/default/search/boolean-charts.html.tmpl
Committed revision 7981.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified js/custom-search.js
modified template/en/default/search/boolean-charts.html.tmpl
Committed revision 7945.