Visually disable VR Shell back/forward buttons when no history is available

chrome/browser/resources/vr_shell/vr_shell_ui.js

Index: chrome/browser/resources/vr_shell/vr_shell_ui.js
diff --git a/chrome/browser/resources/vr_shell/vr_shell_ui.js b/chrome/browser/resources/vr_shell/vr_shell_ui.js
index f5457f9cbb6f319902315ced9a75633a21c98267..013e4e202ad6b5f72d3241a2e48e155f6017e034 100644
--- a/chrome/browser/resources/vr_shell/vr_shell_ui.js
+++ b/chrome/browser/resources/vr_shell/vr_shell_ui.js
@@ -161,6 +161,7 @@ var vrShellUi = (function() {
       let captionId = domId + '-caption';
       this.button = document.querySelector(domId);
       this.caption = document.querySelector(captionId);
+      this.enabled = true;
 
       // Create an invisible parent, from which the button will hover.
       let backing = new api.UiElement(0, 0, 0, 0);
@@ -200,6 +201,15 @@ var vrShellUi = (function() {
       ui.updateElement(this.captionElement.id, update);
     }
 
+    setEnabled(enabled) {
+      this.enabled = enabled;
+      if (enabled) {
+        this.button.classList.remove('disabled-button');
+      } else {
+        this.button.classList.add('disabled-button');
+      }
+    }
+
     configure(buttonOpacity, captionOpacity, distanceForward) {
       this.button.style.opacity = buttonOpacity;
       this.caption.style.opacity = captionOpacity;
@@ -211,7 +221,9 @@ var vrShellUi = (function() {
     }
 
     onMouseEnter() {
-      this.configure(1, 1, 0.015);
+      if (this.enabled) {
+        this.configure(1, 1, 0.015);
+      }
     }
 
     onMouseLeave() {
@@ -275,6 +287,14 @@ var vrShellUi = (function() {
         this.buttons[i].setVisible(this.enabled);
       }
     }
+
+    setBackButtonEnabled(enabled) {
+      this.buttons[0].setEnabled(enabled);

[cjgrant, 2017/03/08 16:32:14]

This indexing looks a bit risky.  Anyone could reorder the buttons above, and
break this.  Maybe get rid of 'descriptors' above, manually build each element
and append them to a list (keeping explicit references to the back and forward
buttons), then have the for loop walk through the list of existing elements
instead.  Just a thought...

[amp, 2017/03/08 16:34:14]

These hard coded indexes seem fragile if we ever happen to change the button
order or add new ones etc.

[acondor_, 2017/03/08 16:42:26]

I agree with the fact that it's risky and the suggested solution seems
reasonable to me. Or, we could make `buttons` an object instead of an array.
What do you think?

[cjgrant, 2017/03/08 18:19:32]

The array was used only as a convenience for positioning a variable number of
buttons in a row, and for showing/hiding them all easily.  I'm not attached to
that idea, so feel free to suggest any modification through this review. :)

+    }
+
+    setForwardButtonEnabled(enabled) {
+      this.buttons[2].setEnabled(enabled);
+    }
   };
 
   /**
@@ -910,6 +930,8 @@ var vrShellUi = (function() {
       this.mode = api.Mode.UNKNOWN;
       this.menuMode = false;
       this.fullscreen = false;
+      this.canGoBack = false;
+      this.canGoForward = false;
 
       this.background = new Background();
       this.contentQuad = new ContentQuad();
@@ -939,6 +961,12 @@ var vrShellUi = (function() {
       this.updateState();
     }
 
+    setHistoryButtonsEnabled(canGoBack, canGoForward) {
+      this.canGoBack = canGoBack;
+      this.canGoForward = canGoForward;
+      this.updateState();
+    }
+
     updateState() {
       /** @const */ var URL_INDICATOR_VISIBILITY_TIMEOUT_MS = 5000;
 
@@ -953,6 +981,8 @@ var vrShellUi = (function() {
       this.contentQuad.setMenuMode(menuMode);
       // TODO(crbug/643815): Set aspect ratio on content quad when available.
       this.controls.setEnabled(menuMode);
+      this.controls.setBackButtonEnabled(this.canGoBack);

[mthiesse, 2017/03/08 16:15:18]

This breaks fullscreen exit doesn't it? I think you want something like:
this.controls.setBackButtonEnabled(this.canGoBack || fullscreen);

[amp, 2017/03/08 16:34:14]

+1.  I was going to ask about this.

Note that regular 2D chrome supports exiting full screen on back navitgation as
well, so it might be worthwhile to see if the mTab.canGoBack() check in
VrShellImpl.java already covers it (but I'm not confident that it will as I
suspect they coded in an exception for full screen as well).

[acondor_, 2017/03/08 16:42:26]

Done.

[mthiesse, 2017/03/08 19:38:16]

mTab.canGoBack() does not cover the fullscreen case (everything is a special
case in clank :P). They simply ask the navigationController whether the page can
navigate backwards/forwards.

+      this.controls.setForwardButtonEnabled(this.canGoForward);
       this.omnibox.setEnabled(menuMode);
       this.urlIndicator.setEnabled(mode == api.Mode.STANDARD && !menuMode);
       this.urlIndicator.setVisibilityTimeout(
@@ -1063,6 +1093,11 @@ var vrShellUi = (function() {
     }
 
     /** @override */
+    onSetHistoryButtonsEnabled(canGoBack, canGoForward) {
+      uiManager.setHistoryButtonsEnabled(canGoBack, canGoForward);
+    }
+
+    /** @override */
     onCommandHandlerFinished() {
       ui.flush();
     }