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

Fixes an issue where ads are not correctly centered on certain platforms. #6003

Merged
merged 3 commits into from Nov 3, 2016
Merged

Fixes an issue where ads are not correctly centered on certain platforms. #6003

merged 3 commits into from Nov 3, 2016

Conversation

glevitzky
Copy link
Contributor

Fixes #5958.

…ansform' to a more commonly supported property.
left: '50%',
bottom: '',
right: '',
webkitTransform: 'translate(-50%, -50%)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just transform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand. The use of transform is the issue that needs correcting.

Copy link
Contributor

@jridgewell jridgewell Nov 3, 2016

Choose a reason for hiding this comment

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

Yes, but setStyles will correctly map transform to webkitTransform.

Copy link
Contributor Author

@glevitzky glevitzky Nov 3, 2016

Choose a reason for hiding this comment

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

Ah, neat. Now I understand what you meant by getVendorJsPropertyName.

@jridgewell
Copy link
Contributor

/to @lannka

@@ -53,6 +54,18 @@ export function doubleclick(global, data) {
};
}

// Center the ad in the container.
const container = global.document.querySelector('#c');
if (container) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When wouldn't there be a container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory never, but querySelector has return type null | Element, while setStyles expects Element.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a dev().assertElement then? Can we do that from ads code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to add dev().assertElement(container) in addition to checking if(container) { ...?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the call to setStyles:

setStyles(dev().assertElement(container), {...});

