1090727 - support jquery in the global/header template, and update the header and footer to use jquery

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[:glob ✱]

the global/header template needs updating to support loading jquery.

rough notes:
- load both jquery and yui2 on every page by default
- new header param "jqlibs" (or "jslibs"?)
  - initially support for jquery-ui only (loads smoothness css + js)
- new header param "noyui"
  - so pages can opt out of loading yui2
  - at some stage we need to invert this, so pages need to opt into yui2
- convert header, footer, and all dependent templates/js to jquery
  - required in order for pages to opt-out
  - yui hacks, such as onpagehide, can be removed if "noyui" is set
  - to simplify testing and reviewing avoid refactoring of code, a straight port is most useful

Comment 1

[David Lawrence [:dkl]]

Attachment: 1090727_1.patch

Working proof of concept of incorporating latest jQuery along side YUI in the Bugzilla code. I have converted the JS that is present in header/footer to JQuery such as js/global.js, etc. I am sure we can improve on what I have here so far but wanted to get it up for feedback.

dkl

Comment 2

[:glob ✱]

Comment on attachment 8520881 [details] [diff] [review]
1090727_1.patch

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

nice, this looks like an excellent start.
note you have the jquery-cookie.js plugin outside of the plugins directory; it would be cleaner to minify it and move it into js/jquery/plugins

one gotcha with migration is YUI's tools will ignore requests with invalid/missing elements, while jquery will throw an error:
> YAHOO.util.Dom.removeClass('monkey', 'banana')
> (returns false)
> $('#monkey').removeClass('banana')
> TypeError: $(...) is null
from just reading the header code here i think it's ok, but this would require testing.

Comment 3

[David Lawrence [:dkl]]

Attachment: 1090727_2.patch

new patch that has minified cookie-min.js and also uses 'use_yui' param (default on) so that some pages can opt out of loading YUI.

Looking through the code, I feel the next phase after this bug can simply be replacing all use of YAHOO.util.Dom.*, YAHOO.util.Event.*, YAHOO.util.JSON.*, etc. with the jQuery counterparts. Some js/*.js files are simply using those util methods and would be easy ports.

The next stage will be more complex such as re-implementing the autocomplete, dupe search and other functions. But I suggest we tackle all low hanging fruit and work out way up.

> one gotcha with migration is YUI's tools will ignore requests with
> invalid/missing elements, while jquery will throw an error:
> > YAHOO.util.Dom.removeClass('monkey', 'banana')
> > (returns false)
> > $('#monkey').removeClass('banana')
> > TypeError: $(...) is null
> from just reading the header code here i think it's ok, but this would
> require testing.

I did not notice this in my testing. I edited some lines in js/global.js to look for non-existent IDs and did not see an error in the logs. Of course the div was not hidden as predicted but no error was generated. Will try some other things to reproduce.

dkl
dkl

Comment 4

[:glob ✱]

Comment on attachment 8521614 [details] [diff] [review]
1090727_2.patch

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

this looks great.  just a few things to sort out..

::: template/en/default/global/header.html.tmpl
@@ +32,4 @@
>    onload = ""
>    style_urls = []
>    yui = []
> +  use_yui = 1

you can't DEFAULT a boolean to true, because they are impossible to set to false when PROCESSing the header.

@@ +66,5 @@
>    # every other JS URL.
>    #%]
>  [% SET starting_js_urls = [
> +    "js/jquery/jquery-min.js",
> +    "js/jquery/plugin/cookie/cookie-min.js",

the cookie plugin should be pushed onto the 'jquery' array, not starting_js_urls.

this will also fix the typo in the path (s/plugin/plugins/), which generates a javascript error when switching languages.

@@ +85,5 @@
> +  [% jq_css_urls.push(jq_css_url) %]
> +[% END %]
> +[% FOREACH jq_css_url = jq_css_urls %]
> +  [% style_urls.push(jq_css_url) %]
> +[% END %]

instead of two FOREACHes you can use .import:

[% undef = style_urls.import(jq_css_urls, jquery_css) %]

Comment 5

[David Lawrence [:dkl]]

Attachment: 1090727_3.patch

Comment 6

[:glob ✱]

Comment on attachment 8522506 [details] [diff] [review]
1090727_3.patch

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

r=glob

during testing i discovered we're breaking "data:" urls which jquery-ui uses; filed as bug 1100368.

::: template/en/default/global/header.html.tmpl
@@ +68,5 @@
>    # every other JS URL.
>    #%]
>  [% SET starting_js_urls = [
> +    "js/jquery/jquery-min.js",
> +    "js/jquery/ui/jquery-ui-min.js"

nit: indentation here is weird; should be 2 spaces not 4.

@@ +79,5 @@
> +
> +[% SET jq_css_urls = [
> +    "js/jquery/ui/jquery-ui-min.css",
> +    "js/jquery/ui/jquery-ui-structure-min.css",
> +    "js/jquery/ui/jquery-ui-theme-min.css",

same indentation issue here

@@ +84,2 @@
>  ] %]
> +[% undef = style_urls.import(jquery_css, jq_css_urls) %] 

remove trailing whitespace

Comment 7

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   11c2093..a50e8fd  master -> master

Comment 8

[Frédéric Buclin]

Comment on attachment 8522506 [details] [diff] [review]
1090727_3.patch

>+++ b/template/en/default/global/header.html.tmpl

>+[% undef = style_urls.import(jquery_css, jq_css_urls) %] 

Err... what does [% undef = ... %] mean???