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

zhouyx · 2016-10-25T23:44:49Z

I force the background-color alpha to be 1 in this PR. Didn't add opacity: 1 !important though. opacity:0 will also apply opacity to amp-ad, I assume no one is using that.

cc @jasti

aghassemi · 2016-10-26T00:02:38Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.css

@@ -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;


as discussed remove this

aghassemi · 2016-10-26T00:08:49Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js


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


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 #

aghassemi · 2016-10-26T00:09:50Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js

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


aghassemi · 2016-10-26T00:10:48Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js

+    if (!isExperimentOn(this.win, TAG)) {
+      return;
+    }
+    const background = this.win.getComputedStyle(this.element)


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

aghassemi · 2016-10-26T00:21:21Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js

+    if (!isExperimentOn(this.win, TAG)) {
+      return;
+    }
+    const background = this.win.getComputedStyle(this.element)


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

aghassemi · 2016-10-26T00:22:10Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js

+    }
+    const background = this.win.getComputedStyle(this.element)
+        .getPropertyValue('background-color');
+    if (!startsWith(background, 'rgba')) {


there is also hsla

aghassemi · 2016-10-26T00:23:03Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js

+    if (!startsWith(background, 'rgba')) {
+      return;
+    }
+    user().warn('AMP-STICKY-AD', 'Do not allow container to be transparent');


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

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.

jridgewell · 2016-10-26T17:42:37Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js

+    }
+
+    // TODO(@zhouyx): Move the opacity style to CSS after remove experiments
+    // Not using setStyles because we will remove this line later.


Hm? It's never removed.

? I will remove once we change our ux and decide to remove experimental protection.

jridgewell · 2016-10-26T17:43:11Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js

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


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

jridgewell · 2016-10-26T17:43:47Z

src/style.js

+ * @param{string} cssColorValue
+ * @return {?string}
+ */
+export function removeAlphaFromBackgroundColor(cssColorValue) {


removeAlphaFromColor

jridgewell · 2016-10-26T17:43:56Z

src/style.js

+ * Return the new "background-color" property without alpha.
+ * Note caller needs to make sure the input cssColorValue is a valid
+ * CSS "background-color" value.
+ * @param{string} cssColorValue


jridgewell · 2016-10-26T18:18:06Z

src/style.js

+ * @param{string} cssColorValue
+ * @return {?string}
+ */
+export function removeAlphaFromBackgroundColor(cssColorValue) {


This can be simplified down to:

return cssColor.replace(/$([^,]+),([^,]+),([^,)]+),[^)]+$/g, '($1,$2,$3, 1)');

😨 OMG regular expression!!!

great suggestion! applied.

zhouyx · 2016-10-26T22:56:32Z

@jridgewell @aghassemi PTAL

aghassemi

LGTM, small comments

aghassemi · 2016-10-26T22:57:57Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js

+    }
+
+    // TODO(@zhouyx): Move the opacity style to CSS after remove experiments
+    // Not using setStyles because we will remove this line later.


but you are using setStyle :)

aghassemi · 2016-10-26T23:00:24Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js

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


nit: semitransparent would probably be more accurate than transparent

aghassemi · 2016-10-26T23:00:53Z

src/style.js

@@ -196,3 +195,16 @@ export function translate(x, opt_y) {
 export function scale(value) {
  return `scale(${value})`;
 }
+
+/**
+ * Remove alpha value from a "background-color" CSS property.


remove references to "background-color" in comments

aghassemi · 2016-10-26T23:07:05Z

test/functional/test-style.js

+    expect(st.removeAlphaFromColor('rgb(1, 1, 1)')).to.equal(
+        'rgb(1, 1, 1)');
+    expect(st.removeAlphaFromColor('hsl(1, 1, 1)')).to.equal(
+        'hsl(1, 1, 1)');


add tests for percent-based RGBA as well:
hsla(120, 100%, 25%, 0.3)

and maybe some negative cases like:

'#333'
'transparent'
''
'white'
'totallyInvalid'

aghassemi · 2016-10-26T23:07:15Z

src/style.js

+ */
+export function removeAlphaFromColor(cssColor) {
+  return cssColor.replace(
+      /\(([^,]+),([^,]+),([^,)]+),[^)]+\)/g, '($1,$2,$3, 1)');


zhouyx · 2016-10-27T00:06:53Z

extensions/amp-sticky-ad/0.1/amp-sticky-ad.js

+    // TODO(@zhouyx): Move the opacity style to CSS after remove experiments
+    // Note: Use setStyle because we will remove this line later.
+    setStyle(this.element, 'opacity', '1 !important');
+    setStyle(this.element, 'background-image', 'none');


add the background-image: none here as well

zhouyx · 2016-10-27T00:07:05Z

extensions/amp-sticky-ad/0.1/test/test-amp-sticky-ad.js

+  it('should not allow container to be set to transparent', () => {
+    toggleExperiment(window, 'amp-sticky-ad-better-ux', true);
+    return getAmpStickyAd({
+      'style': 'background-color: transparent !important',


zhouyx · 2016-10-27T00:07:20Z

src/style.js

@@ -196,3 +195,15 @@ export function translate(x, opt_y) {
 export function scale(value) {
  return `scale(${value})`;
 }
+
+/**
+ * Remove alpha value from a rgba color value.


rephrase the comment

zhouyx · 2016-10-27T00:07:49Z

test/functional/test-style.js

+        'rgba(1, 1, 1, 1)');
+    expect(st.removeAlphaFromColor('rgb(1, 1, 1)')).to.equal(
+        'rgb(1, 1, 1)');
+    expect(st.removeAlphaFromColor('rgba(0, 0, 0,-0.5)')).to.equal(


remove hsla test since we assume it's not a valid input.

* check background-color alpha * s * update JSdoc * address comment 1 * refactor func to style.js * fix test * apply regular expression * address comment * add new test

zhouyx added 3 commits October 25, 2016 16:37

check background-color alpha

43bd0cf

s

7d1073f

update JSdoc

caabd2a

zhouyx added the WG: monetization label Oct 25, 2016

zhouyx assigned aghassemi Oct 25, 2016

aghassemi suggested changes Oct 26, 2016

View reviewed changes

zhouyx added 2 commits October 25, 2016 17:59

address comment 1

3cf0c96

refactor func to style.js

07261fe

jridgewell reviewed Oct 26, 2016

View reviewed changes

fix test

638a859

jridgewell reviewed Oct 26, 2016

View reviewed changes

apply regular expression

e3dd99d

aghassemi approved these changes Oct 26, 2016

View reviewed changes

zhouyx added 2 commits October 26, 2016 16:27

address comment

fe85454

add new test

8b8aab5

zhouyx commented Oct 27, 2016

View reviewed changes

zhouyx merged commit d05a12e into ampproject:master Oct 27, 2016

zhouyx deleted the amp-sticky-improve2 branch October 27, 2016 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zhouyx commented Oct 25, 2016

aghassemi Oct 26, 2016

aghassemi Oct 26, 2016

aghassemi Oct 26, 2016

aghassemi Oct 26, 2016

aghassemi Oct 26, 2016

aghassemi Oct 26, 2016

aghassemi Oct 26, 2016

zhouyx Oct 26, 2016 •

edited

jridgewell Oct 26, 2016

zhouyx Oct 26, 2016

jridgewell Oct 26, 2016

zhouyx Oct 26, 2016

jridgewell Oct 26, 2016

jridgewell Oct 26, 2016

jridgewell Oct 26, 2016

zhouyx Oct 26, 2016

zhouyx Oct 26, 2016

zhouyx commented Oct 26, 2016

aghassemi left a comment

aghassemi Oct 26, 2016

aghassemi Oct 26, 2016

aghassemi Oct 26, 2016

aghassemi Oct 26, 2016

aghassemi Oct 26, 2016

zhouyx Oct 27, 2016

zhouyx Oct 27, 2016

zhouyx Oct 27, 2016

zhouyx Oct 27, 2016


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

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

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

Conversation

zhouyx commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx Oct 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx commented Oct 26, 2016

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx Oct 26, 2016 •

edited