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

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[:glob ✱]

+++ This bug was initially created as a clone of Bug #674416 +++

Tested with IE 8:

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

[:glob ✱]

given bug 674416 is a 4.2 blocker, this should also be a blocker.

Comment 2

[Frédéric Buclin]

This is really weird to file two separate bugs for two distinct browsers.

Comment 3

[Max Kanat-Alexander]

The problem is that IE doesn't have a bfcache and also doesn't support history.replaceState (even IE 9). Thus there might have to be a very unpleasant workaround for IE, or we might decide to simply inform people that their stuff is lost and that they should use "Edit Search" instead.

Comment 4

[Teemu Mannermaa (:wicked)]

Looks like IE 10 will have history.replaceState but we could use hashes/onhashchange event for all HTML 4 browsers to implement this (or use some JS library that already does history backward compatible way for us).

Comment 5

[:glob ✱]

Attachment: patch v1

here's a hack which fixes this for IE.

it uses history.js to emulate history.replaceState in html4 browsers.

essentially as the custom fields are added, the location is changed to:
> query.cgi#query.cgi?field=value&field=value...
note the query_string is being stored in the url's hash.

when you navigate back on IE (or any other html4 browser), the onLoad event is fired, which checks if the location's hash contains this full query_string.  if there's a match, it redirects the current page to the url from the hash:
> query.cgi?field=value&field=value...

this dance only plays out when you use custom fields on IE, otherwise it's business as usual.

Comment 6

[Teemu Mannermaa (:wicked)]

Comment on attachment 575844 [details] [diff] [review]
patch v1

This seems to add non-MPL code so I'd like to know if this "New BSD License" is compatible and can be used. Also, do we need to include the README.md file since it's useless for technicaly purposes. I believe gerv is our expert in this area. :)

Comment 7

[:glob ✱]

i included the readme file to identify the code -- there's no product name/url/version in either the license file or the minified javascript.

Comment 8

[Gervase Markham [:gerv]]

There is no legal problem with us using code under new BSD. But I don't think it's something Bugzilla has done before and so I think justdave or Max or LpSolit should sign off on the principle of using non-MPLed code.

Gerv

Comment 9

[Max Kanat-Alexander]

Comment on attachment 575844 [details] [diff] [review]
patch v1

Review of attachment 575844 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/custom-search.js
@@ +144,5 @@
>          return;
>  
>      var form = YAHOO.util.Dom.getAncestorByTagName(form_member, 'form');
>      var query = YAHOO.util.Connect.setForm(form);
> +    window.History.replaceState(null, document.title, '?' + query);

Ah, no--window.history is the DOM-standard object. I would imagine that on most browsers, if window.History exists, it's a type and not an object.

::: js/history.js/README.md
@@ +1,1 @@
> +Welcome to History.js (v1.7.1 - October 4 2011)

Interesting. Why did you pick this instead of the YUI Browser History Manager?

I don't need this README checked in unless the license requires it.

I'm fine with BSD-licensed code--YUI has a license much like BSD, and we already ship with it.

::: template/en/default/search/search-advanced.html.tmpl
@@ +36,4 @@
>  
>  [% PROCESS global/header.html.tmpl
>    title = "Search for $terms.bugs"
> +  onload = "doOnSelectProduct(0); historyHack()"

I don't see a method named historyHack anywhere in this patch.

Comment 10

[Max Kanat-Alexander]

Also, note that the boolean charts are used in many templates; not just search-advanced.

Comment 11

[:glob ✱]

(In reply to Max Kanat-Alexander from comment #9)
> Ah, no--window.history is the DOM-standard object. I would imagine that on
> most browsers, if window.History exists, it's a type and not an object.

it's correct -- window.History is created by history.js.
please have a read of the README file :)

> Interesting. Why did you pick this instead of the YUI Browser History
> Manager?

yui history would require reasonable refactoring of the custom search code. history.js provides a mechanism which uses html5's replaceState, and degrades automatically for html4 browsers with very minimal effort.

> I don't need this README checked in unless the license requires it.

ok, but i'd like to at least include some sort of readme file as a pointer for developers who have to work on this code later.  all it needs to say is:
> History.js (v1.7.1 - October 4 2011)
> https://github.com/balupton/history.js

> ::: template/en/default/search/search-advanced.html.tmpl
> @@ +36,4 @@
> >  
> >  [% PROCESS global/header.html.tmpl
> >    title = "Search for $terms.bugs"
> > +  onload = "doOnSelectProduct(0); historyHack()"
> 
> I don't see a method named historyHack anywhere in this patch.

oops, that should be redirect_html4_browsers()

> Also, note that the boolean charts are used in many templates; not just search-advanced.

noted, will fix.

Comment 12

[Max Kanat-Alexander]

(In reply to Byron Jones ‹:glob› from comment #11)
> (In reply to Max Kanat-Alexander from comment #9)
> > Ah, no--window.history is the DOM-standard object. I would imagine that on
> > most browsers, if window.History exists, it's a type and not an object.
> 
> it's correct -- window.History is created by history.js.
> please have a read of the README file :)

  Ah, okay. So it will use the normal HTML5 method for browsers that support it? In that case, just add a somewhat-detailed comment there explaining why you use History so others don't get confused.

Comment 13

[:glob ✱]

Attachment: patch v2

Comment 14

[Max Kanat-Alexander]

Comment on attachment 576869 [details] [diff] [review]
patch v2

Review of attachment 576869 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but here's some stuff to fix on checkin:

::: js/custom-search.js
@@ +142,5 @@
> +//
> +// Browsers that do not support HTML5's history.replaceState gain an emulated
> +// version via history.js (as window.History), with the full query_string in
> +// the location's hash. We rewrite the URL to include the hash and redirect to
> +// the new location, which causes custom search fields to work.

I'd rather see a comment right above the use of window.History saying simply something like:

// This comes from history.js. It falls back to the normal window.History object for HTML5 browsers.

::: template/en/default/search/boolean-charts.html.tmpl
@@ +76,5 @@
>    </script>
> +  <script type="text/javascript" src="[% 'js/custom-search.js' FILTER mtime %]"></script>
> +  <script type="text/javascript" src="[% 'js/history.js/native.history.js' FILTER mtime %]"></script>
> +  <script type="text/javascript">
> +    YAHOO.util.Event.on(window, 'load', function() { redirect_html4_browsers() });

Ah, don't use window.load. Normally we'd use OnDomReady, but why not just actually call it immediately, here? I don't see any reason to let the page continue loading.

Also, I suspect you can just pass "redirect_html_browsers" and that you don't need to create a closure.

Comment 15

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified js/custom-search.js
added js/history.js
added js/history.js/license.txt
added js/history.js/native.history.js
added js/history.js/readme.txt
modified template/en/default/search/boolean-charts.html.tmpl
Committed revision 7966.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified js/custom-search.js
added js/history.js
added js/history.js/license.txt
added js/history.js/native.history.js
added js/history.js/readme.txt
modified template/en/default/search/boolean-charts.html.tmpl
Committed revision 8018.