Skip to content

Commit

Permalink
Force transfer sidebar to the Fixed 'transfer' layer. (ampproject#6155)
Browse files Browse the repository at this point in the history
* Force transfer sidebar to the Fixed 'transfer' layer.

* review fix.

* Move to fixed layer before opening.

* Adding to fixed layer before dp block

* moving fl to build cb

* Make isFixed true for forceTransferElements

* Make isFixed true for forceTransferElements

* Review fixes
  • Loading branch information
camelburrito authored and Vanessa Pasque committed Dec 22, 2016
1 parent bd4010f commit 164c6fc
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 16 deletions.
4 changes: 2 additions & 2 deletions extensions/amp-sidebar/0.1/amp-sidebar.js
Expand Up @@ -82,6 +82,8 @@ export class AmpSidebar extends AMP.BaseElement {

this.viewport_ = this.getViewport();

this.viewport_.addToFixedLayer(this.element, /* forceTransfer */true);

if (this.side_ != 'left' && this.side_ != 'right') {
const pageDir =
this.document_.body.getAttribute('dir') ||
Expand Down Expand Up @@ -176,7 +178,6 @@ export class AmpSidebar extends AMP.BaseElement {
this.viewport_.disableTouchZoom();
this.vsync_.mutate(() => {
toggle(this.element, /* display */true);
this.viewport_.addToFixedLayer(this.element);
this.openMask_();
if (this.isIosSafari_) {
this.compensateIosBottombar_();
Expand Down Expand Up @@ -221,7 +222,6 @@ export class AmpSidebar extends AMP.BaseElement {
}
this.openOrCloseTimeOut_ = this.timer_.delay(() => {
if (!this.isOpen_()) {
this.viewport_.removeFromFixedLayer(this.element);
this.vsync_.mutate(() => {
toggle(this.element, /* display */false);
this.schedulePause(this.getRealChildren());
Expand Down
35 changes: 23 additions & 12 deletions src/service/fixed-layer.js
Expand Up @@ -171,9 +171,11 @@ export class FixedLayer {
/**
* Adds the element directly into the fixed layer, bypassing discovery.
* @param {!Element} element
* @param {boolean=} opt_forceTransfer If set to true , then the element needs
* to be forcefully transferred to the fixed layer.
*/
addElement(element) {
this.setupFixedElement_(element, /* selector */ '*');
addElement(element, opt_forceTransfer) {
this.setupFixedElement_(element, /* selector */ '*', opt_forceTransfer);
this.sortInDomOrder_();
this.update();
}
Expand Down Expand Up @@ -205,7 +207,7 @@ export class FixedLayer {
* Performs fixed actions.
* 1. Updates `top` styling if necessary.
* 2. On iOS/Iframe moves elements between fixed layer and BODY depending on
* whether they are currently visible and fixed.
* whether they are currently visible and fixed
* @return {!Promise}
*/
update() {
Expand Down Expand Up @@ -268,9 +270,13 @@ export class FixedLayer {
// Element is indeed fixed. Visibility is added to the test to
// avoid moving around invisible elements.
const isFixed = (
position == 'fixed' &&
element./*OK*/offsetWidth > 0 &&
element./*OK*/offsetHeight > 0);
position == 'fixed' && (
fe.forceTransfer || (
element./*OK*/offsetWidth > 0 &&
element./*OK*/offsetHeight > 0
)
)
);
if (!isFixed) {
state[fe.id] = {
fixed: false,
Expand Down Expand Up @@ -307,11 +313,11 @@ export class FixedLayer {
// `height` is constrained to at most 300px. This is to avoid
// transfering of more substantial sections for now. Likely to be
// relaxed in the future.
const isTransferrable = (
isFixed &&
opacity > 0 &&
element./*OK*/offsetHeight < 300 &&
(this.isAllowedCoord_(top) || this.isAllowedCoord_(bottom)));
const isTransferrable = isFixed && (
fe.forceTransfer || (
opacity > 0 &&
element./*OK*/offsetHeight < 300 &&
(this.isAllowedCoord_(top) || this.isAllowedCoord_(bottom))));
if (isTransferrable) {
hasTransferables = true;
}
Expand Down Expand Up @@ -399,9 +405,11 @@ export class FixedLayer {
*
* @param {!Element} element
* @param {string} selector
* @param {boolean=} opt_forceTransfer If set to true , then the element needs
* to be forcefully transferred to the fixed layer.
* @private
*/
setupFixedElement_(element, selector) {
setupFixedElement_(element, selector, opt_forceTransfer) {
let fe = null;
for (let i = 0; i < this.fixedElements_.length; i++) {
if (this.fixedElements_[i].element == element) {
Expand All @@ -424,6 +432,8 @@ export class FixedLayer {
};
this.fixedElements_.push(fe);
}

fe.forceTransfer = !!opt_forceTransfer;
}

/**
Expand Down Expand Up @@ -668,6 +678,7 @@ export class FixedLayer {
* fixedNow: (boolean|undefined),
* top: (string|undefined),
* transform: (string|undefined),
* forceTransfer: (boolean|undefined),
* }}
*/
let FixedElementDef;
Expand Down
6 changes: 4 additions & 2 deletions src/service/viewport-impl.js
Expand Up @@ -533,9 +533,11 @@ export class Viewport {
/**
* Adds the element to the fixed layer.
* @param {!Element} element
* @param {boolean=} opt_forceTransfer If set to true , then the element needs
* to be forcefully transferred to the fixed layer.
*/
addToFixedLayer(element) {
this.fixedLayer_.addElement(element);
addToFixedLayer(element, opt_forceTransfer) {
this.fixedLayer_.addElement(element, opt_forceTransfer);
}

/**
Expand Down
35 changes: 35 additions & 0 deletions test/functional/test-fixed-layer.js
Expand Up @@ -322,6 +322,20 @@ describe('FixedLayer', () => {
// Remove.
fixedLayer.removeElement(element3);
expect(fixedLayer.fixedElements_).to.have.length(2);

//Add with forceTransfer
fixedLayer.addElement(element3, '*', true);
expect(updateStub.callCount).to.equal(2);
expect(fixedLayer.fixedElements_).to.have.length(3);
const fe1 = fixedLayer.fixedElements_[2];
expect(fe1.id).to.equal('F3');
expect(fe1.element).to.equal(element3);
expect(fe1.selectors).to.deep.equal(['*']);
expect(fe1.forceTransfer).to.be.true;

// Remove.
fixedLayer.removeElement(element3);
expect(fixedLayer.fixedElements_).to.have.length(2);
});

it('should remove node when disappeared from DOM', () => {
Expand Down Expand Up @@ -680,6 +694,27 @@ describe('FixedLayer', () => {
expect(state['F0'].transferrable).to.equal(true);
});

it('should not disregard invisible element if it has forceTransfer', () => {
element1.computedStyle['position'] = 'fixed';
element1.offsetWidth = 0;
element1.offsetHeight = 0;

expect(vsyncTasks).to.have.length(1);
let state = {};
vsyncTasks[0].measure(state);
expect(state['F0'].fixed).to.be.false;
expect(state['F0'].transferrable).to.equal(false);

// Add.
state = {};
fixedLayer.setupFixedElement_(element1, '*', true);
expect(vsyncTasks).to.have.length(1);
vsyncTasks[0].measure(state);

expect(state['F0'].fixed).to.be.true;
expect(state['F0'].transferrable).to.equal(true);
});

it('should collect turn off transferrable with bottom != 0', () => {
element1.computedStyle['position'] = 'fixed';
element1.offsetWidth = 10;
Expand Down

0 comments on commit 164c6fc

Please sign in to comment.