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

Reconsider the names of properties on the VisualViewport interface #35

Closed
ymalik opened this issue Sep 8, 2016 · 10 comments
Closed

Reconsider the names of properties on the VisualViewport interface #35

ymalik opened this issue Sep 8, 2016 · 10 comments
Labels

Comments

@ymalik
Copy link
Collaborator

ymalik commented Sep 8, 2016

Creating an issue from dbaron's concern here: w3ctag/design-reviews#128

The names are a hodgepodge of things from other places in the platform (Element and MouseEvent).

Need to consider the relative values of having (1) a sensible API, locally vs. (2) an API whose names share meaning with names elsewhere in the platform.

@hexalys
Copy link

hexalys commented Sep 11, 2016

Just jumping late on the API spec here... I completely share @dbaron's concern on property name, starting with the API object name. @pp-koch's initial terminology works well for the API's documentation and the concept. But I find visualViewport unnecessary "java-like" long and fairly convoluted.

I suggest a more straight forward view. I don't think it's used anywhere; and would work well in contrast with screen, also leaving a simple viewport name for a future eventual (and probably desired) "layout viewport" dedicated object containing viewport zoom, orientation, accurate dpr information etc.

While this API relates to the viewport; In CSS terms, it does not technically represent the CSS viewport. So I think we should actually get away from using the same names (including terms such as scale).

I propose the following changes:

view = {
    double scrollYOffset;  // Relative to the layout viewport
    double scrollXOffset; // and read-only.                

    double clientX;  // Relative to the document
    double clientY;  // and read-only.

    double width;  // Read-only and excludes the scrollbars
    double height; // if present. CSS pixels visible in the visual viewport.

    double zoom; // Read-only. View zoom factor relative to the viewport width
}

I am replacing scrollTop/Left with scroll[Y/X]Offset matching @dbaron recommendation as per #29 Not sure on the name though.

The pageX property takes into account horizontal scrolling. So I am replacing pageX/Y with clientX/Y so that clientX matches touch events if you exclude scroll. The definition of touch.clientX being:
"The horizontal coordinate of point relative to the viewport in pixels, excluding any scroll offset"

And replacing clientWidth/Height with width/height given that it's more of an abstract similar to screen or window, and to get away from the clientWidth CSSOM definition.

That's my take so far. I'll leave that for others to review.

@bokand
Copy link
Collaborator

bokand commented Sep 13, 2016

Thanks for the suggestions. I'll reply to each property below:

I suggest a more straight forward view. I don't think it's used anywhere; and would work well in contrast with screen, also leaving a simple viewport name for a future eventual (and probably desired) "layout viewport" dedicated object containing viewport zoom, orientation, accurate dpr information etc.

I like it, window.view feels pretty good. The one problem I can see is that it might conflict with existing globals named view. That might be ok though as existing pages won't be using the API yet. I'd like to hear what others think.

While this API relates to the viewport; In CSS terms, it does not technically represent the CSS viewport. So I think we should actually get away from using the same names (including terms such as scale).

What do you mean by "CSS viewport"? This area is notoriously unspec'd so there isn't much that's agreed upon. In particular though, I prefer scale to zoom for two reasons. First, it's easier to distinguish between browser zoom (ctrl+/-) and what we're talking about here ("pinch-zoom"). More importantly, this is the same property as the viewport <meta>'s scale|minimum-scale|maximum-scale|initial-scale properties. Since that's basically a de facto standard I think it's important to match it.

double scrollYOffset; // Relative to the layout viewport
double scrollXOffset; // and read-only.

I'd actually go further and just call these offsetX and offsetY.

double clientX; // Relative to the document
double clientY; // and read-only.

I'm not sure I understand your rationale here. "client" means "viewport" relative in other areas. i.e. touch.clientX is the offset from the left of the viewport, not the document. These properties are suppose to include scroll, both the scrollX|YOffset and the layout viewport's scroll. I think pageX|Y is is clear and unambiguous.

double width; // Read-only and excludes the scrollbars
double height; // if present. CSS pixels visible in the visual viewport.

Agree, this is probably fine.

@hexalys
Copy link

hexalys commented Sep 14, 2016

What do you mean by "CSS viewport"? This area is notoriously unspec'd so there isn't much that's agreed upon. In particular though, I prefer scale to zoom for two reasons. First, it's easier to distinguish between browser zoom (ctrl+/-) and what we're talking about here ("pinch-zoom"). More importantly, this is the same property as the viewport 's scale|minimum-scale|maximum-scale|initial-scale properties. Since that's basically a de facto standard I think it's important to match it.
@bokand

You are right. I got confused on what that scale is though. If I understand correctly, to allow for variable initial scales like I do here. scale technically becomes the scaling factor applied to the visual viewport 'relative to the initial-scale', not the "ideal viewport (size at width=device-width)" as currently stated. In other words, if the author defines a custom viewport width and/or scale, it needs the visual viewport scale value to be relative the custom viewport width, since width=device-width is no longer in context. The wanted scale being currently viewport.width/window.innerwidth. (The scale relative to width=device-width is a prospective viewport.scale value for the layout viewport instead). Correct?

