Fixes for two JS bugs on DCC

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

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Scroll handling.
 //
 // Switches the sidebar between floating on the left and position:fixed
 // depending on whether it's scrolled into view, and manages the scroll-to-top
 // 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');
   var id = el.id || el.textContent.toLowerCase().replace(' ', '-');
   el.insertAdjacentHTML('beforeend',
       '<a class="permalink" title="Permalink" href="#' + id + '">#</a>');
 }
 
 // Add permalinks to heading elements.
 function addPermalinkHeadings(container) {
   if (container) {
     ['h2','h3','h4'].forEach(function(h, i) {
       [].forEach.call(container.querySelectorAll(h), addPermalink);
     });
   }
 }
 
-// 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
+function onMediaQuery(e) {
+  if (e.matches) {
+    // Prevent permalinks from appearing on mobile.
+    document.body.classList.remove('no-permalink');
 
-  document.addEventListener('scroll', function(e) {
-    window.scrollY >= offsetTop ? sidebar.classList.add('sticky') :
-                                  sidebar.classList.remove('sticky');
-  });
+    sidebaroOffsetTop = sidebar.offsetParent.offsetTop
 
-  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');
-      }
+    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');
     }
-  });
+  }
+});
 
-} else {
-  // Prevent permalinks from appearong on mobile.
-  document.body.classList.add('no-permalink');
-
-  articleBody.addEventListener('click', function(e) {
-    if (e.target.localName == 'h2') {
-      e.target.parentElement.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.