Skip to content

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

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

Merged
merged 3 commits into from
Jan 31, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
17 changes: 14 additions & 3 deletions src/lib/core/overlay/position/connected-position-strategy.spec.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import {ElementRef} from '@angular/core';
import {ConnectedPositionStrategy} from './connected-position-strategy';
import {ViewportRuler} from './viewport-ruler';
import {ViewportRuler, VIEWPORT_RULER_PROVIDER} from './viewport-ruler';
import {OverlayPositionBuilder} from './overlay-position-builder';
import {ConnectedOverlayPositionChange} from './connected-position';
import {Scrollable} from '../scroll/scrollable';
import {Subscription} from 'rxjs';
import {TestBed, inject} from '@angular/core/testing';
import Spy = jasmine.Spy;


Expand All @@ -18,6 +19,16 @@ const DEFAULT_WIDTH = 60;

describe('ConnectedPositionStrategy', () => {

let viewportRuler: ViewportRuler;

beforeEach(() => TestBed.configureTestingModule({
providers: [VIEWPORT_RULER_PROVIDER]
}));

beforeEach(inject([ViewportRuler], (_ruler: ViewportRuler) => {
viewportRuler = _ruler;
}));

describe('with origin on document body', () => {
const ORIGIN_HEIGHT = DEFAULT_HEIGHT;
const ORIGIN_WIDTH = DEFAULT_WIDTH;
Expand Down Expand Up @@ -48,7 +59,7 @@ describe('ConnectedPositionStrategy', () => {
overlayContainerElement.appendChild(overlayElement);

fakeElementRef = new FakeElementRef(originElement);
positionBuilder = new OverlayPositionBuilder(new ViewportRuler());
positionBuilder = new OverlayPositionBuilder(viewportRuler);
});

afterEach(() => {
Expand Down Expand Up @@ -457,7 +468,7 @@ describe('ConnectedPositionStrategy', () => {
scrollable.appendChild(originElement);

// Create a strategy with knowledge of the scrollable container
let positionBuilder = new OverlayPositionBuilder(new ViewportRuler());
let positionBuilder = new OverlayPositionBuilder(viewportRuler);
let fakeElementRef = new FakeElementRef(originElement);
strategy = positionBuilder.connectedTo(
fakeElementRef,
Expand Down
24 changes: 17 additions & 7 deletions src/lib/core/overlay/position/viewport-ruler.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {ViewportRuler} from './viewport-ruler';

import {ViewportRuler, VIEWPORT_RULER_PROVIDER} from './viewport-ruler';
import {TestBed, inject} from '@angular/core/testing';

// For all tests, we assume the browser window is 1024x786 (outerWidth x outerHeight).
// The karma config has been set to this for local tests, and it is the default size
Expand All @@ -20,10 +20,14 @@ describe('ViewportRuler', () => {
veryLargeElement.style.width = '6000px';
veryLargeElement.style.height = '6000px';

beforeEach(() => {
ruler = new ViewportRuler();
beforeEach(() => TestBed.configureTestingModule({
providers: [VIEWPORT_RULER_PROVIDER]
}));

beforeEach(inject([ViewportRuler], (viewportRuler: ViewportRuler) => {
ruler = viewportRuler;
scrollTo(0, 0);
});
}));

it('should get the viewport bounds when the page is not scrolled', () => {
let bounds = ruler.getViewportRect();
Expand All @@ -35,7 +39,10 @@ describe('ViewportRuler', () => {

it('should get the viewport bounds when the page is scrolled', () => {
document.body.appendChild(veryLargeElement);

scrollTo(1500, 2000);
// Force an update of the cached viewport geometries because IE11 emits the scroll event later.
ruler._cacheViewportGeometry();

let bounds = ruler.getViewportRect();

Expand Down Expand Up @@ -63,14 +70,17 @@ describe('ViewportRuler', () => {
});

it('should get the scroll position when the page is not scrolled', () => {
var scrollPos = ruler.getViewportScrollPosition();
let scrollPos = ruler.getViewportScrollPosition();
expect(scrollPos.top).toBe(0);
expect(scrollPos.left).toBe(0);
});

it('should get the scroll position when the page is scrolled', () => {
document.body.appendChild(veryLargeElement);

scrollTo(1500, 2000);
// Force an update of the cached viewport geometries because IE11 emits the scroll event later.
ruler._cacheViewportGeometry();

// In the iOS simulator (BrowserStack & SauceLabs), adding the content to the
// body causes karma's iframe for the test to stretch to fit that content once we attempt to
Expand All @@ -82,7 +92,7 @@ describe('ViewportRuler', () => {
return;
}

var scrollPos = ruler.getViewportScrollPosition();
let scrollPos = ruler.getViewportScrollPosition();
expect(scrollPos.top).toBe(2000);
expect(scrollPos.left).toBe(1500);

Expand Down
26 changes: 20 additions & 6 deletions src/lib/core/overlay/position/viewport-ruler.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {Injectable, Optional, SkipSelf} from '@angular/core';
import {ScrollDispatcher} from '../scroll/scroll-dispatcher';


/**
Expand All @@ -7,12 +8,20 @@ import {Injectable, Optional, SkipSelf} from '@angular/core';
*/
@Injectable()
export class ViewportRuler {
// TODO(jelbourn): cache the document's bounding rect and only update it when the window
// is resized (debounced).

/** Cached document client rectangle. */
private _documentRect?: ClientRect;

constructor(scrollDispatcher: ScrollDispatcher) {
// Initially cache the document rectangle.
this._cacheViewportGeometry();

// Subscribe to scroll and resize events and update the document rectangle on changes.
scrollDispatcher.scrolled().subscribe(() => this._cacheViewportGeometry());
}

/** Gets a ClientRect for the viewport's bounds. */
getViewportRect(): ClientRect {
getViewportRect(documentRect = this._documentRect): ClientRect {
// Use the document element's bounding rect rather than the window scroll properties
// (e.g. pageYOffset, scrollY) due to in issue in Chrome and IE where window scroll
// properties and client coordinates (boundingClientRect, clientX/Y, etc.) are in different
Expand All @@ -22,7 +31,6 @@ export class ViewportRuler {
// We use the documentElement instead of the body because, by default (without a css reset)
// browsers typically give the document body an 8px margin, which is not included in
// getBoundingClientRect().
const documentRect = document.documentElement.getBoundingClientRect();
const scrollPosition = this.getViewportScrollPosition(documentRect);
const height = window.innerHeight;
const width = window.innerWidth;
Expand All @@ -42,7 +50,7 @@ export class ViewportRuler {
* Gets the (top, left) scroll position of the viewport.
* @param documentRect
*/
getViewportScrollPosition(documentRect = document.documentElement.getBoundingClientRect()) {
getViewportScrollPosition(documentRect = this._documentRect) {
// The top-left-corner of the viewport is determined by the scroll position of the document
// body, normally just (scrollLeft, scrollTop). However, Chrome and Firefox disagree about
// whether `document.body` or `document.documentElement` is the scrolled element, so reading
Expand All @@ -54,10 +62,16 @@ export class ViewportRuler {

return {top, left};
}

/** Caches the latest client rectangle of the document element. */
_cacheViewportGeometry?() {
this._documentRect = document.documentElement.getBoundingClientRect();
}

}

export function VIEWPORT_RULER_PROVIDER_FACTORY(parentDispatcher: ViewportRuler) {
return parentDispatcher || new ViewportRuler();
return parentDispatcher || new ViewportRuler(new ScrollDispatcher());
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done - Thanks for the help on Slack again.

};

export const VIEWPORT_RULER_PROVIDER = {
Expand Down
16 changes: 13 additions & 3 deletions src/lib/core/ripple/ripple.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {TestBed, ComponentFixture, fakeAsync, tick} from '@angular/core/testing';
import {TestBed, ComponentFixture, fakeAsync, tick, inject} from '@angular/core/testing';
import {Component, ViewChild} from '@angular/core';
import {MdRipple, MdRippleModule} from './ripple';
import {ViewportRuler} from '../overlay/position/viewport-ruler';


/** Creates a DOM event to indicate that a CSS transition for the given property ended. */
Expand Down Expand Up @@ -60,6 +61,7 @@ describe('MdRipple', () => {
let rippleElement: HTMLElement;
let rippleBackground: Element;
let originalBodyMargin: string;
let viewportRuler: ViewportRuler;

beforeEach(() => {
TestBed.configureTestingModule({
Expand All @@ -72,11 +74,13 @@ describe('MdRipple', () => {
});
});

beforeEach(() => {
beforeEach(inject([ViewportRuler], (ruler: ViewportRuler) => {
viewportRuler = ruler;

// Set body margin to 0 during tests so it doesn't mess up position calculations.
originalBodyMargin = document.body.style.margin;
document.body.style.margin = '0';
});
}));

afterEach(() => {
document.body.style.margin = originalBodyMargin;
Expand Down Expand Up @@ -228,6 +232,9 @@ describe('MdRipple', () => {
document.documentElement.scrollTop = pageScrollTop;
// Mobile safari
window.scrollTo(pageScrollLeft, pageScrollTop);
// Force an update of the cached viewport geometries because IE11 emits the
// scroll event later.
viewportRuler._cacheViewportGeometry();
});

afterEach(() => {
Expand All @@ -239,6 +246,9 @@ describe('MdRipple', () => {
document.documentElement.scrollTop = 0;
// Mobile safari
window.scrollTo(0, 0);
// Force an update of the cached viewport geometries because IE11 emits the
// scroll event later.
viewportRuler._cacheViewportGeometry();
});

it('create ripple with correct position', () => {
Expand Down