Issue 2879 - Move element selection into the content process

lib/child/selection.js

 /*

[saroyanm, 2016/11/23 17:44:39]

I thought that this code belongs to Aardvark ?
What is the reason of separation ?
My assumption: It's related to e10s. How we separate processes ?

[Wladimir Palant, 2016/11/24 14:02:01]

Yes, E10S is the reason for this whole change. Any code accessing webpages needs
to go into the child process. The parent on the other hand can mess with the
browser UI (e.g. our tooltips). The two subsystems can communicate via
messaging.

  * This Source Code is subject to the terms of the Mozilla Public License
  * version 2.0 (the "License"). You can obtain a copy of the License at
  * http://mozilla.org/MPL/2.0/.
  */
 
 "use strict";
 
 let {Services} = Cu.import("resource://gre/modules/Services.jsm", {});
 
 let messageManager = require("messageManager");
 let {
   createElement, getWindowSize, getParentElement, getElementPosition
 } = require("./utils");
 
+/**
+ * @typedef State
+ * @type {Object}
+ * @property {Window} window
+ *   The top-level window that we are selecting in.
+ * @property {Element} boxElement
+ *   The element marking the current selection.
+ * @property {Element} anchorElement
+ *   The element that received the last mouse event.
+ * @property {Element} selectedElement
+ *   The element currently selected (usually anchorElement or a parent element
+ *   of it).
+ * @property {boolean} isUserSelected
+ *   Will be true if the user narrowed down the selection to a specific element
+ *   using wider/narrower commands.
+ * @property {Element} lockedAnchor
+ *   When selection is locked, this property will be updated by mouse events
+ *   instead of anchorElement.
+ * @property {number} prevSelectionUpdate
+ *   Time of previous selection update, used for rate limiting.
+ * @property {Object} prevPos
+ *   Position and size of selected element during previous selection update.
+ */
+
+/**
+ * Current selection state. This object will be empty if no selection is
+ * currently in progress.
+ *
+ * @type {State}
+ */
 let state = exports.state = {};

[Wladimir Palant, 2016/11/17 13:54:48]

Original code was storing the selection state as properties of the Aardvark
object. I thought that a separate object for that would be better, already
because resetting state will be much easier then. Unfortunately, resetting is a
bit more complicated than I hoped for - this object is exported, so we cannot
just set it to null or replace with a different object.

[saroyanm, 2016/11/23 17:44:39]

We are setting the properties of state in other function and it's not obvious
what properties it should hold, why not to assign null, so it would be obvious
what properties state suppose to have ? Or at least document them.

[Wladimir Palant, 2016/11/24 14:02:02]

Setting them here doesn't make sense - the definition of an empty state is that
no properties are there (which also makes it easy to clean it up). But I guess
that we can document them now, the naming is pretty non-obvious.

 
 messageManager.addMessageListener("ElemHideHelper:StartSelection", startSelection);
 
 onShutdown.add(() =>
 {
   messageManager.removeMessageListener("ElemHideHelper:StartSelection", startSelection);
 
   stopSelection();
 });
 
 function startSelection(message)
 {
   stopSelection();
 
   let outerWindowID = message.data;
   let wnd = Services.wm.getOuterWindowWithId(outerWindowID);
   if (!wnd || !canSelect(wnd))
     return;
 
   state.window = wnd;
-  state.id = outerWindowID;
 
   wnd.addEventListener("click", onMouseClick, true);
   wnd.addEventListener("wheel", onMouseScroll, true);

[saroyanm, 2016/11/23 17:44:39]

It's a deprecated event:
https://developer.mozilla.org/en-US/docs/Web/API/MouseWheelEvent 

According to the description of "mousemove" event this should be covered by
MouseMove event:
https://developer.mozilla.org/en/docs/Web/Events/mousemove

[Wladimir Palant, 2016/11/24 14:02:02]

That page is about the mousewheel event, not wheel. You should be looking here:
https://developer.mozilla.org/en-US/docs/Web/API/WheelEvent

   wnd.addEventListener("mousemove", onMouseMove, true);
   wnd.addEventListener("pagehide", onPageHide, true);
 
   wnd.focus();
 
   let doc = wnd.document;
   let {elementMarkerClass} = require("info");
   state.boxElement = createElement(doc, "div", {"class": elementMarkerClass}, [
     createElement(doc, "div", {"class": "ehh-border"}),
     createElement(doc, "div", {"class": "ehh-label"}, [
@@ skipping 25 matching lines @@
 
   for (let key of Object.keys(state))
     delete state[key];
 
   messageManager.sendAsyncMessage("ElemHideHelper:SelectionStopped");
 }
 exports.stopSelection = stopSelection;
 
 function canSelect(wnd)
 {
-  let acceptlocalfiles;

[saroyanm, 2016/11/23 17:44:39]

Nit: camelCase is missing

[Wladimir Palant, 2016/11/24 14:02:02]

Done.

+  let acceptLocalFiles;
   try
   {
-    acceptlocalfiles = Services.prefs.getBoolPref("extensions.elemhidehelper.acceptlocalfiles");

[saroyanm, 2016/11/23 17:44:39]

Nit: exceeding 80 chars.

[Wladimir Palant, 2016/11/24 14:02:01]

Done.

+    let pref = "extensions.elemhidehelper.acceptlocalfiles";
+    acceptLocalFiles = Services.prefs.getBoolPref(pref);
   }
   catch (e)
   {
-    acceptlocalfiles = false;
-  }
-
-  if (!acceptlocalfiles)
+    acceptLocalFiles = false;
+  }
+
+  if (!acceptLocalFiles)
   {
     let localSchemes;
     try
     {
       localSchemes = new Set(
         Services.prefs.getCharPref("extensions.adblockplus.whitelistschemes")
                       .split(/\s+/)
       );

[Wladimir Palant, 2016/11/17 13:54:48]

This changes the original logic which was simply hardcoding a bunch of schemes.
Adblock Plus exposes the schemes where element hiding won't apply so we can just
use that preference.

     }
     catch (e)
     {
       localSchemes = new Set();
     }
 
     if (localSchemes.has(wnd.location.protocol.replace(/:$/, "")))
       return false;
   }
 
   return true;
 }
 
 function getElementLabel(elem)
 {
   let tagName = elem.localName;

[saroyanm, 2016/11/23 17:44:39]

localName is a deprecated property:
https://developer.mozilla.org/en-US/docs/Web/API/Node/localName

[Wladimir Palant, 2016/11/24 14:02:02]

That would have been surprising - the docs say that it was removed in Firefox 48
yet I tested this code path successfully. In fact, it says below that only
Node.localName is deprecated. Element.localName on the other hand is alive and
kicking: https://developer.mozilla.org/en-US/docs/Web/API/Element/localName

   let addition = "";
   if (elem.id != "")
     addition += ", id: " + elem.id;
   if (elem.className != "")
     addition += ", class: " + elem.className;
   if (elem.style.cssText != "")
     addition += ", style: " + elem.style.cssText;

[saroyanm, 2016/11/23 17:44:39]

Weak suggestion: What about mentioning that it's na inline-style not style in
general, because we are not showing the styles applied by ID selectors and
Classnames, may lead to confusion.

[Wladimir Palant, 2016/11/24 14:02:02]

Hasn't lead to any confusion so far, we can just as well leave it then :)

 
   return [tagName, addition];
 }
 
 function setAnchorElement(anchor)

[saroyanm, 2016/11/23 17:44:39]

What is the anchor element ? I assume it's not related to "a tag"?

[Wladimir Palant, 2016/11/24 14:02:01]

Nope. I've documented the state properties now, should hopefully make it more
obvious.

 {
   state.anchorElement = anchor;
 
   let newSelection = anchor;
   if (state.isUserSelected)
   {
     // User chose an element via wider/narrower commands, keep the selection if
     // our new anchor is still a child of that element
     let e = newSelection;
     while (e && e != state.selectedElement)
@@ skipping 16 matching lines @@
 
   let border = state.boxElement.querySelector(".ehh-border");
   let label = state.boxElement.querySelector(".ehh-label");
   let labelTag = state.boxElement.querySelector(".ehh-labelTag");
   let labelAddition = state.boxElement.querySelector(".ehh-labelAddition");
 
   let doc = state.window.document;
   let [wndWidth, wndHeight] = getWindowSize(state.window);
 
   let pos = getElementPosition(elem);
   state.boxElement.style.left = Math.min(pos.left - 1, wndWidth - 2) + "px";

[saroyanm, 2016/11/23 17:44:38]

Is this approach even working with RTL websites ?
seems like we are only considering the LTR direction.

[Wladimir Palant, 2016/11/24 14:02:01]

Well, it works with RTL websites. Our UI is always displayed in LTR direction
however, even if extension locale (which is what's relevant here, not the
webpage) is Hebrew. We probably want to file an issue on that, not trivial to
fix however.

[saroyanm, 2016/11/25 16:18:13]

Here you go: #4665

   state.boxElement.style.top = Math.min(pos.top - 1, wndHeight - 2) + "px";
   border.style.width = Math.max(pos.right - pos.left - 2, 0) + "px";
   border.style.height = Math.max(pos.bottom - pos.top - 2, 0) + "px";
 
   [labelTag.textContent, labelAddition.textContent] = getElementLabel(elem);
 
   // If there is not enough space to show the label move it up a little
   if (pos.bottom < wndHeight - 25)
     label.className = "ehh-label";
   else
@@ skipping 26 matching lines @@
 
   require("./commands").select();
   event.preventDefault();
 }
 
 function onMouseScroll(event)
 {
   if (!event.shiftKey || event.altKey || event.ctrlKey || event.metaKey)
     return;
 
   let delta = event.deltaY || event.deltaX;

[Wladimir Palant, 2016/11/17 13:54:48]

This was dead code originally, using DOMMouseScroll event that became
unsupported. I didn't want to commit an untested dead codepath so I changed it
to use the wheel event.

I found that scrolling vertically via trackpad would set event.deltaY. Scrolling
with a hardware mouse and a mouse wheel however would set event.deltaX. We
require Shift to be pressed here and at least on OS X it changes the scrolling
direction for the mouse wheel. That's why I had to accept both vertical and
horizontal scrolling here, unlike the original code.

[saroyanm, 2016/11/23 17:44:38]

Acknowledged.

   if (!delta)
     return;
 
   let commands = require("./commands");

[saroyanm, 2016/11/23 17:44:38]

Detail: we already loading the module in onMouseClick event, why not to load it
in the beginning of the file ?

[Wladimir Palant, 2016/11/24 14:02:01]

That would create a circular reference.

   if (delta > 0)
     commands.wider();
   else
     commands.narrower();
   event.preventDefault();
 }
 
 function onMouseMove(event)
 {
   hideSelection();
@@ skipping 31 matching lines @@
       selectElement(state.selectedElement);
     }
   }
 }
 
 function onPageHide(event)
 {
   stopSelection();
 }
 
 function onAfterPaint(event)

[Wladimir Palant, 2016/11/17 13:54:48]

This code path is untested, I'm not sure whether it works at all. Its original
purpose was keeping up with scrolling but it definitely doesn't do that any more
- not in the original code either. This will need to be fixed later, we need to
listen to other events.

[saroyanm, 2016/11/23 17:44:39]

We do have "onMouseScroll" function, I thought it should take care of your
mentioned issue.

I'm not really sure if we need this event, sounds like it's being fired whenever
something is being changed/drawn on the screen, feels like we are covering all
edge cases, but yes maybe we can discuss that in separate issue.

[Wladimir Palant, 2016/11/24 14:02:02]

Given that the purpose of this function is selecting via mouse wheel - nope.
It's merely a confusing name that we should change occasionally.
The idea was covering things like DOM modifications and scrolling here. I'm
fairly certain that it was working, that was a long time ago however. So - yes,
we should definitely look into this but it's not a very straightforward change.

 {
   // Don't update position too often
   if (state.selectedElement && Date.now() - state.prevSelectionUpdate > 20)
   {
     let pos = getElementPosition(state.selectedElement);
     if (!state.prevPos || state.prevPos.left != pos.left ||
         state.prevPos.right != pos.right || state.prevPos.top != pos.top ||
         state.prevPos.bottom != pos.bottom)
     {
       selectElement(state.selectedElement);
     }
   }
 }