Merge lp:~benji/juju-gui/bug-1104105-browser-compatability-warning-3 into lp:juju-gui/experimental

[Benji York]

Add a warning about unsupported browsers.

The tests for the code are a bit unusual in that the code under test is
embedded in an HTML file, so it must be extracted and the test file built my
the Makefile so it can be tested. I don't think this approach is too heinous.

https://codereview.appspot.com/7299071/

[lp:~benji/juju-gui/bug-1104105-browser-compatability-warning-3]

383. By Benji York on 2013-02-08

merge trunk and resolve conflict

[Brad Crittenden (bac)]

The branch looks good, thanks Benji. One change to make before landing.

https://codereview.appspot.com/7299071/diff/3001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7299071/diff/3001/app/index.html#newcode41
app/index.html:41: Your browser not fully supported.</span>
Please change to: Your browser is not fully supported.

https://codereview.appspot.com/7299071/

[Gary Poster (gary)]

Hi Benji. Interesting test approach that is cool. Will think about it
more.

I raised the concern on IRC last week that make prod appears to be
broken. Here are some other review comments. I'll re-review when you
have these addressed and make prod fixed.

Thank you

Gary

https://codereview.appspot.com/7299071/diff/3001/Makefile
File Makefile (right):

https://codereview.appspot.com/7299071/diff/3001/Makefile#newcode523
Makefile:523: server spritegen test test-debug test-prod undocumented \
test-prep too, yeah?

https://codereview.appspot.com/7299071/diff/3001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7299071/diff/3001/app/index.html#newcode41
app/index.html:41: Your browser not fully supported.</span>
add an "is" in there please.

On IE 10 this is two lines, even without the is, which is a shame. It
would be nice if the panel were wider. I might mess with that.

https://codereview.appspot.com/7299071/diff/3001/app/index.html#newcode50
app/index.html:50: <a href="http://www.google.com/chrome">Chrome</a> to
be fully
I think we ought to say Firefox here too. Maybe we wait for CI for
that. If so, please make a bug about this ("Firefox is not regularly
tested" with description mentioning that CI is fix for this bug, and
that this message should be updated when it is).

