Fixes for two JS bugs on DCC

chrome/common/extensions/docs/static/js/article.js

Index: chrome/common/extensions/docs/static/js/article.js
diff --git a/chrome/common/extensions/docs/static/js/article.js b/chrome/common/extensions/docs/static/js/article.js
index 248f02d206859e7e845b79eb62f64de2bd8f2af6..ce20427f03970df2926cbd6db542eb3646debcbc 100644
--- a/chrome/common/extensions/docs/static/js/article.js
+++ b/chrome/common/extensions/docs/static/js/article.js
@@ -9,11 +9,19 @@
 // button: click logic, and when to show it.
 (function() {
 
-var isLargerThanMobile = window.matchMedia('screen and (min-width: 580px)').matches;
+var isLargerThanMobileQuery =
+    window.matchMedia('screen and (min-width: 580px)');
+isLargerThanMobileQuery.addListener(onMediaQuery);

[Jeffrey Yasskin, 2014/02/04 22:22:02]

I'm surprised this works before onMediaQuery is defined. Maybe I just don't
understand JS evaluation order.

Could you put this .addListener next to the eager call, so all the related logic
is together?

[Arthur Evans, 2014/02/07 22:50:47]

Moved query definition and listener to the end. 

 
 var sidebar = document.querySelector('.inline-toc');
+var sidebaroOffsetTop = null;

[Jeffrey Yasskin, 2014/02/04 22:22:02]

spelling: "sidebaro"?

[Arthur Evans, 2014/02/07 22:50:47]

Done.

 var articleBody = document.querySelector('[itemprop="articleBody"]');
+var setupComplete = false;
 
+// Bomb out if we're not on an article page and have a TOC.

[Jeffrey Yasskin, 2014/02/04 22:22:02]

s/if we're not/unless we're/ to avoid precedence problems.

[Arthur Evans, 2014/02/07 22:50:47]

Done.

+if (!(sidebar && articleBody)) {
+  return;
+}
 
 function addPermalink(el) {
   el.classList.add('has-permalink');
@@ -31,42 +39,49 @@ function addPermalinkHeadings(container) {
   }
 }
 
-// Bomb out if we're not on an article page and have a TOC.
-if (!(sidebar && articleBody)) {
-  return;
+function onScroll(e) {

[Jeffrey Yasskin, 2014/02/04 22:22:02]

We'll be able to replace this with position:sticky as soon as that comes out
from behind a flag: http://www.chromestatus.com/features/6190250464378880.

+  window.scrollY >= sidebaroOffsetTop ? sidebar.classList.add('sticky') :
+                                        sidebar.classList.remove('sticky');
 }
 
-if (isLargerThanMobile) {
-  var toc = sidebar.querySelector('#toc');
-  var offsetTop = sidebar.offsetParent.offsetTop
-
-  document.addEventListener('scroll', function(e) {
-    window.scrollY >= offsetTop ? sidebar.classList.add('sticky') :
-                                  sidebar.classList.remove('sticky');
-  });
-
-  toc.addEventListener('click', function(e) {
-    var parent = e.target.parentElement;
-    if (e.target.localName == 'a' && parent.classList.contains('toplevel')) {
-      // Allow normal link click if  h2 toplevel heading doesn't have h3s.
-      if (parent.querySelector('.toc')) {
-        e.preventDefault();
-        parent.classList.toggle('active');
-      }
-    }
-  });
+function onMediaQuery(e) {
+  if (e.matches) {
+    // Prevent permalinks from appearing on mobile.
+    document.body.classList.remove('no-permalink');
 
-} else {
-  // Prevent permalinks from appearong on mobile.
-  document.body.classList.add('no-permalink');
+    sidebaroOffsetTop = sidebar.offsetParent.offsetTop
 
-  articleBody.addEventListener('click', function(e) {
-    if (e.target.localName == 'h2') {
-      e.target.parentElement.classList.toggle('active');
-    }
-  });
+    document.addEventListener('scroll', onScroll);
+
+  } else {
+    // Prevent permalinks from appearing on mobile.
+    document.body.classList.add('no-permalink');

[Jeffrey Yasskin, 2014/02/04 22:22:02]

Surely this can be done in the CSS, inside a media query, instead of in
javascript.

[ericbidelman, 2014/02/04 22:53:42]

It can, but .no-permalink is a generic class we're using throughout the site and
which affects other related children.

On 2014/02/04 22:22:02, Jeffrey Yasskin wrote:

[Jeffrey Yasskin, 2014/02/04 22:58:42]

Can you show me where?
https://code.google.com/p/chromium/codesearch/#search/&sq=package:chromium$&q...
returns only 4 results, all of which look like they'd be covered by this one
media rule.

On 2014/02/04 22:53:42, ericbidelman wrote:

[ericbidelman, 2014/02/05 23:58:09]

Yep,
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext...
is the one. I'd like to stick with this single toggle since it's already in
place. Introducing an additional media query seems overkill, especially since
this query is only evaluated once 99% of the time.

On 2014/02/04 22:58:42, Jeffrey Yasskin wrote:

[Jeffrey Yasskin, 2014/02/07 23:37:33]

I would still prefer that this SCSS said:

@media screen and (max-width: $break-small) {
  .permalink {
    display: none !important;
  }
}

rather than adding and removing classes in javascript. I don't think "overkill"
applies here, since the media query is less code and easier to follow than the
imperative javascript needed to emulate it. However, I won't insist on it.

On 2014/02/05 23:58:09, ericbidelman wrote:

+
+    document.removeEventListener('scroll', onScroll);
+  }
 }
 
+// Toggle collapsible sections (mobile)

[Jeffrey Yasskin, 2014/02/04 22:22:02]

This is getting registered in more places than just mobile.

Also, please end comments with a period so it's clear you're done with your
thought.

[Arthur Evans, 2014/02/07 22:50:47]

Since the "mobile" logic is based purely on width, some devices will flip back
and forth between the mobile & non-mobile views when orientation changes.

Should we unregister and re-register the click listener in the media query
listener? 

[Jeffrey Yasskin, 2014/02/07 23:37:33]

My thought was that this code is running unconditionally now, so "(mobile)", in
the comment, is incorrect.

If you want clicks on <h2>s to toggle the 'active' class on both mobile (really
"narrow") and non-mobile, this is the way to do it. If you only want that to
happen on mobile, I would probably check the media query inside the event
handler since clicks aren't as performance-sensitive as scrolls.

+articleBody.addEventListener('click', function(e) {
+  if (e.target.localName == 'h2') {
+    e.target.parentElement.classList.toggle('active');
+  }
+});
+
+sidebar.querySelector('#toc').addEventListener('click', function(e) {
+  var parent = e.target.parentElement;
+  if (e.target.localName == 'a' && parent.classList.contains('toplevel')) {
+    // Allow normal link click if  h2 toplevel heading doesn't have h3s.
+    if (parent.querySelector('.toc')) {
+      e.preventDefault();
+      parent.classList.toggle('active');
+    }
+  }
+});
+
+onMediaQuery(isLargerThanMobileQuery);
+
 addPermalinkHeadings(articleBody);
 
 }());
+

[Jeffrey Yasskin, 2014/02/04 22:22:02]

Try not to add extra empty lines at the end of the file.

[Arthur Evans, 2014/02/07 22:50:47]

Done.