From 4704da79eb18fa7b82cf5d0766e8e18f2b8a3abb Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Thu, 28 Jan 2016 14:01:04 -0800 Subject: [PATCH 1/4] Disable horizontal scrollin on iOS when embedded as an IFrame --- src/service/viewport-impl.js | 15 ++++++--------- test/functional/test-viewport.js | 13 +++++-------- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/src/service/viewport-impl.js b/src/service/viewport-impl.js index 0b6f1da23902..b6308c090d05 100644 --- a/src/service/viewport-impl.js +++ b/src/service/viewport-impl.js @@ -678,9 +678,6 @@ export class ViewportBindingNaturalIosEmbed_ { /** @const {!Window} */ this.win = win; - /** @private {number} */ - this.scrollWidth_ = 0; - /** @private {?Element} */ this.scrollPosEl_ = null; @@ -713,9 +710,6 @@ export class ViewportBindingNaturalIosEmbed_ { const documentElement = this.win.document.documentElement; const documentBody = this.win.document.body; - // TODO(dvoytenko): need to also find a way to do this on resize. - this.scrollWidth_ = documentBody./*OK*/scrollWidth || 0; - // Embedded scrolling on iOS is rather complicated. IFrames cannot be sized // and be scrollable. Sizing iframe by scrolling height has a big negative // that "fixed" position is essentially impossible. The only option we @@ -731,11 +725,13 @@ export class ViewportBindingNaturalIosEmbed_ { // -webkit-overflow-scrolling: touch; // } setStyles(documentElement, { - overflow: 'auto', + overflowY: 'auto', + overflowX: 'hidden', webkitOverflowScrolling: 'touch' }); setStyles(documentBody, { - overflow: 'auto', + overflowY: 'auto', + overflowX: 'hidden', webkitOverflowScrolling: 'touch', position: 'absolute', top: 0, @@ -837,7 +833,8 @@ export class ViewportBindingNaturalIosEmbed_ { /** @override */ getScrollWidth() { - return Math.max(this.scrollWidth_, this.win./*OK*/innerWidth); + // There's no good way to calculate scroll width on iOS in this mode. + return this.win./*OK*/innerWidth; } /** @override */ diff --git a/test/functional/test-viewport.js b/test/functional/test-viewport.js index cc9068120c82..5070f3aa5448 100644 --- a/test/functional/test-viewport.js +++ b/test/functional/test-viewport.js @@ -629,21 +629,18 @@ describe('ViewportBindingNaturalIosEmbed', () => { expect(binding.scrollWidth_).to.equal(777); }); - it('should defer scrollWidth to max of window.innerHeight ' + - ' and body.scrollWidth', () => { - binding.scrollWidth_ = 0; + it('should always have scrollWidth equal window.innerWidth', () => { expect(binding.getScrollWidth()).to.equal(555); - - binding.scrollWidth_ = 1000; - expect(binding.getScrollWidth()).to.equal(1000); }); it('should setup document for embed scrolling', () => { const documentElement = windowApi.document.documentElement; const body = windowApi.document.body; - expect(documentElement.style.overflow).to.equal('auto'); + expect(documentElement.style.overflowY).to.equal('auto'); + expect(documentElement.style.overflowX).to.equal('hidden'); expect(documentElement.style.webkitOverflowScrolling).to.equal('touch'); - expect(body.style.overflow).to.equal('auto'); + expect(body.style.overflowY).to.equal('auto'); + expect(body.style.overflowX).to.equal('hidden'); expect(body.style.webkitOverflowScrolling).to.equal('touch'); expect(body.style.position).to.equal('absolute'); expect(body.style.top).to.equal(0); From d70f757918d3cf33fcfc5dc3b35b4217480b09ab Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Thu, 28 Jan 2016 14:23:55 -0800 Subject: [PATCH 2/4] Other browsers too --- src/service/viewport-impl.js | 9 +++++++-- test/functional/test-viewport.js | 24 +++++++++++++++++++----- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/service/viewport-impl.js b/src/service/viewport-impl.js index b6308c090d05..521acf1c4293 100644 --- a/src/service/viewport-impl.js +++ b/src/service/viewport-impl.js @@ -540,8 +540,9 @@ export class ViewportBindingNatural_ { /** * @param {!Window} win + * @param {boolean} isEmbedded */ - constructor(win) { + constructor(win, isEmbedded) { /** @const {!Window} */ this.win = win; @@ -554,6 +555,10 @@ export class ViewportBindingNatural_ { this.win.addEventListener('scroll', () => this.scrollObservable_.fire()); this.win.addEventListener('resize', () => this.resizeObservable_.fire()); + if (isEmbedded) { + setStyles(this.win.document.documentElement, {overflowX: 'hidden'}); + } + log.fine(TAG_, 'initialized natural viewport'); } @@ -1152,7 +1157,7 @@ function createViewport_(window) { } else if (viewer.getViewportType() == 'natural-ios-embed') { binding = new ViewportBindingNaturalIosEmbed_(window); } else { - binding = new ViewportBindingNatural_(window); + binding = new ViewportBindingNatural_(window, viewer.isEmbedded()); } return new Viewport(window, binding, viewer); } diff --git a/test/functional/test-viewport.js b/test/functional/test-viewport.js index 5070f3aa5448..3c7f28513388 100644 --- a/test/functional/test-viewport.js +++ b/test/functional/test-viewport.js @@ -445,6 +445,7 @@ describe('ViewportBindingNatural', () => { let windowMock; let binding; let windowApi; + let documentElement; let windowEventHandlers; beforeEach(() => { @@ -455,8 +456,14 @@ describe('ViewportBindingNatural', () => { windowEventHandlers[eventType] = handler; }; windowApi = new WindowApi(); + documentElement = { + style: {} + }; + windowApi.document = { + documentElement: documentElement + }; windowMock = sandbox.mock(windowApi); - binding = new ViewportBindingNatural_(windowApi); + binding = new ViewportBindingNatural_(windowApi, false); }); afterEach(() => { @@ -467,6 +474,17 @@ describe('ViewportBindingNatural', () => { sandbox = null; }); + it('should NOT override overflow by default', () => { + expect(documentElement.style.overflowX).to.be.undefined; + expect(documentElement.style.overflowY).to.be.undefined; + }); + + it('should override overflow when embedded', () => { + new ViewportBindingNatural_(windowApi, true); + expect(documentElement.style.overflowX).to.equal('hidden'); + expect(documentElement.style.overflowY).to.be.undefined; + }); + it('should subscribe to scroll and resize events', () => { expect(windowEventHandlers['scroll']).to.not.equal(undefined); expect(windowEventHandlers['resize']).to.not.equal(undefined); @@ -625,10 +643,6 @@ describe('ViewportBindingNaturalIosEmbed', () => { expect(bodyEventListeners['scroll']).to.not.equal(undefined); }); - it('should pre-calculate scrollWidth', () => { - expect(binding.scrollWidth_).to.equal(777); - }); - it('should always have scrollWidth equal window.innerWidth', () => { expect(binding.getScrollWidth()).to.equal(555); }); From 039817aae837a415a1fe4d0438046165f8322ddf Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Thu, 28 Jan 2016 19:43:17 -0800 Subject: [PATCH 3/4] Beyond embedded cases as well --- css/amp.css | 9 ++++++++- src/service/viewport-impl.js | 11 ++--------- test/functional/test-viewport.js | 11 +---------- 3 files changed, 11 insertions(+), 20 deletions(-) diff --git a/css/amp.css b/css/amp.css index 9f17520e0b27..c7cf2f174b8e 100644 --- a/css/amp.css +++ b/css/amp.css @@ -14,6 +14,14 @@ * limitations under the License. */ +/** + * Horizontal scrolling interferes with embedded scenarios and predominantly + * the result of the non-responsive design. + */ +html, body { + overflow-x: hidden !important; +} + /** * Margin:0 is currently needed for iOS viewer embeds. * See: @@ -21,7 +29,6 @@ * and {@link ViewportBindingNaturalIosEmbed_} for more info. */ body { - /* Undoes mandatory opacity: 0 in doc. */ margin: 0 !important; } diff --git a/src/service/viewport-impl.js b/src/service/viewport-impl.js index 521acf1c4293..233af9af26a2 100644 --- a/src/service/viewport-impl.js +++ b/src/service/viewport-impl.js @@ -540,9 +540,8 @@ export class ViewportBindingNatural_ { /** * @param {!Window} win - * @param {boolean} isEmbedded */ - constructor(win, isEmbedded) { + constructor(win) { /** @const {!Window} */ this.win = win; @@ -555,10 +554,6 @@ export class ViewportBindingNatural_ { this.win.addEventListener('scroll', () => this.scrollObservable_.fire()); this.win.addEventListener('resize', () => this.resizeObservable_.fire()); - if (isEmbedded) { - setStyles(this.win.document.documentElement, {overflowX: 'hidden'}); - } - log.fine(TAG_, 'initialized natural viewport'); } @@ -731,12 +726,10 @@ export class ViewportBindingNaturalIosEmbed_ { // } setStyles(documentElement, { overflowY: 'auto', - overflowX: 'hidden', webkitOverflowScrolling: 'touch' }); setStyles(documentBody, { overflowY: 'auto', - overflowX: 'hidden', webkitOverflowScrolling: 'touch', position: 'absolute', top: 0, @@ -1157,7 +1150,7 @@ function createViewport_(window) { } else if (viewer.getViewportType() == 'natural-ios-embed') { binding = new ViewportBindingNaturalIosEmbed_(window); } else { - binding = new ViewportBindingNatural_(window, viewer.isEmbedded()); + binding = new ViewportBindingNatural_(window); } return new Viewport(window, binding, viewer); } diff --git a/test/functional/test-viewport.js b/test/functional/test-viewport.js index 3c7f28513388..7a2c591a657c 100644 --- a/test/functional/test-viewport.js +++ b/test/functional/test-viewport.js @@ -463,7 +463,7 @@ describe('ViewportBindingNatural', () => { documentElement: documentElement }; windowMock = sandbox.mock(windowApi); - binding = new ViewportBindingNatural_(windowApi, false); + binding = new ViewportBindingNatural_(windowApi); }); afterEach(() => { @@ -475,13 +475,6 @@ describe('ViewportBindingNatural', () => { }); it('should NOT override overflow by default', () => { - expect(documentElement.style.overflowX).to.be.undefined; - expect(documentElement.style.overflowY).to.be.undefined; - }); - - it('should override overflow when embedded', () => { - new ViewportBindingNatural_(windowApi, true); - expect(documentElement.style.overflowX).to.equal('hidden'); expect(documentElement.style.overflowY).to.be.undefined; }); @@ -651,10 +644,8 @@ describe('ViewportBindingNaturalIosEmbed', () => { const documentElement = windowApi.document.documentElement; const body = windowApi.document.body; expect(documentElement.style.overflowY).to.equal('auto'); - expect(documentElement.style.overflowX).to.equal('hidden'); expect(documentElement.style.webkitOverflowScrolling).to.equal('touch'); expect(body.style.overflowY).to.equal('auto'); - expect(body.style.overflowX).to.equal('hidden'); expect(body.style.webkitOverflowScrolling).to.equal('touch'); expect(body.style.position).to.equal('absolute'); expect(body.style.top).to.equal(0); From 4c9c307e5025020241bd7084d9a9cd35604a3d22 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Thu, 28 Jan 2016 19:44:51 -0800 Subject: [PATCH 4/4] wip --- test/functional/test-viewport.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/functional/test-viewport.js b/test/functional/test-viewport.js index 7a2c591a657c..f3745aebde7e 100644 --- a/test/functional/test-viewport.js +++ b/test/functional/test-viewport.js @@ -474,10 +474,6 @@ describe('ViewportBindingNatural', () => { sandbox = null; }); - it('should NOT override overflow by default', () => { - expect(documentElement.style.overflowY).to.be.undefined; - }); - it('should subscribe to scroll and resize events', () => { expect(windowEventHandlers['scroll']).to.not.equal(undefined); expect(windowEventHandlers['resize']).to.not.equal(undefined);