https://codereview.appspot.com/7299071/diff/3001/app/index.html#newcode113
app/index.html:113: startTheApp = function() {
Unless someone corrects me, I'll assert that is a matter of good
practice to always use var declarations, even at the top level where
they technically are equivalent to what you have written here. Unless
someone argues against them, please add var statements here and below.
(see pastebin from next comment)

https://codereview.appspot.com/7299071/diff/3001/app/index.html#newcode150
app/index.html:150: </script>
This ordering does not accomplish the workflow we want, described in the
graphic assets. The browser warning should be shown before we load the
JS. I think this is one approach to get what we want:
http://pastebin.ubuntu.com/1636315/ . I suspect there are more elegant
ones.

https://codereview.appspot.com/7299071/diff/3001/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/7299071/diff/3001/lib/views/stylesheet.less#newcode1433
lib/views/stylesheet.less:1433: width: 300px;
I'd like to see what this looks like expanded a bit. The goal would be
to have the text fit in a single line at the top, while still looking
good for login.

https://codereview.appspot.com/7299071/

[lp:~benji/juju-gui/bug-1104105-browser-compatability-warning-3]

384. By Benji York on 2013-02-11

fix prod and tweak widths to reduce line breaks

385. By Benji York on 2013-02-11

review fixes

386. By Benji York on 2013-02-11

fix lint

387. By Benji York on 2013-02-11

fix lint

[Gary Poster (gary)]

Land with changes, unless you argue against the changes.

Cool, thank you, and thanks for the source fix. Works well for me.
What was the make prod problem?

The box is still not wide enough for me on FF, but is fine on IE. could
you push ".centered-column div" and ".centered-column .panel
input[type="submit"]" out from 260px to 280px, and ".centered-column
.panel input" out to 270px, unless you disagree? I think the design
folks might want to tweak, but not breaking the header text across lines
is more important than the arguably less attractive proportions of the
wider box, IMO.

https://codereview.appspot.com/7299071/diff/3002/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode108
app/index.html:108: <script id="app-startup">
You could protect all of these functions in a function(){[CODE]}()
namespace hack to keep them out of the global namespace. startTheApp is
the only one that would need to be global.

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode109
app/index.html:109: isBrowserSupported = function(agent) {
Suggest "var isBrowserSupported = function(agent) {"

This is nicer for variable definition, and would work as desired within
the namespace hack I mention.

OTOH, if you don't use var, "function isBrowserSupported(agent) {" is
identical in semantics, at least as clear, and more concise.

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode114
app/index.html:114: getDocument = function() {
Same var discussion

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode118
app/index.html:118: displayBrowserWarning = function() {
Same var discussion

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode123
app/index.html:123: continueWithCurrentBrowser = function() {
Same var discussion

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode130
app/index.html:130: startTheApp = function() {
Same var discussion, except that it needs to be global as you've
implemented it here.

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode136
app/index.html:136: window.setTimeout('startTheApp', 100);
Heh. Yeah, I considered this approach too. My global flag was grody in
my book, but so was this. I leaned toward the global flag, then, but
<shrug>.

https://codereview.appspot.com/7299071/diff/3002/app/index.html#newcode139
app/index.html:139: go = function() {
Same var discussion

https://codereview.appspot.com/7299071/diff/3002/lib/merge-files.js
File lib/merge-files.js (right):

https://codereview.appspot.com/7299071/diff/3002/lib/merge-files.js#newcode50
lib/merge-files.js:50: result.code += '\n//@ sourceMappingURL=' +
sourceMapName;
Good catch. Seems silly that we have to do this.

https://codereview.appspot.com/7299071/

=== modified file '.bzrignore'
--- .bzrignore	2013-02-01 20:27:02 +0000
+++ .bzrignore	2013-02-11 17:53:23 +0000
@@ -20,3 +20,5 @@
 build-debug
 bin/py
 python-shelltoolbox
+test/test_startup.js
+test/extracted_startup_code

=== modified file 'Makefile'
--- Makefile	2013-02-08 17:46:45 +0000
+++ Makefile	2013-02-11 17:53:23 +0000
@@ -33,12 +33,13 @@
 		-e '^server.js$$')
 THIRD_PARTY_JS=app/assets/javascripts/reconnecting-websocket.js
 NODE_TARGETS=node_modules/chai node_modules/cryptojs node_modules/d3 \
-	node_modules/expect.js node_modules/express node_modules/graceful-fs \
-	node_modules/grunt node_modules/jshint node_modules/less \
-	node_modules/minimatch node_modules/mocha node_modules/node-markdown \
-	node_modules/node-minify node_modules/node-spritesheet \
-	node_modules/rimraf node_modules/should node_modules/yui \
-	node_modules/yuidocjs node_modules/recess
+    node_modules/expect.js node_modules/express \
+    node_modules/graceful-fs node_modules/grunt node_modules/jshint \
+    node_modules/less node_modules/minimatch node_modules/mocha \
+    node_modules/node-markdown node_modules/node-minify \
+    node_modules/node-spritesheet node_modules/recess \
+    node_modules/rimraf node_modules/should node_modules/uglify-js \
+    node_modules/yui node_modules/yuidocjs
 EXPECTED_NODE_TARGETS=$(shell echo "$(NODE_TARGETS)" | tr ' ' '\n' | sort \
 	| tr '\n' ' ')
 
@@ -251,7 +252,7 @@
 	# below without the grep to get recess' report.
 	node_modules/recess/bin/recess lib/views/stylesheet.less --config recess.json | grep -q Perfect
 
-lint: gjslint jshint recess yuidoc-lint
+lint: test-prep gjslint jshint recess yuidoc-lint
 
 virtualenv/bin/python:
 	virtualenv virtualenv
@@ -271,7 +272,7 @@
 	ln -sf "$(PWD)/node_modules/yui/assets/skins/sam/rail-x.png" \
 		build-shared/juju-ui/assets/combined-css/rail-x.png
 	bin/merge-files
-	mv *-source-map build-shared/juju-ui/assets/
+	mv *.js.map build-shared/juju-ui/assets/
 
 build-files: $(BUILD_FILES)
 
@@ -349,7 +350,7 @@
 $(LINK_PROD_FILES):
 	$(call link-files,prod)
 	ln -sf "$(PWD)/build-shared/juju-ui/assets/all-yui.js" build-prod/juju-ui/assets/
-	ln -sf "$(PWD)"/build-shared/juju-ui/assets/*-source-map build-prod/juju-ui/assets/
+	ln -sf "$(PWD)"/build-shared/juju-ui/assets/*.js.map build-prod/juju-ui/assets/
 	# Link in the application source code so source maps work.
 	mkdir -p $(PWD)/build-prod/juju-ui/assets/source
 	ln -s $(PWD)/app $(PWD)/build-prod/juju-ui/assets/source
@@ -357,13 +358,30 @@
 
 prep: beautify lint
 
-test-debug: build-debug
+test/extracted_startup_code: app/index.html
+	# Pull the JS out of the index so we can run tests against it.
+	cat app/index.html | \
+	    sed -n '/<script id="app-startup">/,/<\/script>/p'| \
+	    head -n-1 | tail -n+2 > test/extracted_startup_code
+
+test/test_startup.js: test/test_startup.js.top test/test_startup.js.bottom \
+    test/extracted_startup_code
+	# Stitch together the test file for app start-up.
+	echo "// THIS IS A GENERATED FILE.  DO NOT EDIT." > $@
+	echo "// See the Makefile for details." >> $@
+	cat test/test_startup.js.top >> $@
+	cat test/extracted_startup_code >> $@
+	cat test/test_startup.js.bottom >> $@
+
+test-prep: test/test_startup.js
+
+test-debug: build-debug test-prep
 	./test-server.sh debug
 
-test-prod: build-prod
+test-prod: build-prod test-prep
 	./test-server.sh prod
 
-test-server: build-debug
+test-server: build-debug test-prep
 	./test-server.sh debug true
 
 test-misc:
@@ -500,9 +518,9 @@
 
 # targets are alphabetically sorted, they like it that way :-)
 .PHONY: appcache-force appcache-touch beautify build build-files \
-	build-devel clean clean-all clean-deps clean-docs code-doc \
-	debug devel docs dist gjslint help jshint lint main-doc prep prod \
-	recess server spritegen test test-debug test-prod undocumented \
+	build-devel clean clean-all clean-deps clean-docs code-doc debug \
+	devel docs dist gjslint help jshint lint main-doc prep prod recess \
+	server spritegen test test-debug test-prep test-prod undocumented \
 	view-code-doc view-docs view-main-doc yuidoc-lint
 
 .DEFAULT_GOAL := all

=== modified file 'app/app.js'
--- app/app.js	2013-02-08 17:36:58 +0000
+++ app/app.js	2013-02-11 17:53:23 +0000
@@ -638,7 +638,7 @@
      * @private
      */
     onLogin: function() {
-      Y.one('body > #login-mask').hide();
+      Y.one('body > #full-screen-mask').hide();
       this.dispatch();
     },
 

=== added file 'app/assets/images/alert_icon2.png'
Binary files app/assets/images/alert_icon2.png	1970-01-01 00:00:00 +0000 and app/assets/images/alert_icon2.png	2013-02-11 17:53:23 +0000 differ
=== modified file 'app/index.html'
--- app/index.html	2013-02-07 21:04:01 +0000
+++ app/index.html	2013-02-11 17:53:23 +0000
@@ -30,7 +30,32 @@
   </head>
 
   <body>
-      <div id="login-mask" class="crosshatch-background"></div>
+      <div id="full-screen-mask" class="crosshatch-background">
+        <div id="browser-warning" class="centered-column"
+            style="display:none;">
+          <i class="sprite juju_logo" title="Juju GUI"></i>
+          <div class="panel">
+            <div class="header">
+              <div class="error">
+                <span><i class="sprite alert_icon2"></i>
+                Your browser is not fully supported.</span>
+              </div>
+            </div>
+            <p>
+              If you continue to use Juju with your current browser your
+              experience may not be as good as we would like it to be.
+            </p>
+            <p>
+              Please use the latest version of
+              <a href="http://www.google.com/chrome">Chrome</a> to be fully
+              supported.
+            </p>
+            <form onsubmit="return continueWithCurrentBrowser();">
+              <input type="submit" value="Continue"/>
+            </form>
+          </div>
+        </div>
+      </div>
       <div id="notifier-box"></div>
       <div id="viewport-wrapper">
         <div id="vp-left-border"></div>
@@ -80,17 +105,69 @@
         </div>
         <div id="vp-right-border"></div>
     </div>
-    <!-- javascript away -->
+    <script id="app-startup">
+      isBrowserSupported = function(agent) {
+        // At the moment, only Chrome is supported.
+        return (/Chrome/.test(agent));
+      };
+
+      getDocument = function() {
+        return document;
+      };
+
+      displayBrowserWarning = function() {
+        getDocument()
+          .getElementById('browser-warning').style.display = 'block';
+      };
+
+      continueWithCurrentBrowser = function() {
+        getDocument()
+          .getElementById('browser-warning').style.display = 'none';
+        startTheApp();
+        return false;
+      };
+
+      startTheApp = function() {
+        // This function will be redefined when all the app's JavaScript is
+        // loaded.  We want to keep trying until that happens.
+
+        // Tell jslint that we really do want to evaluate a string:
+        /*jslint evil: true */
+        window.setTimeout('startTheApp', 100);
+      };
+
+      go = function() {
+        if (isBrowserSupported(navigator.userAgent)) {
+          startTheApp();
+        } else {
+          displayBrowserWarning();
+        }
+      };
+    </script>
+    <!--
+      Load the (potentially slow to download) core of the app.  We do this here
+      because we want the browser warning to execute before spending the time
+      to download an app the user might not be able to use anyway.
+    -->
     <script src="/juju-ui/assets/all-yui.js"></script>
     <script src="/juju-ui/assets/modules.js"></script>
     <script src="/juju-ui/assets/config.js"></script>
     <script>
-      YUI(GlobalConfig).use(["juju-gui"], function(Y) {
-        app = new Y.juju.App(juju_config);
-        // We need to activate the hotkeys when running the application
-        // in production. Unit tests should call it manually.
-        app.activateHotkeys();
-      });
+      // Now that all of the above JS is loaded we can define the real start
+      // function which will be picked up by the setTimeout and the app will
+      // start.
+      startTheApp = function() {
+        YUI(GlobalConfig).use(['juju-gui'], function(Y) {
+          app = new Y.juju.App(juju_config);
+          // We need to activate the hotkeys when running the application
+          // in production. Unit tests should call it manually.
+          app.activateHotkeys();
+        });
+      };
+      // This call is here instead of in the "app-startup" script tag above
+      // because we extract that JS in order to test it.  This bit here is just
+      // to bootstrap the app when actually loaded into a browser.
+      go();
     </script>
   </body>
 </html>

=== modified file 'app/templates/login.handlebars'
--- app/templates/login.handlebars	2013-01-10 10:25:55 +0000
+++ app/templates/login.handlebars	2013-02-11 17:53:23 +0000
@@ -1,7 +1,7 @@
-<div id="login-column">
+<div class="centered-column">
   <i class="sprite juju_logo" title="Juju GUI"></i>
-  <div id="login-form">
-    <div id="login-header">
+  <div id="login-form" class="panel">
+    <div class="header">
       {{environment_name}} <span class="provider-type">{{provider_type}}</span>
     </div>
     <form>

=== modified file 'app/views/login.js'
--- app/views/login.js	2013-02-08 17:36:58 +0000
+++ app/views/login.js	2013-02-11 17:53:23 +0000
@@ -56,12 +56,12 @@
       // In order to have the mask cover everything, it needs to be an
       // immediate child of the body.  In order for it to render immediately
       // when the app loads, it needs to be in index.html.
-      var loginMask = Y.one('body > #login-mask');
-      if (!loginMask) {
-        // No login mask in the DOM, as is the case in tests.
+      var mask = Y.one('body > #full-screen-mask');
+      if (!mask) {
+        // No mask in the DOM, as is the case in tests.
         return this;
       }
-      loginMask.show();
+      mask.show();
       var env = this.get('env');
       var environment_name_node = Y.one('#environment-name');
       var provider_type_node = Y.one('#provider-type');

=== modified file 'lib/merge-files.js'
--- lib/merge-files.js	2013-02-06 17:37:02 +0000
+++ lib/merge-files.js	2013-02-11 17:53:23 +0000
@@ -32,7 +32,7 @@
 
   // Combine one or more JavaScript files.
   function combineJs(files, outputFile) {
-    var sourceMapName = syspath.basename(outputFile, '.js') + '-source-map';
+    var sourceMapName = syspath.basename(outputFile) + '.map';
     // We feed the minifier relative path names so the source map will map to
     // relative URLs that can be easily served.
     var relative_files = [];
@@ -45,6 +45,9 @@
       outSourceMap: sourceMapName,
       sourceRoot: 'source'
     });
+    // The uglifyjs script does this instead of the library, so we have to do
+    // it ourselves since we are using uglify as a library and not a script.
+    result.code += '\n//@ sourceMappingURL=' + sourceMapName;
     fs.writeFileSync(outputFile, result.code, 'utf8', throw_error);
     fs.writeFileSync(sourceMapName, result.map, 'utf8', throw_error);
   }

=== modified file 'lib/views/stylesheet.less'
--- lib/views/stylesheet.less	2013-02-07 19:04:00 +0000
+++ lib/views/stylesheet.less	2013-02-11 17:53:23 +0000
@@ -1414,7 +1414,7 @@
   }
 }
 
-#login-mask {
+#full-screen-mask {
     display: block;
     position: absolute;
     top: 0;
@@ -1423,18 +1423,18 @@
     height: 100%;
     z-index: 10000;
 }
-#login-column {
+.centered-column {
     display: block;
     z-index: 10001;
     position: absolute;
     top: 80px;
     left: 50%;
     text-align: center;
-    width: 300px;
-    margin-left: -150px;
+    width: 400px;
+    margin-left: -200px;
 
     div {
-        width: 220px;
+        width: 260px;
         text-align: left;
         margin: 0 auto;
         margin-bottom: 1ex;
@@ -1445,17 +1445,20 @@
             text-decoration: none
         }
     }
-    @login-panel-color: #f5f5f5;
-    @login-panel-gradient: (@login-panel-color - #111);
-    @login-panel-shadow: (@login-panel-color - #444);
+
+    @panel-color: #f5f5f5;
+    @panel-gradient: (@panel-color - #111);
+    @panel-shadow: (@panel-color - #444);
     @curve-radius: ~"24px 28px";
-    div#login-form {
+
+    .panel {
         input {
+            width: 250px;
             margin-top: 8px;
             margin-bottom: 8px;
         }
         input[type=submit] {
-            width: 220px;
+            width: 260px;
             height: 26px;
             text-transform: uppercase;
             text-shadow: none;
@@ -1474,12 +1477,10 @@
         padding: 5px 25px 5px;
         margin-top: 3ex;
         margin-bottom: 4ex;
-        .create-button(@login-panel-color,
-                       @login-panel-gradient,
-                       @login-panel-shadow,
-                       -6px);
-        .create-border-radius(~"24px / 28px");
-        div#login-header {
+        .create-button(@panel-color, @panel-gradient, @panel-shadow, -6px);
+        .create-border-radius(@curve-radius);
+
+        .header {
             padding: 15px 25px 10px 25px;
             margin-left: -25px;
             margin-top: -5px;
@@ -1489,8 +1490,8 @@
             -moz-border-radius-topright: @curve-radius;
             border-top-left-radius: @curve-radius;
             border-top-right-radius: @curve-radius;
-            @gradient-start: @login-panel-gradient;
-            @gradient-end: @login-panel-shadow;
+            @gradient-start: @panel-gradient;
+            @gradient-end: @panel-shadow;
             background-image: -ms-linear-gradient(
                 top, @gradient-start, @gradient-end);
             background-image: -webkit-gradient(

=== modified file 'test/index.html'
--- test/index.html	2013-02-07 16:40:46 +0000
+++ test/index.html	2013-02-11 17:53:23 +0000
@@ -28,6 +28,7 @@
 
 
   <!-- Tests (Alphabetical)-->
+  <script src="test_startup.js"></script>
   <script src="test_app.js"></script>
   <script src="test_app_hotkeys.js"></script>
   <script src="test_application_notifications.js"></script>

=== modified file 'test/test_login.js'
--- test/test_login.js	2013-01-18 13:40:28 +0000
+++ test/test_login.js	2013-02-11 17:53:23 +0000
@@ -85,7 +85,7 @@
 
   describe('login view', function() {
     var requires = ['node', 'juju-gui', 'juju-views', 'juju-tests-utils'];
-    var Y, conn, env, utils, juju, views, loginView, container, loginMask;
+    var Y, conn, env, utils, juju, views, loginView, container, mask;
     var test = it; // We aren't really doing BDD so let's be more direct.
 
     before(function(done) {
@@ -104,7 +104,7 @@
       conn.open();
       container = Y.one('body').appendChild('<div/>');
       // Needed by the render method.
-      loginMask = Y.one('body').appendChild('<div/>').set('id', 'login-mask');
+      mask = Y.one('body').appendChild('<div/>').set('id', 'full-screen-mask');
       loginView = new views.login(
           {container: container, env: env, help_text: 'Help text'});
     });
@@ -112,7 +112,7 @@
     afterEach(function() {
       env.destroy();
       container.remove(true);
-      loginMask.remove(true);
+      mask.remove(true);
     });
 
     test('the view login method logs in through the environment', function() {

=== added file 'test/test_startup.js.bottom'
--- test/test_startup.js.bottom	1970-01-01 00:00:00 +0000
+++ test/test_startup.js.bottom	2013-02-11 17:53:23 +0000
@@ -0,0 +1,50 @@
+      startTheApp = function() {
+        appStarted = true;
+      };
+      getDocument = function() {
+        return {
+          getElementById: function(id) {
+            assert.equal(id, 'browser-warning');
+            return {style: warningStyle};
+          }
+        };
+      };
+    });
+
+    it('knows that Chrome is supported', function() {
+      assert.isTrue(isBrowserSupported('Chrome'));
+    });
+
+    it('knows that Firefox is not supported', function() {
+      assert.isFalse(isBrowserSupported('Firefox'));
+    });
+
+    it('knows that IE is not supported', function() {
+      assert.isFalse(isBrowserSupported('MSIE'));
+    });
+
+    it('can display the browser warning', function() {
+      displayBrowserWarning();
+      assert.equal(warningStyle.display, 'block');
+    });
+
+    it('will hide the browser warning when the user continues', function() {
+      continueWithCurrentBrowser();
+      assert.equal(warningStyle.display, 'none');
+    });
+
+    it('will stop event propigation when the user continues', function() {
+      var result = continueWithCurrentBrowser();
+      // Since the function is an event handler of a submit button and we do
+      // not want the form submittion to actually happen, the handler must
+      // return false.
+      assert.isFalse(result);
+    });
+
+    it('will start the app when the user continues', function() {
+      continueWithCurrentBrowser();
+      assert.isTrue(appStarted);
+    });
+
+  });
+}) ();

=== added file 'test/test_startup.js.top'
--- test/test_startup.js.top	1970-01-01 00:00:00 +0000
+++ test/test_startup.js.top	2013-02-11 17:53:23 +0000
@@ -0,0 +1,10 @@
+'use strict';
+
+(function() {
+  describe('Application start-up', function() {
+    var startTheApp, isBrowserSupported, displayBrowserWarning,
+        continueWithCurrentBrowser, go, app, juju_config, getDocument,
+        appStarted, warningStyle;
+    before(function() {
+      warningStyle = {display: null};
+      appStarted = false;