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

biancadanforth · 2019-07-19T03:36:15Z

The width and height properties in the return value from getComputedStyle are of the form '*px'.

danielhertenstein · 2019-07-19T13:03:45Z

Does this mean that rule didn't work exactly properly before?

erikrose · 2019-07-19T15:45:42Z

Yep!

erikrose · 2019-07-19T15:47:51Z

utilsForFrontend.mjs

@@ -468,8 +468,8 @@ export function isVisible(fnodeOrElement) {
        if (style.visibility === 'hidden' ||
            style.display === 'none' ||
            style.opacity === '0' ||
-            style.width === '0' ||
-            style.height === '0') {
+            style.width === '0px' ||


While we're Doing Right Things, we might as well also cover cases like "0%", which I just happened to hit while testing this. My gosh, there are going to be a lot of them. :-( Maybe we should just rely on getBoundingClientRect instead? Would that always show a zero height or width when there is one?

Ah, okay, what if we use a variation on Emilio's initial recommended version (the second one with getBoxQuads()) from the Price Tracker issue which omits getComputedStyle().width/height entirely) with the else statement you have here regarding offscreen elements?

Sounds good. I assume we'd add something to maintain this offscreen-detection functionality from the current version:

// It wasn't hidden based on a computed style. See if it's // offscreen: const rect = element.getBoundingClientRect(); const frame = element.ownerDocument.defaultView; // window or iframe if ((rect.right + frame.scrollX < 0) || (rect.bottom + frame.scrollY < 0)) { return false; }

The updated implementation should cover that scenario, as we window.scrollTo to update the viewport location before checking for clickability.

I actually ended up going with the third option checking for clickability (see "Updating isVisible: second attempt" in this comment), but added the workaround that doesn't require privileged Firefox code. This yields a 53% reduction in Fathom-related jank (see the commit message for more details and actual profiles).

If/when the CSS working group updates element(s)FromPoint to include the option to ignore viewport clipping, then we can modify this method and we'll see the 81% reduction as described in that comment.

biancadanforth · 2019-07-25T03:21:42Z

Hm... isVisible didn't have any test coverage initially, and I don't see anything set up for functional testing. Perhaps that can be a follow-up issue if desired?

erikrose · 2019-07-26T01:11:54Z

Yes, there's no testing right now that uses an actual browser. It would be great to rig some for error-prone things like this function. Please open an issue!

biancadanforth · 2019-07-29T19:36:24Z

Edit: Just amended the commit subject so that it actually closes the issue when merged.

@erikrose Ready for your review! As discussed, this patch includes only the correctness fix and a performance improvement that excludes memoization, which is covered in #121 .

erikrose · 2019-07-30T14:40:47Z

utilsForFrontend.mjs

+    const scrollY = window.pageYOffset;
+    const absX = rect.x + scrollX;
+    const absY = rect.y + scrollY;
+    window.scrollTo(absX, absY);


Does this make the page visibly jump? (That is, after all, its purpose; we're just hoping we can put it back before it redraws.) If not now, is there any guarantee that it won't start visibly jumping in a later Firefox (or other browser)?

Admittedly, this workaround is awkward. Though I got some heartening information from Emilio/the web[1], if the user is scrolling while isVisible happens to be running, there can be jumpiness. I'll have to revise my approach here.

[1]:

I asked Emilio, the graphics/layout engineer who has been helping me analyze alternative approaches:

I think that work around is ok.

Javascript needs to run before rendering, the engine cannot simply
interrupt a script and do layout in order to figure out what changed.

Those calls are sync so I think you're ok.

My only concern is what happens if the user tries to scroll while that
code is running, since scrolling is async. But I think it should be
fine since we only tell the compositor about the main thread scroll
state after painting.

I was reading a document on best practices, and it recommends:

...if you’re in an animation like scrolling, you should ideally be looking to keep your JavaScript to something in the region of 3-4 ms.

When I look at this implementation’s profile in Price Tracker (the Stack Chart, Focus: isVisible), I don’t see any calls exceeding 2 ms, and the average looks to be closer to 1 ms.

erikrose

If we can assert with reasonable confidence that the 2 scrollTo() calls won't make the page visibly jump (like perhaps we know FF never paints while JS is running—though that particular claim is demonstrably false), I'm happy to have this merged. Great performance numbers, Bianca!

erikrose · 2019-07-30T16:31:15Z

utilsForFrontend.mjs

+    window.scrollTo(absX, absY);
+    const newX = absX - window.pageXOffset;
+    const newY = absY - window.pageYOffset;
+    const eles = document.elementsFromPoint(newX, newY);


It's a nit, but I'd go with the full elements or something else unabbreviated here. We don't really have a convention of abbreviating things in this codebase (apart from fnodes, I suppose, but that has a glossary entry of its own).

erikrose · 2019-08-03T00:20:52Z

In today's team meeting, we decided that scrollTo bore too much risk of making UI seem wonky. We decided to land the implementation that makes privileged code very fast while containing an else clause that makes non-privileged code merely correct (which the existing implementation isn't) and somewhat fast.

Unfortunately, the previous commit's approach based on clickability proved untenable.[1] A distant second sync solution is to early return where possible and use the cheapest styles first (the second implementation approach[2]), which reduces Fathom jank by 13%*, though there are a couple differences: * No 'getBoxQuads'. This is an experimental API only enabled on Nightly. * Checks if the element is off-screen. The Price Tracker implementation was missing this check. Unfortunately, this implementation still uses 'ancestors', which causes expensive XRays[3] work in extension applications and still triggers layout flushes at suboptimal times. This is something that can be avoided with an async solution to the tune of a 40% reduction in jank using 'requestAnimationFrame' and 'setTimeout'[4]. On the brighter side, it is more correct than the previous implementation, removing 'getComputedStyle().width' and 'getComputedStyle().height' completely and covering more valid cases than before. *: This is slightly worse than the expected 16%, because my original implementation in Price Tracker did not check for elements off-screen as the Fathom implementation does. Its profile[5] shows: - The largest unresponsive chunk is still caused by Fathom extraction, contributing 399 ms of jank right around page load. - `isVisible` made up 238 ms (60%) of this jank. - This change reduced overall Fathom-related jank by 61 ms (13%) compared to the original implementation of isVisible[2]. [1]: #116 (comment) [2]: mozilla/price-tracker#319 (comment) [3]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision [4]: mozilla/price-tracker#319 (comment) [5]: https://perfht.ml/2T0oYQS

biancadanforth · 2019-08-03T03:38:57Z

@erikrose , would you like me to submit the Firefox-specific solution as part of this PR or in a separate PR?

erikrose · 2019-08-07T16:24:47Z

Let's do a separate one. I almost always lean toward getting *something* landed earlier so people can start enjoying it—and in case we get interrupted. Thanks! :-D

biancadanforth · 2019-08-07T16:44:33Z

Okay thanks @erikrose ; I agree with you on that. It's good to keep PRs small. This will make #122 more reasonable as well. I have requested your review then.

erikrose

Just a couple of little things. Thanks for your thorough pursuit of this! The code looks good. I especially like your use of the ternary operator, which makes it clear that the various variables are initialized despite the branches.

erikrose · 2019-08-07T20:53:30Z

utilsForFrontend.mjs

+ * 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


This part should be moved to a comment inside the function, as putting it in the doclet (the /** block above the function) causes it to be exposed in the rendered documentation.

erikrose · 2019-08-07T20:55:12Z

utilsForFrontend.mjs

 */
 export function isVisible(fnodeOrElement) {
    const element = toDomElement(fnodeOrElement);
+    // Avoid reading ``display: none`` due to Bug 1381071


Great place for knowledge like this.

erikrose · 2019-08-07T21:02:34Z

utilsForFrontend.mjs

+            return false;
+        }
+        // Check if the element is off-screen
+        if (isElement && ((rect.right + frame.scrollX < 0) || (rect.bottom + frame.scrollY < 0))) {


Unless I'm mistaken, you can move this to before the for loop, strip off the isElement condition, and it'll be equivalent.

Yes, I can do that. I also modified the check, as previously, I hadn't checked all four conditions (element is above, to the left, to the right or below the visible portion of the page).

The width and height properties in the return value from getComputedStyle are of the form '*px'.

Compared to a baseline profile[1] of Price Tracker's current 'isVisible' implementation (on which Fathom's 'isVisible' method is based), this clickability approach offers a 53% (146 ms) reduction in Fathom-related jank[2] for the same locally hosted sample page. This is largely due to removing the use of the 'ancestors' Fathom method in 'isVisible'[3], which was performing a lot of redundant layout accesses (and triggering a lot of layout flushes) for the same elements. Also, at least in an extension application, DOM accesses (e.g. repeatedly getting the next 'parentNode' in 'ancestors') are very expensive due to X-Rays[4]. Notes: * If the proposal to the W3C CSS Working Group (see inline comment in patch) is implemented, this clickability approach could forgo the workaround and see as much as 81% (374 ms) reduction in Fathom-related jank[3]. * This implementation can still benefit from memoization, as the same element (e.g. 'div') could be considered for multiple different 'type's[6]. [1]: https://perfht.ml/30wkWT7 [2]: https://perfht.ml/2Y5FCQ1 [3]: mozilla/price-tracker#319 [4]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision" [6]: https://mozilla.github.io/fathom/glossary.html

Unfortunately, the previous commit's approach based on clickability proved untenable.[1] A distant second sync solution is to early return where possible and use the cheapest styles first (the second implementation approach[2]), which reduces Fathom jank by 13%*, though there are a couple differences: * No 'getBoxQuads'. This is an experimental API only enabled on Nightly. * Checks if the element is off-screen. The Price Tracker implementation was missing this check. Unfortunately, this implementation still uses 'ancestors', which causes expensive XRays[3] work in extension applications and still triggers layout flushes at suboptimal times. This is something that can be avoided with an async solution to the tune of a 40% reduction in jank using 'requestAnimationFrame' and 'setTimeout'[4]. On the brighter side, it is more correct than the previous implementation, removing 'getComputedStyle().width' and 'getComputedStyle().height' completely and covering more valid cases than before. *: This is slightly worse than the expected 16%, because my original implementation in Price Tracker did not check for elements off-screen as the Fathom implementation does. Its profile[5] shows: - The largest unresponsive chunk is still caused by Fathom extraction, contributing 399 ms of jank right around page load. - `isVisible` made up 238 ms (60%) of this jank. - This change reduced overall Fathom-related jank by 61 ms (13%) compared to the original implementation of isVisible[2]. [1]: #116 (comment) [2]: mozilla/price-tracker#319 (comment) [3]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision [4]: mozilla/price-tracker#319 (comment) [5]: https://perfht.ml/2T0oYQS

biancadanforth · 2019-08-09T02:35:52Z

@erikrose , This is ready for another look!

erikrose · 2019-08-09T15:36:13Z

Oh noes! Since you force-pushed, now I don't know what's new anymore. :-( Let me see if I can figure it out.

biancadanforth · 2019-08-09T15:47:48Z

@erikrose Just the last two commits are new.

erikrose · 2019-08-09T15:48:29Z

Thanks. Just as soon as I said that and reloaded the page, GitHub started showing me "These commits are new since you reviewed". :-)

biancadanforth · 2019-08-09T15:51:23Z

Yeah I was just editing the commit subject from one of the earlier commits to be more accurate. I'll try to keep rebasing to a minimum to avoid confusion.

erikrose · 2019-08-09T15:38:29Z

utilsForFrontend.mjs

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


Why parens here but not in the sum above?

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

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.

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.

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

erikrose · 2019-08-09T16:54:52Z

utilsForFrontend.mjs

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


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.

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.

erikrose · 2019-08-09T16:57:49Z

utilsForFrontend.mjs

    const frame = element.ownerDocument.defaultView;
+    if (elementRect.x + elementRect.width < 0
+        || (elementRect.y + elementRect.height) < 0
+        || (elementRect.x > frame.innerWidth || elementRect.y > frame.innerHeight)


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).

* Add a new eslint rule, 'operator-linebreak: [error, after]' in light of style-related feedback and fixed violations * Ensure comments are complete sentences and are consistent * Remove extra parens from conditional statements * Update the first check in 'isVisible' to account for an extra case I found while working on #122

biancadanforth · 2019-08-10T20:09:54Z

@erikrose ready for another pass!

erikrose · 2019-08-12T15:05:58Z

utilsForFrontend.mjs

@@ -160,8 +160,8 @@ export function *inlineTexts(element, shouldTraverse = element => true) {
    for (let child of walk(element,
                           element => !(isBlock(element) ||
                                        element.tagName === 'SCRIPT' &&
-                                        element.tagName === 'STYLE')
-                                      && shouldTraverse(element))) {
+                                        element.tagName === 'STYLE') &&


I think this actually make it harder to understand. I don't remember writing this (though I probably did), but here's how I read the original:

The ! lines up with the &&, suggesting that 2 clauses are ANDed together. That's what's actually going on.

When you move the 2nd &&, the 2 &&s line up exactly, and it starts looking like shouldTraverse is just one more clause in the earlier &&, unless you take the time to painstakingly balance the parens.

So I think the original indentation visually conveys the structure more clearly. Does that make sense?

I see what you mean, though that prevents us from having an eslint rule to cover it, as leaving this as-is is not consistent (from eslint's perspective) with changing the other one.

I'm happy to change it back as you prefer, however.

erikrose

So, unless you see a reason not to, can you roll back the utilsForFrontEnd change and the lint rule? And I have that one question about the !== vs. ===. Then we should be ready to merge at last! Thanks, Bianca!

erikrose · 2019-08-12T16:07:41Z

utilsForFrontend.mjs

-    if (elementRect.width === 0 && elementRect.height === 0) {
+    const elementStyle = getComputedStyle(element);
+    // Alternative to reading ``display: none`` due to Bug 1381071.
+    if (elementRect.width === 0 && elementRect.height === 0 && elementStyle.overflow !== 'hidden') {


Shouldn't this be elementStyle.overflow === 'hidden'? As you say below, "Zero-sized ancestors don’t make descendants hidden unless the descendant has overflow: hidden".

The check you're thinking of is done inside the for loop below. This first check is a proxy for checking display: none, for which just checking width and height === 0 is not sufficient as I learned while working on #122 .

Ah, so this is a check strictly for the visibility of the element passed in, not any of its descendents. A child, for example, could still be visible. Callers will have to be aware of this. That'd be worth documenting in the doclet, as I wasn't front-end-savvy enough to realize it.

Hm... I'm not sure I follow what you want me to document. In general, this method is to check if the element passed in is visible. It doesn't make any claims about that element's descendants.

We discussed this synchronously, and we're just going to leave off any additional comments for now.

biancadanforth · 2019-08-12T20:34:12Z

@erikrose , Ready for another look.

erikrose

Merge it! I'm sorry the review went on so long, but I think we've got something good here. Looking forward to the tests landing as well. (In the future, of course, we'll try to land them together.) Thanks, Bianca!

erikrose · 2019-08-13T01:46:36Z

utilsForFrontend.mjs

-    if (elementRect.width === 0 && elementRect.height === 0) {
+    const elementStyle = getComputedStyle(element);
+    // Alternative to reading ``display: none`` due to Bug 1381071.
+    if (elementRect.width === 0 && elementRect.height === 0 && elementStyle.overflow !== 'hidden') {


Ah, so this is a check strictly for the visibility of the element passed in, not any of its descendents. A child, for example, could still be visible. Callers will have to be aware of this. That'd be worth documenting in the doclet, as I wasn't front-end-savvy enough to realize it.

biancadanforth requested a review from erikrose July 19, 2019 03:36

erikrose suggested changes Jul 19, 2019

View reviewed changes

biancadanforth mentioned this pull request Jul 19, 2019

Make Fathom extraction more performant mozilla/price-tracker#319

Open

biancadanforth force-pushed the is-visible branch from 2290a34 to a711471 Compare July 24, 2019 20:50

biancadanforth changed the title ~~Use correct values in isVisible condition~~ Fix #118: Improve isVisible for correctness and performance Jul 24, 2019

biancadanforth force-pushed the is-visible branch from a711471 to 2c76c08 Compare July 25, 2019 02:59

biancadanforth mentioned this pull request Jul 29, 2019

Add JavaScript functional test harness #122

Closed

biancadanforth requested a review from erikrose July 29, 2019 19:35

biancadanforth force-pushed the is-visible branch from 2c76c08 to 6d8c030 Compare July 29, 2019 19:40

erikrose reviewed Jul 30, 2019

View reviewed changes

biancadanforth requested a review from erikrose August 7, 2019 16:43

erikrose suggested changes Aug 7, 2019

View reviewed changes

biancadanforth added 5 commits August 8, 2019 18:09

Use correct values in isVisible condition

68ce4cd

The width and height properties in the return value from getComputedStyle are of the form '*px'.

Fix #118: Incorporate review feedback

c912f13

Fix eslint warnings from #51.

23f86cf

biancadanforth force-pushed the is-visible branch from 5e4d0b6 to 23f86cf Compare August 9, 2019 02:29

biancadanforth requested a review from erikrose August 9, 2019 02:30

biancadanforth mentioned this pull request Aug 9, 2019

WIP - add JS functional test harness biancadanforth/fathom#2

Merged

erikrose suggested changes Aug 9, 2019

View reviewed changes

biancadanforth requested a review from erikrose August 10, 2019 20:08

biancadanforth mentioned this pull request Aug 10, 2019

#122: Add JavaScript functional test harness #129

Merged

erikrose reviewed Aug 12, 2019

View reviewed changes

erikrose suggested changes Aug 12, 2019

View reviewed changes

Revert operator-linebreak eslint rule

b96c4e3

biancadanforth requested a review from erikrose August 12, 2019 20:33

erikrose approved these changes Aug 13, 2019

View reviewed changes

biancadanforth merged commit 871728f into master Aug 13, 2019

biancadanforth deleted the is-visible branch August 13, 2019 23:01

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

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

Conversation

biancadanforth commented Jul 19, 2019

danielhertenstein commented Jul 19, 2019

erikrose commented Jul 19, 2019

Choose a reason for hiding this comment

biancadanforth Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

biancadanforth commented Jul 25, 2019

erikrose commented Jul 26, 2019

biancadanforth commented Jul 29, 2019 • edited Loading

erikrose Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikrose commented Aug 3, 2019

biancadanforth commented Aug 3, 2019

erikrose commented Aug 7, 2019 via email

biancadanforth commented Aug 7, 2019

erikrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth commented Aug 9, 2019

erikrose commented Aug 9, 2019

biancadanforth commented Aug 9, 2019

erikrose commented Aug 9, 2019 via email

biancadanforth commented Aug 9, 2019

Choose a reason for hiding this comment

biancadanforth Aug 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth commented Aug 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biancadanforth commented Aug 12, 2019

erikrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

biancadanforth Jul 19, 2019 •

edited

Loading

biancadanforth Jul 29, 2019 •

edited

Loading

biancadanforth commented Jul 29, 2019 •

edited

Loading

erikrose Jul 30, 2019 •

edited

Loading

biancadanforth Aug 10, 2019 •

edited

Loading