Expose the Visual Viewport API to web content

RESOLVED FIXED in Firefox 63

Status

()

P3
normal
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: kats, Assigned: tanushree, Mentored)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete, feature})

unspecified
mozilla63
dev-doc-complete, feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 attachments, 2 obsolete attachments)

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]
Blocks: 1395365
(Assignee)

Updated

9 months ago
Assignee: nobody → tpodder
Mentor: botond
(Assignee)

Updated

9 months ago
Depends on: 1470267
(Assignee)

Updated

8 months ago
Depends on: 1477610
(Assignee)

Updated

8 months ago
Blocks: 1477829
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

8 months ago
mozreview-review
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 4

8 months ago
mozreview-review
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)
(Assignee)

Updated

8 months ago
Blocks: 1478776
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8995043 - Attachment is obsolete: true
(Assignee)

Comment 6

8 months ago
mozreview-review-reply
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.
(Assignee)

Comment 7

8 months ago
mozreview-review
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"
(Assignee)

Comment 8

8 months ago
mozreview-review
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 9

8 months ago
mozreview-review
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)

Comment 10

8 months ago
> 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 11

8 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

8 months ago
mozreview-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 hidden (mozreview-request)
(Assignee)

Comment 16

8 months ago
mozreview-review-reply
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 17

8 months ago
mozreview-review
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)
(Assignee)

Comment 18

7 months ago
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
Comment hidden (mozreview-request)
Attachment #8999444 - Attachment is obsolete: true
(Assignee)

Comment 20

7 months ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Blocks: 1484523

Comment 23

7 months ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 25

7 months ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a236c987f26c
Expose the Visual Viewport API to web content. r=botond,Nika
(Assignee)

Comment 27

7 months ago
Addressed the build failures and updated the patch. Should be good for landing.
Flags: needinfo?(tanushree.podder.103)

Comment 28

7 months ago
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!

Comment 30

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9044d545791f
Status: NEW → RESOLVED
Last Resolved: 7 months 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.
You need to log in before you can comment on or make changes to this bug.