Skip to content
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

amp-sticky-ad: Force background-color alpha to be 1 #5819

Merged
merged 9 commits into from Oct 27, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion extensions/amp-sticky-ad/0.1/amp-sticky-ad.css
Expand Up @@ -23,7 +23,7 @@ amp-sticky-ad {
z-index: 11;
max-height: 100px !important;
box-sizing: border-box;
background-color: rgba(0, 0, 0, 0.5);
background-color: #fff;
Copy link
Contributor

Choose a reason for hiding this comment

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

as discussed remove this

}

.-amp-sticky-ad-layout {
Expand Down
28 changes: 28 additions & 0 deletions extensions/amp-sticky-ad/0.1/amp-sticky-ad.js
Expand Up @@ -20,7 +20,12 @@ import {dev,user} from '../../../src/log';
import {removeElement} from '../../../src/dom';
import {toggle} from '../../../src/style';
import {listenOnce} from '../../../src/event-helper';
import {startsWith} from '../../../src/string';
import {setStyle} from '../../../src/style';
import {isExperimentOn} from '../../../src/experiments';

/** @private @const {string} */
const TAG = 'amp-sticky-ad-v1';
Copy link
Contributor

Choose a reason for hiding this comment

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

name this UX_EXPERIMENT, unlike other cases we are not sharing tag name and experiment name here.

Also maybe a more descriptive experiment name? amp-sticky-ad-better-ux ?

Also cleanup issue for the experiment with a comment pointing to issue #


class AmpStickyAd extends AMP.BaseElement {
/** @param {!AmpElement} element */
Expand Down Expand Up @@ -169,6 +174,7 @@ class AmpStickyAd extends AMP.BaseElement {
// Set sticky-ad to visible and change container style
this.element.setAttribute('visible', '');
this.element.classList.add('amp-sticky-ad-loaded');
this.forceOpacity_();
});
});
}
Expand Down Expand Up @@ -201,6 +207,28 @@ class AmpStickyAd extends AMP.BaseElement {
this.viewport_.updatePaddingBottom(0);
});
}

/**
* To check for background-color alpha and force it to be 1.
* Whoever call this needs to make sure it's in a vsync.
Copy link
Contributor

Choose a reason for hiding this comment

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

*calls

* @private
*/
forceOpacity_() {
if (!isExperimentOn(this.win, TAG)) {
return;
}
const background = this.win.getComputedStyle(this.element)
Copy link
Contributor

Choose a reason for hiding this comment

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

as discussed, force opacity here with a TODO to move it to CSS

Copy link
Contributor

Choose a reason for hiding this comment

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

move this logic to style.js with some unit tests. Maybe something like removeAlphaFromColor(cssColorValue)

.getPropertyValue('background-color');
if (!startsWith(background, 'rgba')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

there is also hsla

return;
}
user().warn('AMP-STICKY-AD', 'Do not allow container to be transparent');
Copy link
Contributor

Choose a reason for hiding this comment

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

just having grba or hsla does not mean it is transparent, we need to check if alpha value is < 1.

Copy link
Contributor Author

@zhouyx zhouyx Oct 26, 2016

Choose a reason for hiding this comment

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

I found that when rgba has an alpha value equal or greater than 1. getComputedStyle background-color will always return rgb instead. I tested this with Chrome and Safari, haven't tested with hsla, but I assume it would be the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

#error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, I thought about using warn or error. #error feels too strong to me

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: semitransparent would probably be more accurate than transparent

const backgroundColor = background.substring(
background.indexOf('(') + 1,
background.lastIndexOf(','));
console.log('set styles');
setStyle(this.element, 'background-color', 'rgb(' + backgroundColor + ')');
}
}

AMP.registerElement('amp-sticky-ad', AmpStickyAd, CSS);
25 changes: 24 additions & 1 deletion extensions/amp-sticky-ad/0.1/test/test-amp-sticky-ad.js
Expand Up @@ -15,6 +15,7 @@
*/

import {createIframePromise} from '../../../../testing/iframe';
import {toggleExperiment} from '../../../../src/experiments';
import * as sinon from 'sinon';
import '../amp-sticky-ad';
import '../../../amp-ad/0.1/amp-ad';
Expand All @@ -30,10 +31,13 @@ describe('amp-sticky-ad', () => {
sandbox.restore();
});

function getAmpStickyAd() {
function getAmpStickyAd(opt_attributes) {
return createIframePromise().then(iframe => {
const ampStickyAd = iframe.doc.createElement('amp-sticky-ad');
ampStickyAd.setAttribute('layout', 'nodisplay');
for (const attr in opt_attributes) {
ampStickyAd.setAttribute(attr, opt_attributes[attr]);
}
const ampAd = iframe.doc.createElement('amp-ad');
ampAd.setAttribute('width', '300');
ampAd.setAttribute('height', '50');
Expand Down Expand Up @@ -377,4 +381,23 @@ describe('amp-sticky-ad', () => {
.to.be.true;
});
});

it('should not allow container to be set transparent', () => {
toggleExperiment(window, 'amp-sticky-ad-v1', true);
return getAmpStickyAd({
'style': 'background-color: rgba(55, 55, 55, 0.55) !important',
}).then(obj => {
console.log(obj.ampStickyAd);
const stickyAdElement = obj.ampStickyAd;
const impl = stickyAdElement.implementation_;
impl.vsync_.mutate = function(callback) {
callback();
};
impl.scheduleLayoutForAd_();
impl.ad_.dispatchEvent(new Event('amp:built'));
impl.ad_.dispatchEvent(new Event('amp:load:end'));
expect(window.getComputedStyle(stickyAdElement)
.getPropertyValue('background-color')).to.equal('rgb(55, 55, 55)');
});
});
});
5 changes: 5 additions & 0 deletions tools/experiments/experiments.js
Expand Up @@ -218,6 +218,11 @@ const EXPERIMENTS = [
name: 'Split AMP\'s loading phase into chunks',
cleanupIssue: 'https://github.com/ampproject/amphtml/issues/5535',
},
{
id: 'amp-sticky-ad-v1',
name: 'New breaking UX changes make to amp-sticky-ad',
cleanupIssue: 'https://github.com/ampproject/amphtml/issues/5432',
},
];

if (getMode().localDev) {
Expand Down