Skip to content

Commit

Permalink
fix(ripple): Fix the ripple position (#1907)
Browse files Browse the repository at this point in the history
* Add test. Use viewport ruler

* fix lint

* Add ViewportRuler to providers

* .

* .

* .
  • Loading branch information
tinayuangao authored and mmalerba committed Dec 8, 2016
1 parent 4c49f5f commit dd508ea
Show file tree
Hide file tree
Showing 14 changed files with 121 additions and 15 deletions.
5 changes: 5 additions & 0 deletions src/lib/button/button.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import {async, TestBed, ComponentFixture} from '@angular/core/testing';
import {Component} from '@angular/core';
import {By} from '@angular/platform-browser';
import {MdButtonModule} from './button';
import {ViewportRuler} from '../core/overlay/position/viewport-ruler';
import {FakeViewportRuler} from '../core/overlay/position/fake-viewport-ruler';


describe('MdButton', () => {
Expand All @@ -10,6 +12,9 @@ describe('MdButton', () => {
TestBed.configureTestingModule({
imports: [MdButtonModule.forRoot()],
declarations: [TestApp],
providers: [
{provide: ViewportRuler, useClass: FakeViewportRuler},
]
});

TestBed.compileComponents();
Expand Down
3 changes: 2 additions & 1 deletion src/lib/button/button.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from '@angular/core';
import {CommonModule} from '@angular/common';
import {MdRippleModule, coerceBooleanProperty, DefaultStyleCompatibilityModeModule} from '../core';
import {ViewportRuler} from '../core/overlay/position/viewport-ruler';


// TODO(jelbourn): Make the `isMouseDown` stuff done with one global listener.
Expand Down Expand Up @@ -166,7 +167,7 @@ export class MdButtonModule {
static forRoot(): ModuleWithProviders {
return {
ngModule: MdButtonModule,
providers: []
providers: [ViewportRuler]
};
}
}
5 changes: 5 additions & 0 deletions src/lib/checkbox/checkbox.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import {
import {Component, DebugElement} from '@angular/core';
import {By} from '@angular/platform-browser';
import {MdCheckbox, MdCheckboxChange, MdCheckboxModule} from './checkbox';
import {ViewportRuler} from '../core/overlay/position/viewport-ruler';
import {FakeViewportRuler} from '../core/overlay/position/fake-viewport-ruler';


// TODO: Implement E2E tests for spacebar/click behavior for checking/unchecking
Expand All @@ -35,6 +37,9 @@ describe('MdCheckbox', () => {
CheckboxWithChangeEvent,
CheckboxWithFormControl,
],
providers: [
{provide: ViewportRuler, useClass: FakeViewportRuler},
]
});

TestBed.compileComponents();
Expand Down
3 changes: 2 additions & 1 deletion src/lib/checkbox/checkbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {CommonModule} from '@angular/common';
import {NG_VALUE_ACCESSOR, ControlValueAccessor} from '@angular/forms';
import {coerceBooleanProperty} from '../core/coersion/boolean-property';
import {MdRippleModule, DefaultStyleCompatibilityModeModule} from '../core';
import {ViewportRuler} from '../core/overlay/position/viewport-ruler';


/**
Expand Down Expand Up @@ -391,7 +392,7 @@ export class MdCheckboxModule {
static forRoot(): ModuleWithProviders {
return {
ngModule: MdCheckboxModule,
providers: []
providers: [ViewportRuler]
};
}
}
11 changes: 11 additions & 0 deletions src/lib/core/overlay/position/fake-viewport-ruler.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export class FakeViewportRuler {
getViewportRect() {
return {
left: 0, top: 0, width: 1014, height: 686, bottom: 686, right: 1014
};
}

getViewportScrollPosition() {
return {top: 0, left: 0};
}
}
8 changes: 2 additions & 6 deletions src/lib/core/overlay/position/viewport-ruler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,8 @@ export class ViewportRuler {
// `scrollTop` and `scrollLeft` is inconsistent. However, using the bounding rect of
// `document.documentElement` works consistently, where the `top` and `left` values will
// equal negative the scroll position.
const top = documentRect.top < 0 && document.body.scrollTop == 0 ?
-documentRect.top :
document.body.scrollTop;
const left = documentRect.left < 0 && document.body.scrollLeft == 0 ?
-documentRect.left :
document.body.scrollLeft;
const top = -documentRect.top || document.body.scrollTop || window.scrollY || 0;
const left = -documentRect.left || document.body.scrollLeft || window.scrollX || 0;

return {top, left};
}
Expand Down
63 changes: 63 additions & 0 deletions src/lib/core/ripple/ripple.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ describe('MdRipple', () => {
expect(pxStringToFloat(ripple.style.height)).toBeCloseTo(2 * expectedRadius, 1);
});


it('cleans up the event handlers when the container gets destroyed', () => {
fixture = TestBed.createComponent(RippleContainerWithNgIf);
fixture.detectChanges();
Expand All @@ -208,6 +209,68 @@ describe('MdRipple', () => {
rippleElement.dispatchEvent(createMouseEvent('mousedown'));
expect(rippleBackground.classList).not.toContain('md-ripple-active');
});

describe('when page is scrolled', () => {
var veryLargeElement: HTMLDivElement = document.createElement('div');
var pageScrollTop = 500;
var pageScrollLeft = 500;

beforeEach(() => {
// Add a very large element to make the page scroll
veryLargeElement.style.width = '4000px';
veryLargeElement.style.height = '4000px';
document.body.appendChild(veryLargeElement);
document.body.scrollTop = pageScrollTop;
document.body.scrollLeft = pageScrollLeft;
// Firefox
document.documentElement.scrollLeft = pageScrollLeft;
document.documentElement.scrollTop = pageScrollTop;
// Mobile safari
window.scrollTo(pageScrollLeft, pageScrollTop);
});

afterEach(() => {
document.body.removeChild(veryLargeElement);
document.body.scrollTop = 0;
document.body.scrollLeft = 0;
// Firefox
document.documentElement.scrollLeft = 0;
document.documentElement.scrollTop = 0;
// Mobile safari
window.scrollTo(0, 0);
});

it('create ripple with correct position', () => {
let elementTop = 600;
let elementLeft = 750;
let left = 50;
let top = 75;

rippleElement.style.position = 'absolute';
rippleElement.style.left = `${elementLeft}px`;
rippleElement.style.top = `${elementTop}px`;

// Simulate a keyboard-triggered click by setting event coordinates to 0.
const clickEvent = createMouseEvent('click', {
clientX: left + elementLeft - pageScrollLeft,
clientY: top + elementTop - pageScrollTop,
screenX: left + elementLeft,
screenY: top + elementTop
});
rippleElement.dispatchEvent(clickEvent);

const expectedRadius = Math.sqrt(250 * 250 + 125 * 125);
const expectedLeft = left - expectedRadius;
const expectedTop = top - expectedRadius;

const ripple = <HTMLElement>rippleElement.querySelector('.md-ripple-foreground');
expect(pxStringToFloat(ripple.style.left)).toBeCloseTo(expectedLeft, 1);
expect(pxStringToFloat(ripple.style.top)).toBeCloseTo(expectedTop, 1);
expect(pxStringToFloat(ripple.style.width)).toBeCloseTo(2 * expectedRadius, 1);
expect(pxStringToFloat(ripple.style.height)).toBeCloseTo(2 * expectedRadius, 1);
});
});

});

describe('configuring behavior', () => {
Expand Down
12 changes: 9 additions & 3 deletions src/lib/core/ripple/ripple.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
ForegroundRippleState,
} from './ripple-renderer';
import {DefaultStyleCompatibilityModeModule} from '../compatibility/default-mode';
import {ViewportRuler} from '../overlay/position/viewport-ruler';


@Directive({
Expand Down Expand Up @@ -62,14 +63,16 @@ export class MdRipple implements OnInit, OnDestroy, OnChanges {
@HostBinding('class.md-ripple-unbounded') @Input('md-ripple-unbounded') unbounded: boolean;

private _rippleRenderer: RippleRenderer;
_ruler: ViewportRuler;

constructor(_elementRef: ElementRef, _ngZone: NgZone) {
constructor(_elementRef: ElementRef, _ngZone: NgZone, _ruler: ViewportRuler) {
// These event handlers are attached to the element that triggers the ripple animations.
const eventHandlers = new Map<string, (e: Event) => void>();
eventHandlers.set('mousedown', (event: MouseEvent) => this._mouseDown(event));
eventHandlers.set('click', (event: MouseEvent) => this._click(event));
eventHandlers.set('mouseleave', (event: MouseEvent) => this._mouseLeave(event));
this._rippleRenderer = new RippleRenderer(_elementRef, eventHandlers, _ngZone);
this._ruler = _ruler;
}

/** TODO: internal */
Expand Down Expand Up @@ -163,7 +166,10 @@ export class MdRipple implements OnInit, OnDestroy, OnChanges {
// FIXME: This fails on IE11, which still sets pageX/Y and screenX/Y on keyboard clicks.
const isKeyEvent =
(event.screenX === 0 && event.screenY === 0 && event.pageX === 0 && event.pageY === 0);
this.end(event.pageX, event.pageY, isKeyEvent);

this.end(event.pageX - this._ruler.getViewportScrollPosition().left,
event.pageY - this._ruler.getViewportScrollPosition().top,
isKeyEvent);
}
}

Expand All @@ -188,7 +194,7 @@ export class MdRippleModule {
static forRoot(): ModuleWithProviders {
return {
ngModule: MdRippleModule,
providers: []
providers: [ViewportRuler]
};
}
}
5 changes: 5 additions & 0 deletions src/lib/radio/radio.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import {NgControl, FormsModule, ReactiveFormsModule, FormControl} from '@angular
import {Component, DebugElement} from '@angular/core';
import {By} from '@angular/platform-browser';
import {MdRadioGroup, MdRadioButton, MdRadioChange, MdRadioModule} from './radio';
import {ViewportRuler} from '../core/overlay/position/viewport-ruler';
import {FakeViewportRuler} from '../core/overlay/position/fake-viewport-ruler';


describe('MdRadio', () => {
Expand All @@ -16,6 +18,9 @@ describe('MdRadio', () => {
RadioGroupWithFormControl,
StandaloneRadioButtons,
],
providers: [
{provide: ViewportRuler, useClass: FakeViewportRuler},
]
});

TestBed.compileComponents();
Expand Down
3 changes: 2 additions & 1 deletion src/lib/radio/radio.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
DefaultStyleCompatibilityModeModule,
} from '../core';
import {coerceBooleanProperty} from '../core/coersion/boolean-property';
import {ViewportRuler} from '../core/overlay/position/viewport-ruler';


/**
Expand Down Expand Up @@ -478,7 +479,7 @@ export class MdRadioModule {
static forRoot(): ModuleWithProviders {
return {
ngModule: MdRadioModule,
providers: [MdUniqueSelectionDispatcher],
providers: [MdUniqueSelectionDispatcher, ViewportRuler],
};
}
}
5 changes: 5 additions & 0 deletions src/lib/tabs/tab-group.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import {Component, ViewChild} from '@angular/core';
import {By} from '@angular/platform-browser';
import {Observable} from 'rxjs/Observable';
import {MdTab} from './tab';
import {ViewportRuler} from '../core/overlay/position/viewport-ruler';
import {FakeViewportRuler} from '../core/overlay/position/fake-viewport-ruler';


describe('MdTabGroup', () => {
Expand All @@ -19,6 +21,9 @@ describe('MdTabGroup', () => {
AsyncTabsTestApp,
DisabledTabsTestApp,
TabGroupWithSimpleApi,
],
providers: [
{provide: ViewportRuler, useClass: FakeViewportRuler},
]
});

Expand Down
3 changes: 2 additions & 1 deletion src/lib/tabs/tab-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import 'rxjs/add/operator/map';
import {MdRippleModule} from '../core/ripple/ripple';
import {MdTab} from './tab';
import {MdTabBody} from './tab-body';
import {ViewportRuler} from '../core/overlay/position/viewport-ruler';


/** Used to generate unique ID's for each tab component */
Expand Down Expand Up @@ -296,7 +297,7 @@ export class MdTabsModule {
static forRoot(): ModuleWithProviders {
return {
ngModule: MdTabsModule,
providers: []
providers: [ViewportRuler]
};
}
}
5 changes: 5 additions & 0 deletions src/lib/tabs/tab-nav-bar/tab-nav-bar.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import {async, ComponentFixture, TestBed} from '@angular/core/testing';
import {MdTabsModule} from '../tab-group';
import {Component} from '@angular/core';
import {By} from '@angular/platform-browser';
import {ViewportRuler} from '../../core/overlay/position/viewport-ruler';
import {FakeViewportRuler} from '../../core/overlay/position/fake-viewport-ruler';


describe('MdTabNavBar', () => {
Expand All @@ -13,6 +15,9 @@ describe('MdTabNavBar', () => {
SimpleTabNavBarTestApp,
TabLinkWithNgIf,
],
providers: [
{provide: ViewportRuler, useClass: FakeViewportRuler},
]
});

TestBed.compileComponents();
Expand Down
5 changes: 3 additions & 2 deletions src/lib/tabs/tab-nav-bar/tab-nav-bar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
} from '@angular/core';
import {MdInkBar} from '../ink-bar';
import {MdRipple} from '../../core/ripple/ripple';
import {ViewportRuler} from '../../core/overlay/position/viewport-ruler';

/**
* Navigation component matching the styles of the tab group header.
Expand Down Expand Up @@ -60,8 +61,8 @@ export class MdTabLink {
selector: '[md-tab-link], [mat-tab-link]',
})
export class MdTabLinkRipple extends MdRipple implements OnDestroy {
constructor(private _element: ElementRef, private _ngZone: NgZone) {
super(_element, _ngZone);
constructor(private _element: ElementRef, private _ngZone: NgZone, _ruler: ViewportRuler) {
super(_element, _ngZone, _ruler);
}

// In certain cases the parent destroy handler
Expand Down

0 comments on commit dd508ea

Please sign in to comment.