touch forwarding from ampdoc to viewer #7563
Conversation
| @@ -14,7 +14,8 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| import {Messaging, WindowPortEmulator} from './messaging.js'; | |||
| import {Messaging, WindowPortEmulator} from './messaging'; | |||
| import {TouchHandler} from './touch_handler'; | |||
There was a problem hiding this comment.
Please change file name to touch-handler.
| @@ -231,6 +272,7 @@ export class Messaging { | |||
| this.sendResponseError_(requestId, message.name, reason); | |||
| }); | |||
| } | |||
| return true; | |||
There was a problem hiding this comment.
Is new return type necessary? If yes, please add @return to jsdoc and document. Though, since we throw an error above, it looks like it's always true?
| @@ -160,6 +161,8 @@ export class AmpViewerIntegration { | |||
|
|
|||
| listenOnce( | |||
| this.win, 'unload', this.handleUnload_.bind(this, messaging)); | |||
|
|
|||
| new TouchHandler(this.win, messaging); | |||
There was a problem hiding this comment.
Let's add viewer.getParam('swipe') == '1' right away?
There was a problem hiding this comment.
Should this go in the cap parameter instead of being it's own parameter? I don't care, but if you want to switch to cap I'm happy to start using it for anything new.
| constructor(win, messaging) { | ||
| /** @const {!Window} */ | ||
| this.win = win; | ||
| /** @private {!./messaging.Messaging} */ |
| listenForTouchEvents() { | ||
| const handleEvent = this.handleEvent_.bind(this); | ||
|
|
||
| listen(this.win, 'touchstart', handleEvent); |
There was a problem hiding this comment.
Let's listen on this.win.document. Window is not cross-platform.
| handleEvent_(e) { | ||
| switch (e.type) { | ||
| case 'touchstart': | ||
| this.tracking_ = true; |
There was a problem hiding this comment.
Is there any scroll direction protection to avoid forwarding 100% of events?
There was a problem hiding this comment.
The Google script does do some detection, which is what it uses tracking_ for. This was done primarily to support horizontal scrolling within an iframe. This is no longer possible so I think this should work without the detection. It would just be an optimization to add it.
There was a problem hiding this comment.
after chatting offline w/ @ericfs, I'm removing the tracking_ as it's not really useful.
| const msg = this.copyTouchEvent_(e); | ||
| this.messaging_.sendRequest(e.type, msg, false); | ||
| } | ||
| if (this.scrollLocked_) { |
There was a problem hiding this comment.
Let's add TODO(...): switch to passive events and pan-touch action
| return this.requestFullOverlay_(); | ||
| case 'cancelFullOverlay': | ||
| return this.cancelFullOverlay_(); | ||
| case 'pushHistory': |
There was a problem hiding this comment.
FYI. replaceHistory is also a WIP @muxin. Though not really relevant to this cl.
| @@ -160,6 +161,8 @@ export class AmpViewerIntegration { | |||
|
|
|||
| listenOnce( | |||
| this.win, 'unload', this.handleUnload_.bind(this, messaging)); | |||
|
|
|||
| new TouchHandler(this.win, messaging); | |||
There was a problem hiding this comment.
Should this go in the cap parameter instead of being it's own parameter? I don't care, but if you want to switch to cap I'm happy to start using it for anything new.
| handleEvent_(e) { | ||
| switch (e.type) { | ||
| case 'touchstart': | ||
| this.tracking_ = true; |
There was a problem hiding this comment.
The Google script does do some detection, which is what it uses tracking_ for. This was done primarily to support horizontal scrolling within an iframe. This is no longer possible so I think this should work without the detection. It would just be an optimization to add it.
|
/cc @brandondiamond |
chenshay
changed the title
| case 'touchmove': | ||
| this.forwardEvent_(e); | ||
| break; | ||
| default: |
* touch forwarding * touch_handler * touch events * another * rename * bla * copy partial event * process touch events * fix * bla * type * remove mouse events * resolving merge conflicts * register handlers + scroll locking * remove console.log * fixes * file rename * unit tests * unit tests * lint
* touch forwarding * touch_handler * touch events * another * rename * bla * copy partial event * process touch events * fix * bla * type * remove mouse events * resolving merge conflicts * register handlers + scroll locking * remove console.log * fixes * file rename * unit tests * unit tests * lint
No description provided.