PS: The argument has been made here already.

I'd actually go further and just call these offsetX and offsetY.

Ok, in that case it's "relative to the origin of the padding edge of the document" as per Mouse events.

I'm not sure I understand your rationale here. "client" means "viewport" relative in other areas. i.e. touch.clientX is the offset from the left of the viewport, not the document. These properties are suppose to include scroll, both the scrollX|YOffset and the layout viewport's scroll. I think pageX|Y is is clear and unambiguous.

I think my reasoning was about not including a fixed inner scroll. But that's probably out of scope in this context. No change needed on pageX|Y.

@bokand
Copy link
Collaborator

bokand commented Sep 15, 2016

You are right. I got confused on what that scale is though. If I understand correctly, to allow for variable initial scales like I do here. scale technically becomes the scaling factor applied to the visual viewport 'relative to the initial-scale', not the "ideal viewport (size at width=device-width)" as currently stated. In other words, if the author defines a custom viewport width and/or scale, it needs the visual viewport scale value to be relative the custom viewport width, since width=device-width is no longer in context. The wanted scale being currently viewport.width/window.innerwidth. (The scale relative to width=device-width is a prospective viewport.scale value for the layout viewport instead). Correct?

I believe it's fairly easy to go from one interpretation of scale to another and I'd like to avoid adding many notions of scale as that'll make it rather confusing for authors. The definition as currently stated (scale relative to ideal) is really the scale of a CSS pixel relative to the device independent pixel (DIP) and it's useful since it's convenient for physical pixel calculations. That is, to go from CSS pixels to physical pixels we have physicalWidth = cssWidth / (visualViewport.scale * window.devicePixelRatio).

You can still easily get the scale relative to your custom viewport width with scaleRelativeToCustomWidth = customWidth / visualViewport.width but I think that's a less useful notion of scale. It also differs from what we currently refer to as scale in the viewport meta.

Ok, in that case it's "relative to the origin of the padding edge of the document" as per Mouse events.

These values are relative to the layout viewport rather than document. That's a good point about which edge though...I think we want to use the margin edge. If you think of the (unzoomed, desktop) viewport, adding a margin on or doesn't extend the margin past the scrollbars. This seems to be a special case for the viewport in that margin is treated almost like an "outer padding". I think it would be weird though if, when fully scrolled to the top left, the visualViewport would had a negative offset.

@jonathanKingston
Copy link
Member

Can this account for https://drafts.csswg.org/css-logical-props/ using the names blockSize and inlineStart for example? This would account for text direction.

@bokand
Copy link
Collaborator

bokand commented Oct 13, 2016

The visual viewport doesn't have any content though and, as such, is agnostic to text direction itself. Though its position relative to the layout viewport / page could be thought of in terms of block and inline directions, I'm not sure it provides any benefit vs choosing a physical origin since it has no content of its own.

@jonathanKingston
Copy link
Member

Is fair yeah, the only benefit would be persuading people to use those properties over height/width etc.

@dbaron
Copy link
Contributor

dbaron commented Oct 31, 2016

So we discussed w3ctag/design-reviews#128 a bit more in today's TAG meeting. I think most TAG members did agree that the naming situation isn't great.

One thing that came to mind was having an API that's based on DOMRect. This could, say, mean window.visualViewport having two different DOMRect properties, e.g.:
window.visualViewport.clientView (or .inLayoutViewport, or something)
window.visualViewport.pageView (or .inPage, or something).

One other thought (mainly from me, not discussed much in the meeting) is that this could bear a distant relation to APIs for coordinate space conversion, e.g., as discussed in a 2013 public-pointer-events thread.

We'd like to see this spec move forward and don't want to hold it up over naming, but we'd also like, if possible, to see names that will be easier for developers to remember and work with.

@bokand
Copy link
Collaborator

bokand commented Apr 24, 2017

I've updated the spec with the following naming:

window.view = {
  double offsetX;
  double offsetY;

  double pageX;
  double pageY;

  double width;
  double height;

  double scale;
}

What do folks think of this? @dbaron Would it be possible to get TAG to take a quick look at the names again? DOMRect sounds good but I think there's redundancy between the offset/size pairs which would get worse if we add sizes with included scrollbars (#19) or unscaled sizes (#14).

bokand added a commit that referenced this issue May 23, 2017
This updates the naming of the properties and viewport object itself:

`window.visualViewport` -> `window.view`
`scrollLeft` -> `offsetLeft`
`scrollTop` -> `offsetTop`
`pageX` -> `pageLeft`
`pageY` -> `pageTop`
`clientWidth` -> `width`
`clientHeight` -> `height`

For #35
@bokand
Copy link
Collaborator

bokand commented May 23, 2017

I've further updated the spec with some more recommendations from TAG so the API currently looks like this:

window.view = {
  double offsetLeft;
  double offsetTop;

  double pageLeft;
  double pageTop;

  double width;
  double height;

  double scale;
}

Absent any compelling counterarguments I'm inclined to leave this bikeshed as-is.

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

No branches or pull requests

5 participants