1493208 - Re-enable APZ in popups with remote content

Status: RESOLVED FIXED

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

Description

[Ryan Hunt [:rhunt] (on leave until early May)]

APZ in popups was disabled in bug 1381097. In the long term, we'd like to re-enable it again.

Comment 1

[Botond Ballo [:botond]]

Ryan, I assume it's safe to say you're not planning to work on this? If so, could you un-assign yourself?

Comment 2

[Ryan Hunt [:rhunt] (on leave until early May)]

Yes, feel free to take :)

Comment 3

[Botond Ballo [:botond]]

While working on bug 1560770, I made some realizations which are relevant for this bug, particularly when it comes to WebExtension popups which have remote content:

• Some popups render their content into a fixed-size viewport (let's called this "type A"), while others are sized around their content ("type B").
• With desktop zooming enabled, we need to decide which popups we want to be zoomable, and which we don't. (Presumably, users of addons like Tile Tabs which render web pages in large popups that are functionally similar to a regular browser window, would prefer that such popups be zoomable.)
• Zooming a popup requires it to be remote, and requires its content to use a MobileViewportManager.
• MobileViewportManager does not currently support the notion of "sizing around content" ("type B"), it requires a fixed viewport size.
• Therefore, we'll need to either:
  • have MVM support "sizing around content"; or
  • distinguish between popups of "type A" and "type B", and only create an MVM for the former.

Comment 4

[Botond Ballo [:botond]]

During a discussion with :jrmuizel it also came up that we probably don't want to enable APZ in popups which are not actually scrollable, since it's kind of heavy-weight (requires OMTC and so on). Certainly, we don't want it for e.g. tooltips.

On the other hand, you might have a large WebExtension popup which is not scrollable at the default zoom level, but which we'd want to be able to zoom.

Perhaps "scrollable (at default zoom level) OR remote" is a reasonable condition.

Comment 5

[Chris Peterson [:cpeterson]]

Fission Milestone: --- → MVP

Moving to Fission M7 Beta milestone because we want to be feature complete before Fission rides from Nightly to Beta. All features and tests should be Fission compatible (or granted an exception) before Beta. The Fission MVP milestone is for critical bugs we find in Firefox Beta that will block Fission from riding to Release.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

If there's a failure in browser_ext_browserAction_popup_resize.js when we turn this on, it might be because of bug 1648669. In particular, the MVM check referenced in that bug will start passing (because APZ is enabled on the window) and we might start using a stale composition size when laying out the scrollbars for the popup. But maybe I'm wrong and everything will be great.

Comment 7

[Neha Kochar [:neha]]

Botond, is this required for input handling for OOP iframes in popups?

Comment 8

[Botond Ballo [:botond]]

Yes, that is the Fission connection. Input events over an OOP iframe in a popup will not be routed to the iframe's process without APZ enabled in the popup.

(There are other advantages to having APZ enabled in popups as well, such as smoother scrolling, and APZ-only scrolling features like kinetic scrolling.)

Comment 9

[Neha Kochar [:neha]]

Thanks Botond. Then this is important enough to be tracked in M6c.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I will handle this. (I will maybe open a new bug to enable APZ in only popup windows having remote iframe.)

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Pushed a try run with enabling apz.popups.enabled pref; https://treeherder.mozilla.org/jobs?repo=try&revision=77b83dd686b568d621dea9b95404750fa6a28885

The result looks good, the test browser_ext_browserAction_popup_resize.js kats was concerned worked fine there and the test works fine locally. BUT the original extension (in bug 1381097) which is one of the reason why we had to disable APZ in popups doesn't work. Popup window doesn't appear at all.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: A screenshot of a popup by an extension

The popup window does appear to some extent if the extension icon is placed at left of the URL entry field. Here is a screenshot.

So it looks like the popup window is misaligned at (0, 0) of the browser window and cut off by "the content process region + some margin".

Some caveats I've noticed;

1. The popup window works properly on Mac/Linux with WebRender
2. The popup window is misaligned as the screenshot but hit testing works as if the popup window is aligned properly on Mac/Linux with non WebRender
3. The popup window is misaligned and hit hit testing doesn't work properly on Windows with/without WebRender

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1493208 - Tweak scrollable rect offset to (0, 0) for the popup window opened by extensions. r?botond

So that we can make sure APZ handles (0, 0) based coordinates for remote
contents in popup windows.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1493208 - Enable apz.popups.enabled. r?botond

Depends on D99309

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I will land after this soft freeze window finished.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af56e843dab2
Tweak scrollable rect offset to (0, 0) for the popup window opened by extensions. r=botond
https://hg.mozilla.org/integration/autoland/rev/d8bd960f9b82
Enable apz.popups.enabled. r=botond

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/af56e843dab2
https://hg.mozilla.org/mozilla-central/rev/d8bd960f9b82

Comment 18

[Botond Ballo [:botond]]

Adjusting bug title to clarify that we enabled APZ in popups with remote content only.