feat(viewport-ruler): cache document client rect #2538

devversion · 2017-01-05T15:26:20Z

Now the ViewportRuler service, which is used in many components, caches the documents client rect to avoid frequent recalculation of styles in the browser (basically each click for ripples etc.)

The ScrollDispatcher is used as the service which triggers an update of the cached rectangles because it listens to the resize and scroll events.

Note: It's important that the ViewportRuler also updates the rectangle for scroll events, because the ViewportRuler is also responsible for the scroll position of the viewport.

Debouncing the ScrollDispatcher will be a follow-up PR

mmalerba · 2017-01-05T17:41:05Z

src/lib/core/overlay/position/viewport-ruler.ts

+    this.updateDocumentRectangle();
+
+    // Subscribe to scroll and resize events and update the document rectangle on changes.
+    scrollDispatcher


I think this can all fit on one line

mmalerba · 2017-01-05T17:42:57Z

src/lib/core/overlay/position/viewport-ruler.ts

+  private _documentRect: ClientRect;
+
+  constructor(scrollDispatcher: ScrollDispatcher) {
+


nit: I can't stand extra whitespace, you can leave it if you want but I'll definitely delete it if I ever edit this file :p

Haha will change it. Just in case you'd touch it 😄

mmalerba · 2017-01-05T17:47:26Z

src/lib/core/overlay/position/viewport-ruler.ts

+   * @internal
+   * Updates the cached client rectangle of the document element.
+   */
+  updateDocumentRectangle() {


I think our convention is to name internal methods with _, if that's not possible we add a @docs-private comment. IIRC marking stuff @internal caused issues in google3

Yep; using @internal causes issues with the Closure Compiler, which is why we have the @docs-private annotation. It's easier to just use an underscore prefix, though (without the private access modifier if it's not actually private).

I would also call this something like cacheViewportGeometry; using the "update" verb implies that calling this function will enact some change on the document rectangle instead of reading (and caching) it.

Will change that 👍

mmalerba · 2017-01-05T17:49:01Z

src/lib/core/overlay/position/viewport-ruler.spec.ts

    scrollTo(1500, 2000);
+    ruler.updateDocumentRectangle();


comment why

mmalerba · 2017-01-05T17:49:59Z

src/lib/core/ripple/ripple.spec.ts

@@ -228,6 +232,8 @@ describe('MdRipple', () => {
        document.documentElement.scrollTop = pageScrollTop;
        // Mobile safari
        window.scrollTo(pageScrollLeft, pageScrollTop);
+        // Force an update of the cached rectangle in the ViewportRuler.


jelbourn · 2017-01-05T18:00:38Z

src/lib/core/overlay/position/viewport-ruler.ts

+   * @internal
+   * Updates the cached client rectangle of the document element.
+   */
+  updateDocumentRectangle() {


Yep; using @internal causes issues with the Closure Compiler, which is why we have the @docs-private annotation. It's easier to just use an underscore prefix, though (without the private access modifier if it's not actually private).

I would also call this something like cacheViewportGeometry; using the "update" verb implies that calling this function will enact some change on the document rectangle instead of reading (and caching) it.

jelbourn · 2017-01-05T18:10:15Z

src/lib/core/overlay/position/connected-position-strategy.spec.ts

+    // the real ViewportRuler class because Typescript complains when using a class as an interface.
+    // Also we don't want to extend the ViewportRuler because we'd need to call the constructor
+    // in the superclass. In short: We should use Dependency Injection to fake the ViewportRuler.
+    let fakeViewportRuler: any;


TypeScript is fine with doing implements RealClass; it's one of the language features. The problem is the new private member, which causes any other implementation to automatically fail to match. You should be able to resolve this by marking the private member as optional (example).

(and the internal method as well)

Oh nice! - I wasted so much time trying to solve that.. (considered it as a challenge)

mmalerba · 2017-01-05T18:29:08Z

src/lib/core/overlay/position/viewport-ruler.ts

@@ -55,4 +63,12 @@ export class ViewportRuler {

    return {top, left};
  }
+
+  /**
+   * Caches the latest client rectangle of the document element.


nit: /** one-liners like this */

Changed. Had that because of the @internal from before

kara · 2017-01-06T22:08:03Z

@devversion Can you take another look at this? Tests are failing in Google for buttons and tabs because they can't find ScrollDispatcher. Now that ViewportRuler depends on ScrollDispatcher, you may need to make changes to those modules as well.

Edit: actually nevermind! I think Jeremy's PR might make changing yours unnecessary...

devversion · 2017-01-06T22:29:44Z

@kara Just confirmed that you are right. Our tests didn't cover it because of the FakeViewportRuler classes we had - And yeah waiting for Jeremy's PR would make sense.

Now the `ViewportRuler` service, which is used in many components, caches the documents `BoundingClientRect` to avoid frequent recalculation of styles in the browser (basically each click for ripples etc.) > The `ScrollDispatcher` is used as the service which triggers an update of the cached rectangles because it listens to the `resize` and `scroll` events. It's important that the `ViewportRuler` also updates the rectangle for scroll events, because the ViewportRuler is also responsible for the scroll position of the viewport.

devversion · 2017-01-11T11:00:12Z

@kara @jelbourn Rebased the PR on top of the #2556.

jelbourn · 2017-01-11T17:52:13Z

src/lib/core/overlay/position/viewport-ruler.ts

 }

 export function VIEWPORT_RULER_PROVIDER_FACTORY(parentDispatcher: ViewportRuler) {
-  return parentDispatcher || new ViewportRuler();
+  return parentDispatcher || new ViewportRuler(new ScrollDispatcher());


Instead of doing new ScrollDispatcher(), it should inject the existing scroll dispatcher.
(see MdIconRegistry for an example)

Done - Thanks for the help on Slack again.

jelbourn

LGTM

angular-automatic-lock-bot · 2019-09-06T07:03:03Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

devversion added the pr: needs review label Jan 5, 2017

devversion requested a review from jelbourn January 5, 2017 15:26

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 5, 2017

devversion requested a review from mmalerba January 5, 2017 17:16

mmalerba reviewed Jan 5, 2017

View reviewed changes

jelbourn reviewed Jan 5, 2017

View reviewed changes

mmalerba approved these changes Jan 5, 2017

View reviewed changes

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jan 5, 2017

kara removed the action: merge The PR is ready for merge by the caretaker label Jan 6, 2017

devversion mentioned this pull request Jan 7, 2017

feat: remove the need for forRoot on material NgModules #2556

Merged

devversion force-pushed the feat/viewport-ruler-caching branch from cb5fc15 to a8f88bd Compare January 11, 2017 10:34

jelbourn reviewed Jan 11, 2017

View reviewed changes

devversion added 2 commits January 11, 2017 20:21

Add ScrollDispatcher to provider factory

a275906

Remove unnecessary import

746b70a

jelbourn approved these changes Jan 12, 2017

View reviewed changes

jelbourn added the action: merge The PR is ready for merge by the caretaker label Jan 12, 2017

kara merged commit d0c8f18 into angular:master Jan 31, 2017

devversion deleted the feat/viewport-ruler-caching branch January 31, 2017 09:44

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

		private _documentRect: ClientRect;

		constructor(scrollDispatcher: ScrollDispatcher) {

feat(viewport-ruler): cache document client rect #2538

feat(viewport-ruler): cache document client rect #2538

Uh oh!

Conversation

devversion commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kara commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devversion commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devversion commented Jan 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 6, 2019

Uh oh!

Uh oh!

devversion commented Jan 5, 2017 •

edited

Loading

kara commented Jan 6, 2017 •

edited

Loading

devversion commented Jan 6, 2017 •

edited

Loading