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.

Sign up for GitHub

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

Fix #118: Improve isVisible for correctness and performance #116

Merged
merged 7 commits into from
Aug 13, 2019
Merged

Fix #118: Improve isVisible for correctness and performance #116

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
68ce4cd
Use correct values in isVisible condition
biancadanforth Jul 19, 2019
335657e
#118: Change isVisible to check for clickability to improve performance
biancadanforth Jul 24, 2019
cfd7a50
#118: Fall back to next best sync solution
biancadanforth Aug 3, 2019
c912f13
Fix #118: Incorporate review feedback
biancadanforth Aug 9, 2019
23f86cf
Fix eslint warnings from #51.
biancadanforth Aug 9, 2019
c0a81db
Incorporate more review feedback
biancadanforth Aug 10, 2019
b96c4e3
Revert operator-linebreak eslint rule
biancadanforth Aug 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/demos.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {assert} from 'chai';

import {dom, out, rule, ruleset, type} from '../index';
import {dom, rule, ruleset, type} from '../index';
import {sigmoid, staticDom} from '../utils';


Expand Down
2 changes: 1 addition & 1 deletion test/lhs_tests.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {assert} from 'chai';

import {dom, rule, ruleset, out, type} from '../index';
import {dom, rule, ruleset, type} from '../index';
import {staticDom} from '../utils';


Expand Down
2 changes: 1 addition & 1 deletion test/ruleset_tests.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {assert} from 'chai';

import {distance} from '../clusters';
import {and, dom, nearest, out, props, rule, ruleset, score, type} from '../index';
import {and, dom, nearest, props, rule, ruleset, score, type} from '../index';
import {domSort, sigmoid, staticDom} from '../utils';


Expand Down
2 changes: 1 addition & 1 deletion test/utils_tests.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {assert} from 'chai';
import {dom, out, rule, ruleset, score, type} from '../index';
import {dom, rule, ruleset, score, type} from '../index';
import {NiceSet, toposort, staticDom, attributesMatch} from '../utils';


Expand Down
15 changes: 8 additions & 7 deletions utilsForFrontend.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,11 +460,9 @@ export function sigmoid(x) {
/**
* Return whether an element is practically visible, considering things like 0
* size or opacity, ``visibility: hidden`` and ``overflow: hidden``.
*
* This could be 5x more efficient if https://github.com/w3c/csswg-drafts/issues/4122
* happens.
*/
export function isVisible(fnodeOrElement) {
// This could be 5x more efficient if https://github.com/w3c/csswg-drafts/issues/4122 happens.
const element = toDomElement(fnodeOrElement);
// Avoid reading ``display: none`` due to Bug 1381071
Copy link
Contributor

Choose a reason for hiding this comment

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

Great place for knowledge like this.

const elementRect = element.getBoundingClientRect();
Expand All @@ -475,7 +473,14 @@ export function isVisible(fnodeOrElement) {
if (elementStyle.visibility === 'hidden') {
return false;
}
// Check if the element is off-screen
Copy link
Contributor

Choose a reason for hiding this comment

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

I was saying to Daniel the other day that I like to punctuate comments like sentences if they are, in fact, sentences. I think it's influence from https://www.python.org/dev/peps/pep-0008/#comments. But on comments like this that take up the whole line and refer to code below, I often stick a colon afterward. That way, even if you don't skip a line before, it's clear you're talking about code only below it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can get behind the complete sentence thing. Not sure if I will remember your preference around colons, but I'll try to keep that in mind...I added a colon.

const frame = element.ownerDocument.defaultView;
if (elementRect.x + elementRect.width < 0
|| (elementRect.y + elementRect.height) < 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why parens here but not in the sum above?

Also, if you put the || on the right, these similar lines will line up nicely.

Copy link
Collaborator Author

@biancadanforth biancadanforth Aug 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Do we want to add an eslint rule on parens? e.g. no-extra-parens: error I have no strong opinion. It seems like you may want more nuance than that based on the violations that come up if I were to add a strict ban on extra parens.

It sounds like you do have an opinion about operators in conditional statements, so I added an eslint rule for that in .eslintrc.yml: operator-linebreak: [error, after] and fixed the one other instance of this elsewhere in utilsForFrontend.mjs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a benefit in making everybody memorize the precedence of every little operator, which would seem to be a requirement for no-extra-parens. I'm just looking for consistency. When I see inconsistency, I think "Aha, the author is trying to show me that something is different about this other case," and I waste time looking for it. That's all.

Copy link
Contributor

Choose a reason for hiding this comment

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

As for the operator placement, I was just trying to make the very similar lines line up for faster scanning.

|| (elementRect.x > frame.innerWidth || elementRect.y > frame.innerHeight)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the parens around this pair of conditions shouldn't be here, since you didn't put them around the top pair (and they don't affect the logic).

) {
return false;
}
for (const ancestor of ancestors(element)) {
const isElement = ancestor === element;
const style = isElement ? elementStyle : getComputedStyle(ancestor);
Expand All @@ -493,10 +498,6 @@ export function isVisible(fnodeOrElement) {
// has overflow: hidden
return false;
}
// Check if the element is off-screen
if (isElement && ((rect.right + frame.scrollX < 0) || (rect.bottom + frame.scrollY < 0))) {
return false;
}
}
return true;
}
Expand Down