Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate the highlighter layer #34

Merged
merged 7 commits into from
Apr 24, 2018

Conversation

rajatvijay
Copy link
Contributor

@rajatvijay rajatvijay commented Apr 5, 2018

This is in reference to the issue #3.

Important:

  1. The strategy I am using here is to inject a div with position: absolute at the bottom of the body of the webpage. This will be used as the highlighterLayer.
  2. I have removed the color picker for the outline since the highlighterLayer removes the requirement of showing the outline at all.
  3. I have removed the unhighlightNode method since we don't have to unhighlight each node separately, hiding the highlighterLayer can fulfill the purpose. Thus the method unhighlightAll is the only way to unhighlight.
  4. Since we are not modifying the node for the purpose of highlighting. I have removed the STYLING_ATTRIBUTE and all its references entirely. Instead, I am using a new global constant HIGHLIGHTER_LAYER_ID to uniquely identify the layer for the purposes of highlighting and unhighlighting.

for (let node of currentNodes) {
unhighlightNode(node);
let highlighterLayer;
if (highlighterLayer = document.querySelector(`#${HIGHLIGHTER_LAYER_ID}`)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of assignments within conditions.
Could you change to:

let highlighterLayer = document.querySelector(`#${HIGHLIGHTER_LAYER_ID}`);
if (highlighterLayer) {
  highighterLayer.classList.add("hide");
}

height: ${height}px;`;

let highlightLayer;
if (!(highlightLayer = document.querySelector(`#${HIGHLIGHTER_LAYER_ID}`))) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as before. Also, it would be better to move this to another function like:

function getHighlighterLayer() {
  let highlighterLayer = document.querySelector(`#${HIGHLIGHTER_LAYER_ID}`);\
  if (highlighterLayer) {
    return highlighterLayer;
  }
 
  highlightLayer = document.createElement("div");
  highlightLayer.id = HIGHLIGHTER_LAYER_ID;
  document.body.appendChild(highlightLayer);
  return highlighterLayer;
}

This way you can just call this function anytime you need to access the layer.

@@ -285,17 +283,25 @@ let nextUnique = (function uniqueNumberGenerator() {
* @param {DOMNode} node The node to be highlighted.
*/
function highlightNode(node) {
node.setAttribute(STYLING_ATTRIBUTE, true);
updateOutline(node);
const {top, left, width, height} = node.getBoundingClientRect()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we will need something more complex here. This does not work when you scroll the page (the layer appears misplaced). And I think it will also fail when there are nested iframes.

There's another API in Firefox that makes this much easier, but I don't know if it will work outside of nightly: node.getBoxQuads().
In this particular case, it could be used like this:
node.getBoxQuads({ relativeTo: document.documentElement })[0].bounds
But, my recollection is that it only works on Nightly (or if the caller is chrome-privileged, which I think addons aren't).

So, if this function can't be used, we'll need to extract this logic to a separate function like getNodeAbsoluteCoordinates(node) or something like that, and in this function, have all the code needed to take scroll and nested iframes into account.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@captainbrosset

I have fixed the top two comments. For this one, we can use:

function getNodeAbsoluteCoordinates(node) {
  var rect = node.getBoundingClientRect(),
  return { 
    top: rect.top + window.pageXOffset, 
    left: rect.left + window.pageYOffset
  }
}

Copy link
Owner

@captainbrosset captainbrosset left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I got confused with GitHub's review workflow, and thought I could approve/request changes per commit. Turns out you can't. So there are comments that are marked as "outdated" , simply because they are about pieces of code that have been later changed by other commits).

@captainbrosset
Copy link
Owner

The latest changes look good to me. We still have problems when the page is scrolled, but we could look at that in a later commit.

@rajatvijay
Copy link
Contributor Author

@captainbrosset

I have fixed the top two comments. For this one, we can use:

function getNodeAbsoluteCoordinates(node) {
  var rect = node.getBoundingClientRect(),
  return { 
    top: rect.top + window.pageXOffset, 
    left: rect.left + window.pageYOffset
  }
}

@captainbrosset
Copy link
Owner

Do you mind pushing a new commit with this new function then? I'll merge the PR then.
Thanks!

@captainbrosset captainbrosset merged commit fe2491a into captainbrosset:master Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants