Closed Bug 1357785 Opened 7 years ago Closed 6 years ago

Expose the Visual Viewport API to web content

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: kats, Assigned: tanushree, Mentored)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, feature, Whiteboard: [gfx-noted])

Attachments

(3 files, 2 obsolete files)

Bug 1357785 - Expose the Visual Viewport API to web content.
59 bytes, text/x-review-board-request
nika
: review+
botond
: review+
Details
Visual Viewport Behavior on Chome
118.19 KB, image/png
Details
Visual Viewport API patch
11.53 KB, patch
Details | Diff | Splinter Review 
Chrome is going to be shipping the Visual Viewport API [1] soon. I looked over the spec again today and I think it makes sense and we should support it as well.

We have bug 1123938 on file for actually changing the way our viewports work, but the API itself is actually orthogonal to that and can be implemented on top of what we have now. It basically exposes a bunch of read-only properties that describe the visual viewport relative to the layout viewport, without assuming any particular UA behaviour.

[1] https://github.com/wicg/viewportapi
Keywords: feature
Priority: -- → P3
Whiteboard: [gfx-noted]
Assignee: nobody → tpodder
Mentor: botond
Depends on: 1470267
Depends on: 1477610
Blocks: 1477829
Comment on attachment 8995042 [details]
Bug 1357785 - Expose the Visual Viewport API to web content.

https://reviewboard.mozilla.org/r/259522/#review266766

Thanks for the patch!

Please go ahead and flag a DOM peer (such as Nika) for additional review after addressing these comments.

::: commit-message-7764e:3
(Diff revision 1)
> +Bug 1357785 - Part 1: Expose the Visual Viewport API to web content. r?botond
> +
> +Implement the Visual Viewport API according to the spec:

Please mention in the commit message that this patch does not implement the 'onresize' and 'oncroll' attributes. We should probably also file a bug to track implementing those.

::: dom/base/VisualViewport.h:24
(Diff revision 1)
> +/* Visual Viewport API spec:  https://wicg.github.io/visual-viewport/#the-visualviewport-interface */
> +class VisualViewport final: public mozilla::DOMEventTargetHelper
> +{
> +
> +  public:
> +    explicit VisualViewport(nsPIDOMWindowInner* aWindow, RefPtr<nsIPresShell> aPresShell);

The formatting convention for classes in our codebase is not to indent access specifiers (like "public:"), and to indent member declarations by one level (2 spaces).

(Here, the access specifiers are indented by one level, and the member declarations by two levels.)

::: dom/base/VisualViewport.h:26
(Diff revision 1)
> +{
> +
> +  public:
> +    explicit VisualViewport(nsPIDOMWindowInner* aWindow, RefPtr<nsIPresShell> aPresShell);
> +
> +    double OffsetLeft();

Please mark all getters in this class (public and private) as const.

::: dom/base/VisualViewport.h:39
(Diff revision 1)
> +    virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +
> +  protected:
> +    virtual ~VisualViewport();
> +
> +    CSSIntSize VisualViewportSize();

Since we don't expect to derive from these class, these methods can be private rather than protected.

::: dom/base/VisualViewport.cpp:35
(Diff revision 1)
> +
> +CSSIntSize VisualViewport::VisualViewportSize()
> +{
> +  CSSIntSize size = CSSIntSize(0,0);
> +
> +  if (mPresShell->IsScrollPositionClampingScrollPortSizeSet()) {

An SPC-SPS will only be set for the RCD's pres shell, and only on mobile.

In other situations, the visual viewport and the layout viewport are identical, and we need to return the layout viewport size here. It can be accessed via nsIScrollableFrame::GetScrollPortRect() (and then get its Size()).

::: dom/base/nsGlobalWindowInner.cpp:2245
(Diff revision 1)
> +    nsIDocShell* docShell = this->GetDocShell();
> +    RefPtr<nsIPresShell> presShell = docShell ? docShell->GetPresShell() : nullptr;
> +    if (presShell) {
> +      mVisualViewport = new mozilla::dom::VisualViewport(this, presShell);
> +    } else {
> +      aError.Throw(NS_ERROR_UNEXPECTED);

I don't think there's a need to throw if you're signaling failure by returning nullptr.

::: dom/webidl/Window.webidl:566
(Diff revision 1)
>  };
>  
>  Window implements WebGPUProvider;
> +
> +partial interface Window {
> +  [SameObject, Pref="dom.visualviewport.enabled", Throws, Replaceable] readonly attribute VisualViewport visualViewport;

The Pref="dom.visualviewport.enabled" part should be in the same patch that introduces that pref.

In this case, I don't see much value in separating that patch; I would just put everything into one patch.
Attachment #8995042 - Flags: review?(botond)
Comment on attachment 8995043 [details]
Bug 1357785 - Part 2: Enable the Visual Viewport API behind a pref.

https://reviewboard.mozilla.org/r/259524/#review266768

As mentioned, I would just fold this patch into the other one.

::: gfx/thebes/gfxPrefs.h:374
(Diff revision 1)
>    DECL_GFX_PREF(Live, "browser.ui.zoom.force-user-scalable",   ForceUserScalable, bool, false);
>    DECL_GFX_PREF(Live, "browser.viewport.desktopWidth",         DesktopViewportWidth, int32_t, 980);
>  
>    DECL_GFX_PREF(Live, "dom.ipc.plugins.asyncdrawing.enabled",  PluginAsyncDrawingEnabled, bool, false);
>    DECL_GFX_PREF(Live, "dom.meta-viewport.enabled",             MetaViewportEnabled, bool, false);
> +  DECL_GFX_PREF(Live, "dom.visualviewport.enabled",            VisualViewportEnabled, bool, false);

I would add a hyphen for readability:

  dom.visual-viewport.enabled
  
but I'll let Nika make the call on that.

::: mobile/android/app/mobile.js:767
(Diff revision 1)
>  
>  // Enable meta-viewport support for font inflation code
>  pref("dom.meta-viewport.enabled", true);
>  
> +// Enable Visual Viewport API
> +pref("dom.visualviewport.enabled", false);

There is no need to add a pref to mobile.js unless you want its value to be different on Android than on other platforms. The value from all.js will apply to all platforms.
Attachment #8995043 - Flags: review?(botond)
Blocks: 1478776
Attachment #8995043 - Attachment is obsolete: true
Comment on attachment 8995042 [details]
Bug 1357785 - Expose the Visual Viewport API to web content.

https://reviewboard.mozilla.org/r/259522/#review266766

> I don't think there's a need to throw if you're signaling failure by returning nullptr.

I had a discussion with Nika yesterday about error handling and retrieving valid browsing contexts. We decided that we should retrieve the presshell for each of the attributes to avoid edge cases when the presShell is discarded or inactive. If we don't have a valid presShell, we will return 0 for the attributes as that's what Chrome does right now. I have updated my review according to the discussion. I'll fix this issue once I have your approval for the current approach.
Comment on attachment 8995042 [details]
Bug 1357785 - Expose the Visual Viewport API to web content.

https://reviewboard.mozilla.org/r/259520/#review266990

::: mobile/android/app/mobile.js:767
(Diff revision 2)
>  
>  // Enable meta-viewport support for font inflation code
>  pref("dom.meta-viewport.enabled", true);
>  
> +// Enable Visual Viewport API
> +pref("dom.visualviewport.enabled", false);

Nika, which preference name would you suggest
"dom.visualviewport.enabled"
or
"dom.visual-viewport.enabled"
Comment on attachment 8995042 [details]
Bug 1357785 - Expose the Visual Viewport API to web content.

https://reviewboard.mozilla.org/r/259522/#review266994

::: modules/libpref/init/all.js:5570
(Diff revision 2)
>  
>  // Enable meta-viewport support in remote APZ-enabled frames.
>  pref("dom.meta-viewport.enabled", false);
>  
> +// Enable Visual Viewport API
> +pref("dom.visualviewport.enabled", false);

Nika, which of the two names would you suggest for the preference  
"dom.visualviewport.enabled"
or
"dom.visual-viewport.enabled"
Comment on attachment 8995042 [details]
Bug 1357785 - Expose the Visual Viewport API to web content.

https://reviewboard.mozilla.org/r/259522/#review267030

Ok, returning 0 as the fallback value for positions / sizes is fine if that's what Chrome does.

::: dom/base/VisualViewport.cpp:63
(Diff revisions 1 - 2)
>    return size.height;
>  }
>  
> -double VisualViewport::Scale()
> +double VisualViewport::Scale() const
>  {
> -  return mPresShell->GetResolution();
> +  double scale = 0;

We probably want 1 rather than 0 as a fallback value for scale.
Attachment #8995042 - Flags: review?(botond)
> Nika, which of the two names would you suggest for the preference  
> "dom.visualviewport.enabled"
> or
> "dom.visual-viewport.enabled"

*shrug* - let's go with "dom.visualviewport.enabled".
Comment on attachment 8995042 [details]
Bug 1357785 - Expose the Visual Viewport API to web content.

https://reviewboard.mozilla.org/r/259522/#review267322

r=me on the DOM changes with the style fixes requested :-)

::: dom/base/VisualViewport.cpp:30
(Diff revision 2)
> +VisualViewport::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
> +{
> +  return VisualViewport_Binding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +CSSSize VisualViewport::VisualViewportSize() const

nit: return type on a separate line from class name, here and below.

::: dom/base/VisualViewport.cpp:49
(Diff revision 2)
> +    }
> +  }
> +  return size;
> +}
> +
> +double VisualViewport::Width() const

nit: newline after double

::: dom/base/VisualViewport.cpp:120
(Diff revision 2)
> +
> +nsIPresShell* VisualViewport::GetPresShell() const
> +{
> +  nsCOMPtr<nsPIDOMWindowInner> window = GetOwner();
> +  if (!window)
> +    return nullptr;

nit: Please put { curlys } around if blocks, even if they're only 1-liners.

::: dom/webidl/Window.webidl:566
(Diff revision 2)
>  };
>  
>  Window implements WebGPUProvider;
> +
> +partial interface Window {
> +  [SameObject, Pref="dom.visualviewport.enabled", Throws, Replaceable] readonly attribute VisualViewport visualViewport;

nit: newline after ']'

::: mobile/android/app/mobile.js:766
(Diff revision 2)
>  pref("device.storage.enabled", true);
>  
>  // Enable meta-viewport support for font inflation code
>  pref("dom.meta-viewport.enabled", true);
>  
> +// Enable Visual Viewport API

I think this should probably say "Disable Visual Viewport API"

::: modules/libpref/init/all.js:5570
(Diff revision 2)
>  
>  // Enable meta-viewport support in remote APZ-enabled frames.
>  pref("dom.meta-viewport.enabled", false);
>  
> +// Enable Visual Viewport API
> +pref("dom.visualviewport.enabled", false);

As I mention in the bug, what you have right now is good :-)
Attachment #8995042 - Flags: review?(nika) → review+
Comment on attachment 8995042 [details]
Bug 1357785 - Expose the Visual Viewport API to web content.

https://reviewboard.mozilla.org/r/259522/#review268484

::: dom/base/VisualViewport.cpp:67
(Diff revisions 2 - 4)
>  }
>  
> -double VisualViewport::Scale() const
> +double
> +VisualViewport::Scale() const
>  {
>    double scale = 0;

I'm still uneasy about using 0 as the fallback value for scale.

The spec for scale says:

1. If there is no output device, return 1 and abort these steps.

You mentioned that Chrome's behaviour was to use 0 as the fallback value. Could you describe the steps you used to test this? If Chrome's behaviour deviates from the spec, we should make sure there is a Chrome bug on file about it.

::: mobile/android/app/mobile.js:767
(Diff revisions 2 - 4)
>  
>  // Enable meta-viewport support for font inflation code
>  pref("dom.meta-viewport.enabled", true);
>  
> -// Enable Visual Viewport API
> +// Disable Visual Viewport API
>  pref("dom.visualviewport.enabled", false);

I mentioned in comment 4 that adding the pref to mobile.js is not necessary unless you want it to have a different value than in all.js.
Attachment #8995042 - Flags: review?(botond)
Comment on attachment 8995042 [details]
Bug 1357785 - Expose the Visual Viewport API to web content.

https://reviewboard.mozilla.org/r/259522/#review268484

> I mentioned in comment 4 that adding the pref to mobile.js is not necessary unless you want it to have a different value than in all.js.

Oops, sorry, I missed that comment.
Comment on attachment 8995042 [details]
Bug 1357785 - Expose the Visual Viewport API to web content.

https://reviewboard.mozilla.org/r/259522/#review268648

Thanks for the update.

Clearing review flag until the issue of the fallback value for scale is addressed.
Attachment #8995042 - Flags: review?(botond)
Implemented the non-event handler attributes of the Visual Viewport API
according to the spec: https://wicg.github.io/visual-viewport

The 'onresize' and 'onscroll' attributes will be implemented in the bug
1478776.
MozReview-Commit-ID: G4bkIZ9VtZ2
Attachment #8999444 - Attachment is obsolete: true
Nika and I tested the behavior of the Visual Viewport API in Chrome for the situation in which the window was unloaded but visual viewport object was still present. In such situations, the visual viewport's scale is 0. The screenshot attached demonstrates the steps we followed and the final result. 

The spec for the Visual Viewport API says that the scale should be 1 when "there is no output device". Nika also agrees that the scale should be 1 in such situations as it could affect calculations which depend on the scale. 

However, I am still not sure if the situation we tested for is the same as the situation described in the spec.
Blocks: 1484523
Comment on attachment 8995042 [details]
Bug 1357785 - Expose the Visual Viewport API to web content.

https://reviewboard.mozilla.org/r/259522/#review269608

Looks good, thanks! Just one nit:

::: commit-message-7764e:11
(Diff revisions 5 - 8)
>  
>  The 'onresize' and 'onscroll' attributes will be implemented in the bug
>  1478776.
>  MozReview-Commit-ID: G4bkIZ9VtZ2
> +
> +Tags: #secure-revision

Please remove the Phabricator metadata from the commit message.
Attachment #8995042 - Flags: review?(botond) → review+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a236c987f26c
Expose the Visual Viewport API to web content. r=botond,Nika
Addressed the build failures and updated the patch. Should be good for landing.
Flags: needinfo?(tanushree.podder.103)
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9044d545791f
Expose the Visual Viewport API to web content. r=botond, r=nika
My bad, I should have asked for the patch to be rebased and then pushed to Try, before landing. (In this case, the patch needed to be rebased across bug 1471708.)

Thank you Markus for landing the updated patch!
https://hg.mozilla.org/mozilla-central/rev/9044d545791f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
This API was already documented — see https://developer.mozilla.org/en-US/docs/Web/API/VisualViewport. I have read through these and given it a copy edit.

I also submitted a PR to our BCD repo to update the compat data:
https://github.com/mdn/browser-compat-data/pull/2759

Last of all, because this is currently implemented behind a flag, I didn't add it to the Fx63 rel notes; I've added it to our experimental feature page instead:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features#DOM

Let me know if you think anything else is needed. Thanks!
Flags: needinfo?(tanushree.podder.103)
Keywords: dev-doc-needed → dev-doc-complete
Thanks for looking at the documentation, Chris!

I have a suggestion for a brief addition to the introductory description of the API. Something like:

"Note that only the top-level window has a visual viewport that's distinct from the layout viewport. Therefore, it's generally only the VisualViewport object of the top-level window that's interesting. (For an iframe, visual viewport metrics like visualViewport.width always correspond to layout viewport metrics like document.documentElement.clientWidth.)"

It also occurs to me that if a reader is not already familiar with the terms "visual viewport" and "layout viewport" (which are used throughout the documentation), they may be confused. There is a document that explains these terms fairly well:

https://github.com/bokand/bokand.github.io/blob/master/web_viewports_explainer.md

Would it be all right to link to it? (Not sure what the policy is about linking to documentation outside of MDN.) Eventually, we may want to refine that explainer document into a MDN page.
Flags: needinfo?(tanushree.podder.103)
Another thought: I don't really want to encourage web developers to use the Visual Viewport API to emulate position:device-fixed, for a couple of reasons:

  1) The whole point of bug 656036 is to avoid keeping elements
     fixed to the visual viewport, so they don't take up screen
     real estate as you zoom.

     If web developers start using the Visual Viewport API to
     keep e.g. ads fixed to the visual viewport, we're kind of
     back to square one on that.

  2) Using the Visual Viewport API to emulate position:device-fixed
     will result in the "fixed" elements jittering as you async- 
     scroll, which would be a poor UX.

     If we decide that keeping elements fixed to the visual
     viewport is an important use case, we should just implement
     position:device-fixed.