@lannka lannka added the LGTM label Nov 3, 2016
@lannka lannka merged commit 0a1da8f into ampproject:master Nov 3, 2016
@glevitzky glevitzky deleted the ad-shift-fix branch November 3, 2016 20:16
ruturajv-media-net pushed a commit to ruturajv-media-net/amphtml that referenced this pull request Nov 4, 2016
…tion rewrite (ampproject#5982)

* Update amp-ad-network-doubleclick-impl.md

* Reverting doc changes

The previous change was made in error (wrong branch).

* Initial changes, need to update tests

* Start fixing tests

* Fix unit test coverage

* revert a4a examples

Introduce amp-auto-id attribute to AMP element. (ampproject#5973)

* Introduce amp-auto-id attribute to AMP element.

* Fix tests

* Address comments

* Address comments

* revert

amp-live-list.md polling clarification (ampproject#5991)

Only report 1% of errors if a page has non-AMP JS (ampproject#5994)

Fixes ampproject#5732

Allow for hosted testing to override where third party frame is retrieved from (ampproject#5890)

* Allow for hosted testing to override where third party frame is retrieved from

* test my own site

* fix ava tests

Remove unsupported query selector feature (ampproject#5999)

Workaround for misbehaving webview viewer (ampproject#6001)

A webview viewer from Google is seing origins of the form `www.google.com`.

For the time being we accept these as they trigger errors in our error reporting that aren't useful. As this form of origins can only occur with webviews that have full control over the page via JS injection anyway, this change should make no difference for normal web usage.

A4A integration tests (ampproject#5812)

* First draft of full integration tests for A4A.

* First draft of full integration tests for A4A.

* Got tests of exception conditions working.  Refactored some common stuff to utils.js.

* Refactored XDom and friendly iframe tests to helpers.

* Added tests.

* Resolved merge conflicts.

* Centralized visibility check and updated visibility test.

* Moved all A4A tests to using central visibility test.

* Lint fixes.

* Changed custom expectations to local functions.

* Replaced stringToArrayBuffer with utf8Encode.

* Fixed visibility assertion.

* Lint fixes.

* Added a comment (partly in order to get Travis to re-run).

* Changes in response to reviews.

* Rebased and fixed small rebase error.

Fixes an issue where ads are not correctly centered on certain platforms. (ampproject#6003)

* Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property.

* Added check for existence and changed to plain old 'transform'.

* Now asserting the existence of the container.

amp-sticky-ad close button new style (ampproject#5979)

* unit style

* fix css

* fix test

* rebase

* fix z-index

Other JS errors: Use startsWith (ampproject#6006)

Fixes ampproject#5994 (comment).

Fix log calls without TAGNAME (ampproject#6005)

My presubmit was a bit too lenient, and too strict at the same time.

refactor amp-ad.css (ampproject#5992)

Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 (ampproject#5576)

* Combine amp-analytics var docs with var substitutions doc re: ampproject#1302

* Updated non-supported vars + format changes for amp-analytics vars

* Updates per Avi's feedback

* Updated descriptions for amp-carousel vars

add version parameter to AMP.extension signature (ampproject#5989)

* add version parameter to AMP.extension signature

* apply recommendations

* remove TODO

Improve error reporting (ampproject#6019)

- Include the current `location.hash`. We are missing it, because it is not included in the referrer.
- Include previously recorded errors, so it is easier to identify follow on errors.
- Kill the 2000 char limit as it isn't important for AMP's target browsers.

Include the originalHash (ampproject#6020)

We remove the hash in embedding mode, so the original code doesn't actually work.

Fix error: "Can't find variable: TextDecoder"  (ampproject#6011)

* Update amp-ad-network-doubleclick-impl.md

* Reverting doc changes

The previous change was made in error (wrong branch).

* Initial changes, need to update tests

* Start fixing tests

* Fix unit test coverage

* revert a4a examples

* Include self (window) when verifying TextDecoder/TextEncoder existence

* fix upstream conflict

* PR feedback

* PR feedback
ruturajv-media-net pushed a commit to ruturajv-media-net/amphtml that referenced this pull request Nov 4, 2016
Update ads.amp.html

Modify A4A AMP Creative to use ampRuntimeUtf16CharOffsets from validation rewrite (ampproject#5982)

* Update amp-ad-network-doubleclick-impl.md

* Reverting doc changes

The previous change was made in error (wrong branch).

* Initial changes, need to update tests

* Start fixing tests

* Fix unit test coverage

* revert a4a examples

Introduce amp-auto-id attribute to AMP element. (ampproject#5973)

* Introduce amp-auto-id attribute to AMP element.

* Fix tests

* Address comments

* Address comments

* revert

amp-live-list.md polling clarification (ampproject#5991)

Only report 1% of errors if a page has non-AMP JS (ampproject#5994)

Fixes ampproject#5732

Allow for hosted testing to override where third party frame is retrieved from (ampproject#5890)

* Allow for hosted testing to override where third party frame is retrieved from

* test my own site

* fix ava tests

Remove unsupported query selector feature (ampproject#5999)

Workaround for misbehaving webview viewer (ampproject#6001)

A webview viewer from Google is seing origins of the form `www.google.com`.

For the time being we accept these as they trigger errors in our error reporting that aren't useful. As this form of origins can only occur with webviews that have full control over the page via JS injection anyway, this change should make no difference for normal web usage.

A4A integration tests (ampproject#5812)

* First draft of full integration tests for A4A.

* First draft of full integration tests for A4A.

* Got tests of exception conditions working.  Refactored some common stuff to utils.js.

* Refactored XDom and friendly iframe tests to helpers.

* Added tests.

* Resolved merge conflicts.

* Centralized visibility check and updated visibility test.

* Moved all A4A tests to using central visibility test.

* Lint fixes.

* Changed custom expectations to local functions.

* Replaced stringToArrayBuffer with utf8Encode.

* Fixed visibility assertion.

* Lint fixes.

* Added a comment (partly in order to get Travis to re-run).

* Changes in response to reviews.

* Rebased and fixed small rebase error.

Fixes an issue where ads are not correctly centered on certain platforms. (ampproject#6003)

* Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property.

* Added check for existence and changed to plain old 'transform'.

* Now asserting the existence of the container.

amp-sticky-ad close button new style (ampproject#5979)

* unit style

* fix css

* fix test

* rebase

* fix z-index

Other JS errors: Use startsWith (ampproject#6006)

Fixes ampproject#5994 (comment).

Fix log calls without TAGNAME (ampproject#6005)

My presubmit was a bit too lenient, and too strict at the same time.

refactor amp-ad.css (ampproject#5992)

Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 (ampproject#5576)

* Combine amp-analytics var docs with var substitutions doc re: ampproject#1302

* Updated non-supported vars + format changes for amp-analytics vars

* Updates per Avi's feedback

* Updated descriptions for amp-carousel vars

add version parameter to AMP.extension signature (ampproject#5989)

* add version parameter to AMP.extension signature

* apply recommendations

* remove TODO

Improve error reporting (ampproject#6019)

- Include the current `location.hash`. We are missing it, because it is not included in the referrer.
- Include previously recorded errors, so it is easier to identify follow on errors.
- Kill the 2000 char limit as it isn't important for AMP's target browsers.

Include the originalHash (ampproject#6020)

We remove the hash in embedding mode, so the original code doesn't actually work.

Fix error: "Can't find variable: TextDecoder"  (ampproject#6011)

* Update amp-ad-network-doubleclick-impl.md

* Reverting doc changes

The previous change was made in error (wrong branch).

* Initial changes, need to update tests

* Start fixing tests

* Fix unit test coverage

* revert a4a examples

* Include self (window) when verifying TextDecoder/TextEncoder existence

* fix upstream conflict

* PR feedback

* PR feedback
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
…rms. (ampproject#6003)

* Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property.

* Added check for existence and changed to plain old 'transform'.

* Now asserting the existence of the container.
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
…rms. (ampproject#6003)

* Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property.

* Added check for existence and changed to plain old 'transform'.

* Now asserting the existence of the container.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants