Issue 5692 - Create Browser Selector with Browser Detection Component for help.eyeo.com

static/js/main.js

-(function(){
+(function()
+{
   document.addEventListener("DOMContentLoaded", function()
   {
 
     /**************************************************************************
      * General
      **************************************************************************/
 
     // Change html class name from "no-js" to "js"
     document.documentElement.className = "js";
 
@@ skipping 19 matching lines @@
     }
 
     /**************************************************************************
      * CustomSelect
      **************************************************************************/
 
     function CustomSelect(select)
     {
       this.select = select;
       this.close();
-      select.addEventListener("click", this._onClick.bind(this), false);
+      this.select
+          .addEventListener("click", this._onClick.bind(this), false);
+      this.select
+          .addEventListener("focusout", this._onFocusOut.bind(this), false);
+    }
+
+    CustomSelect.prototype._onFocusOut = function()
+    {
+      // setTimeout to allow document.activeElement 
+      // to move to newly focused element
+      setTimeout(function()
+      {
+        var newFocus = document.activeElement;
+        
+        if (newFocus
+              .classList.contains("custom-select-selected") ||
+            newFocus
+              .classList.contains("custom-select-option") ||
+            newFocus
+              .parentElement
+              .classList.contains("custom-select-option"))
+        {
+          return;
+        }
+
+        this.close();
+        
+      }.bind(this), 1);
     }
 
     CustomSelect.prototype._onClick = function(event)
     {
       if (event.target.classList.contains("custom-select-selected"))
       {
         var options = this.select.querySelector(".custom-select-options");
         if (options.getAttribute("aria-hidden") == "true")
         {
           this.open();
@@ skipping 144 matching lines @@
     var productTopicsAccordion = document.getElementById('product-topics-accordion');
     if (productTopicsAccordion)
     {
       new Accordion(productTopicsAccordion);
     }
 
     /**************************************************************************
      * BrowserSelect
      **************************************************************************/
 
     function BrowserSelect(select)

[juliandoucette, 2017/10/13 13:39:56]

Note: You could extend CustomSelect's prototype (e.g. BrowserSelect.prototype =
extend({}, CustomSelect.prototype)) and apply CustomSelect (e.g.
CustomSelect.apply(this, [select]) and use this.select instead of
this.browserSelect.

What you did here is fine too.

[ire, 2017/10/17 15:03:33]

Cool. I'll keep what i did here then.

     {
-      this.browserSelect = select;
-      this.browserCustomSelect = new CustomSelect(this.browserSelect);
-      
-      this.DEFAULT_BROWSER = "chrome";
+      this.select = select;
+      CustomSelect.apply(this, [this.select]);
+      
       this.BROWSER_STORAGE_KEY = "BROWSER";
       this.BROWSER_AUTODETECTED_STORAGE_KEY = "BROWSER_AUTODETECTED";
-      
-      this.browserSelect
+      this.SUPPORTED_BROWSERS = ["chrome", "opera", "samsungBrowser", 
+                                 "yandexbrowser", "maxthon", "msie", 
+                                 "msedge", "firefox", "ios", "safari"];
+      this.DEFAULT_BROWSER = "chrome";
+      
+      this.select
         .addEventListener("click", this._onClickOrKeyDown.bind(this), false);
       

[juliandoucette, 2017/10/13 13:39:56]

NIT: You should be able to click-away too (click elsewhere to close the
dropdown)?

[ire, 2017/10/17 15:03:34]

Yes, I will fix this in a separate issue

https://issues.adblockplus.org/ticket/5868#ticket

[juliandoucette, 2017/10/18 15:04:10]

Acknowledged.

-      this.browserSelect
+      this.select
         .addEventListener("keydown", this._onClickOrKeyDown.bind(this), false);
 
       var storedBrowser = localStorage.getItem(this.BROWSER_STORAGE_KEY);
-      if (storedBrowser)
-      {
-        this.selectOption(storedBrowser);
-      }
-      else
-      {
-        this.detectBrowser();
-      }
-    }
+      if (storedBrowser) this.selectOption(storedBrowser);
+      else this.detectBrowser();
+    }
+
+    BrowserSelect.prototype = Object.create(CustomSelect.prototype);
+    BrowserSelect.prototype.constructor = BrowserSelect;
 
     BrowserSelect.prototype.detectBrowser = function()
     {
-      localStorage.setItem(this.BROWSER_AUTODETECTED_STORAGE_KEY, "true");
-
-      var browser;
-      if (bowser.chrome) browser = "chrome";

[juliandoucette, 2017/10/13 13:39:55]

Wait, what? How can browser have properties if it was just defined as undefined?

[ire, 2017/10/17 15:03:34]

You can override previously defined (or undefined) properties in this way

[juliandoucette, 2017/10/18 15:04:10]

Can you elaborate / explain when/how this variable is not undefined?

[ire, 2017/10/20 13:40:46]

Sure!

I create a pen to illustrate this -
https://codepen.io/ire/pen/MExoQM?editors=0011

Basically, variables created with var/let can have their values updated in this
way. You can initialise a variable with one value, then update it later on.
Then, depending when the variable reference is used, the value can be different.

-      else if (bowser.opera) browser = "opera";
-      else if (bowser.coast) browser = "opera";
-      else if (bowser.samsungBrowser) browser = "samsung";
-      else if (bowser.yandexbrowser) browser = "yandex";
-      else if (bowser.maxthon) browser = "maxthon";
-      else if (bowser.msie) browser = "ie";
-      else if (bowser.msedge) browser = "edge";
-      else if (bowser.firefox) browser = "firefox";
-      else if (bowser.ios) browser = "ios";
-      else if (bowser.safari) browser = "safari";

[juliandoucette, 2017/10/18 15:04:10]

Why check each browser property and not *just* browser.name?

[ire, 2017/10/20 13:40:46]

I felt it wouldn't be as reliable/steady as this. I couldn't find a set list of
the browser names without having to go into the bowser.js code. 

-      else
-      {
-        localStorage.removeItem(this.BROWSER_AUTODETECTED_STORAGE_KEY);
-        browser = this.DEFAULT_BROWSER;
-      }
-
-      this.selectOption(browser);  
-    }
+      for (var i = 0; i < this.SUPPORTED_BROWSERS.length; i++)
+      {
+        var supportedBrowser = this.SUPPORTED_BROWSERS[i];
+        if (bowser[supportedBrowser])
+        {
+          localStorage.setItem(this.BROWSER_AUTODETECTED_STORAGE_KEY, "true");
+          return this.selectOption(supportedBrowser);
+        }
+      }
+
+      this.selectOption(this.DEFAULT_BROWSER);  
+    };
 
     BrowserSelect.prototype.selectOption = function(browser)
     {
-      // Save value to Local Storage
       localStorage.setItem(this.BROWSER_STORAGE_KEY, browser);
 
       // Change body class
-      document.body.className = "ua-"+browser;

[juliandoucette, 2017/10/13 13:39:55]

What if there are not related classes attached to the body later?

[juliandoucette, 2017/10/13 13:41:36]

unrelated*

[ire, 2017/10/17 15:03:34]

Done.

+      var bodyClassList = Array.prototype.slice.call(document.body.classList);
+      for (var i = 0; i < bodyClassList.length; i++)
+      {
+        if (bodyClassList[i].indexOf('ua-') > -1)
+        {
+          document.body.classList.remove(bodyClassList[i]);
+        }
+      }
+      document.body.classList.add("ua-" + browser);
+
+      // Check selected option
+      var selectedItem = this.select
+                             .querySelector("[data-value='" + browser + "']");
+      selectedItem.setAttribute("aria-checked", "true");
 
       // Set selected option
-      var selectedOption = document.createElement("li");
-      selectedOption.innerHTML = this
-                                  .browserSelect

[juliandoucette, 2017/10/13 13:39:55]

NIT: spaces around +?

[ire, 2017/10/17 15:03:34]

I thought we don't put spaces within the ()?

[juliandoucette, 2017/10/18 15:04:11]

We don't put spaces after the opening brace and before the closing brace AFAIK
(I can't find a reference in our code-style).

(1 + 1) //good
( 1 + 1 ) //bad

[ire, 2017/10/20 13:40:48]

Alright, makes sense. Done.

-                                  .querySelector("[data-value='"+browser+"']")
-                                  .innerHTML;
+      var selectedOption = selectedItem.innerHTML;
 
       if (localStorage.getItem(this.BROWSER_AUTODETECTED_STORAGE_KEY))
       {
-        selectedOption.innerHTML += "<span class='muted'>(autodetected)</span>";

[juliandoucette, 2017/10/13 13:39:55]

This isn't translated. I suggest translating it in the HTML and show/hiding or
add/removing it.

[ire, 2017/10/17 15:03:34]

I place it in a template so I wouldn't have to have it in the HTML for all items

[juliandoucette, 2017/10/18 15:04:10]

You mean you will place it in a template? Or something else?

[ire, 2017/10/20 13:40:48]

Sorry I meant to say "I will place it in a template" (as I have done)

-      }
-
-      this.browserSelect
-        .querySelector(".custom-select-selected")
-        .innerHTML = selectedOption.innerHTML;
-    }
+        var autodetected = document
+                              .getElementById('browser-select-autodetected')
+                              .innerHTML;
+        selectedOption += "<span class='muted'>(" + autodetected + ")</span>";
+      }
+
+      this.select
+          .querySelector(".custom-select-selected")
+          .innerHTML = selectedOption;
+    };
 
     BrowserSelect.prototype._onClickOrKeyDown = function(event)
     {
       if (!event.target.classList.contains("custom-select-option")) return;
 
       var IS_ENTER_KEY = event.key == "Enter" || event.keyCode == 13;
       if (event.keyCode && !IS_ENTER_KEY) return;

[juliandoucette, 2017/10/13 13:39:56]

Why check event.keyCode here?

[ire, 2017/10/17 15:03:33]

Because this function is also relevant for clicks. I'm checking here to return
only if it was a keyDown event AND the key wasn't the enter key. 

[juliandoucette, 2017/10/18 15:04:11]

Acknowledged. I was confused.

 
       localStorage.removeItem(this.BROWSER_AUTODETECTED_STORAGE_KEY);
 
+      // Uncheck previously checked option
+      this.select
+          .querySelector("[aria-checked='true']")
+          .setAttribute("aria-checked", "false");
+
       this.selectOption(event.target.getAttribute("data-value"));
 
-      // Close Select
-      this.browserCustomSelect.close();
-    }
+      this.close();
+    };
 
     var browserSelect = document.getElementById("browser-select");
     if (browserSelect)
     {
       new BrowserSelect(browserSelect);
     }
 
   }, false);
 }());