To that end, it would be nice if that example on the MDN page wasn't about emulating position:device-fixed. I will try to think of a better example. (This also serves as a good reminder for me that we need to go through the examples in the spec's README document (https://github.com/WICG/visual-viewport) and verify that they work as expected in Firefox.)
(In reply to Botond Ballo [:botond] from comment #32)
> Thanks for looking at the documentation, Chris!
> 
> I have a suggestion for a brief addition to the introductory description of
> the API. Something like:
> 
> "Note that only the top-level window has a visual viewport that's distinct
> from the layout viewport. Therefore, it's generally only the VisualViewport
> object of the top-level window that's interesting. (For an iframe, visual
> viewport metrics like visualViewport.width always correspond to layout
> viewport metrics like document.documentElement.clientWidth.)"

Done — added as a note to the top of the article.

> 
> It also occurs to me that if a reader is not already familiar with the terms
> "visual viewport" and "layout viewport" (which are used throughout the
> documentation), they may be confused. There is a document that explains
> these terms fairly well:
> 
> https://github.com/bokand/bokand.github.io/blob/master/
> web_viewports_explainer.md
> 
> Would it be all right to link to it? (Not sure what the policy is about
> linking to documentation outside of MDN.) Eventually, we may want to refine
> that explainer document into a MDN page.

Linked! I think eventually we should add glossary entries for these concepts, and perhaps some kind of more focused concept/explainer doc. I'll file a documentation bug for that.
(In reply to Botond Ballo [:botond] from comment #33)
> Another thought: I don't really want to encourage web developers to use the
> Visual Viewport API to emulate position:device-fixed, for a couple of
> reasons...

This makes sense; I'd love your help to work on a decent example.

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #35)

(In reply to Botond Ballo [:botond] from comment #33)

Another thought: I don't really want to encourage web developers to use the
Visual Viewport API to emulate position:device-fixed, for a couple of
reasons...

This makes sense; I'd love your help to work on a decent example.

Hi Chris! I wanted to follow up on this as the Visual Viewport API is shipping (on Android) in 68 (see bug 1512813).

We had a discussion about position: device-fixed being the leading example of the Visual Viewport API, and it looks like David from the Chromium team (and who authored the API) agrees that it's not a great example, just the most obvious one to illustrate the API.

Given that, here is a proposal for how we could proceed:

  • Turn the "Hide on zoom" example from the spec README into a second MDN example.
  • Keep the position: device-fixed MDN example, but add a footnote mentioning that emulating position: device-fixed in this way is not recommended as it can result in the fixed element flickering during scrolling.

How does that sound?

Flags: needinfo?(cmills)

(In reply to Botond Ballo [:botond] from comment #36)

Hi Chris! I wanted to follow up on this as the Visual Viewport API is shipping (on Android) in 68 (see bug 1512813).

We had a discussion about position: device-fixed being the leading example of the Visual Viewport API, and it looks like David from the Chromium team (and who authored the API) agrees that it's not a great example, just the most obvious one to illustrate the API.

Given that, here is a proposal for how we could proceed:

  • Turn the "Hide on zoom" example from the spec README into a second MDN example.
  • Keep the position: device-fixed MDN example, but add a footnote mentioning that emulating position: device-fixed in this way is not recommended as it can result in the fixed element flickering during scrolling.

How does that sound?

I like it; done: https://developer.mozilla.org/en-US/docs/Web/API/VisualViewport#Examples

Let me know if you think this looks OK.

I changed the wording of the footnote to "should be used with care" rather than "not recommended". I feel that if we don't recommend it, then why show it at all? We could even swap it out for the "fixed to keyboard" example, if you think that would be better.

FYI, we are following bug 1512813 too, and will make sure to update the compat data in our round of Fx68 changes

Flags: needinfo?(cmills)

Yup, that looks good, thanks!

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #37)

I changed the wording of the footnote to "should be used with care" rather than "not recommended". I feel that if we don't recommend it, then why show it at all? We could even swap it out for the "fixed to keyboard" example, if you think that would be better.

Fair enough. Keeping the example and the current "should be used with care" wording seems reasonable to me.

Fair enough. Keeping the example and the current "should be used with care" wording seems reasonable to me.

OK, cool. If you're happy, then I'm happy.

No longer regressions: 1804409
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: