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

Review window.visualViewport API #128

Closed
bokand opened this issue Aug 5, 2016 · 8 comments
Closed

Review window.visualViewport API #128

bokand opened this issue Aug 5, 2016 · 8 comments
Assignees

Comments

@bokand
Copy link

bokand commented Aug 5, 2016

It's cutting it a little close but we wanted to ship this in Chrome M54 which is branching in ~3-4 weeks. I only realized that the intent to ship process requires/suggests a TAG review but the API surface is quite small and simple.

Link: https://github.com/WICG/ViewportAPI
Blink Intent to Implement
Chromestatus entry

Thanks.

@torgo
Copy link
Member

torgo commented Aug 10, 2016

Taking a look at this on our call today.

@torgo
Copy link
Member

torgo commented Aug 10, 2016

Hi @bokand - we discussed / triaged on today's TAG call. Some of this discussion and potential issues raised by @plinss @hadleybeeman and @dbaron are in the minutes. We're going to try to write up a more complete review within the next week. In the mean time we can continue discussion in this thread if you have any immediate response to the questions raised in our minutes. Thanks!

@torgo torgo added this to the tag-telcon-2016-08-17 milestone Aug 10, 2016
@bokand
Copy link
Author

bokand commented Aug 11, 2016

Thanks! Re: privacy review, I can't think of how it might affect privacy
but I don't have enough experience to really know. FWIW, all these values
can be (roughly) calculated already, albeit with terrible hacks and some UA
checks. You can look at some of the madness in the polyfill
https://github.com/WICG/ViewportAPI/blob/master/polyfill/visualViewport.js
at how they can be calculated today (Still trying to make it work in iOS
Safari).

Another thing worth mentioning, this fits in the larger question of
coordinate spaces. Edge/IE moved to the model of having a separate viewport
for layout/visual and tried to make pinch-zoom mostly invisible to the
page. However, for compatibility, they left some properties responsive to
pinch-zoom. Chrome matched Edge's semantics. This leaves us in a position
where getBoundingClientRect and event client coordinates are relative to
the layout viewport while window.scrollX/Y and window.innerWidth/Height
report the values of the visual viewport. This leads to bugs as seen in the
duplicates of crbug.com/489206. In Safari, all these APIs are relative to
the visual viewport.

We tried to remedy the situation in crbug.com/571297, shipping the solution
for a full milestone in M48 which made scrollX/Y and innerWidth/Height
relative to the layout viewport and made pinch-zoom totally invisible to
script. We didn't see any reports of broken content but got significant
developer push back since they could no longer reason about the
pinch-zoomed state without an additional API. We undid that change and it
was part of our motivation for prioritizing this API.

IMO, we'd still like to move in the direction of making pinch-zoom (and
other features affecting the visual viewport, like the on screen keyboard
on some platforms) invisible to the existing API surface. This has the nice
property that these actions don't inadvertently cause the page to change.
e.g. User zooms in to see some item on the page but the script keeps an
overlay plastered to the screen obscuring the intended item. This is
especially useful since most development still takes place on desktop
devices using emulation where these issues don't come up as readily. With
this API, if a page really wants to do something on these actions it has to
be explicit about it. The alternative is to make all the APIs relative to
the visual viewport but this has the down side of being easier to build
surprising/broken UX.

Sorry for the wall-of-text, just wanted to give some background.

Thanks,
David

@dbaron
Copy link
Member

dbaron commented Aug 16, 2016

Is there a URL for viewing the spec without cloning the repo and viewing it locally (or just reading the raw HTML in github's UI)? (The repo is using a master branch rather than a gh-pages branch, so I can't use a wicg.github.io URL...)

@ymalik
Copy link

ymalik commented Aug 17, 2016

Here is a URL to the draft spec: https://rawgit.com/WICG/ViewportAPI/master/index.html

@dbaron
Copy link
Member

dbaron commented Aug 30, 2016

OK, I read through the spec and filed a bunch of issues (see above).

I think the one other thing I'm concerned about is whether the names of the properties on the VisualViewport interface make sense. They're certainly a hodgepodge of things from other places in the platform (Element and MouseEvent), although I'm not aware of scale occuring elsewhere as a property (but see Window.devicePixelRatio). It's not obvious to me that, put together, they really make sense as an API. Perhaps some diagrams would help. But on the flip side, it's not clear to me what the relative values of having (1) a sensible API, locally vs. (2) an API whose names share meaning with names elsewhere in the platform are. On the flip side, it's also not clear to me how solid the analogies are between these properties and the correspondingly-named ones on Element and MouseEvent.

I don't have any suggestions on the topic immediately, but it might be worth a little more discussion.

@bokand
Copy link
Author

bokand commented Aug 31, 2016

Thanks, the feedback is useful!

Regarding the scale attribute. The reason we leaned towards scale rather than zoom is to match the <meta name="viewport"> tag. That's really the only place where pinch-zoom values are defined so I think matching that is useful.

@dbaron
Copy link
Member

dbaron commented Oct 31, 2016

FWIW, the current link to the spec is now different.

We had a little more discussion in today's TAG meeting about WICG/visual-viewport#35 and provided a little further feedback there.

I think the TAG is ready to close this review issue, although if it's possible to make further improvements on the naming we'd be happy to see that, or even to provide further feedback if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants