1560770 - Add-on popups are tiny and unusable with apz.allow_zooming=true

Status: VERIFIED FIXED

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

Description

[Asif Youssuff]

Attachment: zooming.png

STR:

1. Install uBlock Origin
2. set apz.allow_zooming=true
3. restart Firefox
4. click uBlock Origin icon in toolbar

What happens:

The popup is tiny and unusuable.

Expected result:

Popup should work like when apz.allow_zooming=false.

This has been an issue since at least version 57, based on a quick mozregression check -- at least on my Ubuntu Eoan machine.

Comment 3

[nelson]

Interestingly, if the add-on icon is moved into the overflow menu, the popup will then display properly. FF 68.0.1 on macOS 10.14.5.

Comment 4

[mkatych]

Description of the bug:

When apz.allow_zooming is true in about:config, the html body fails to display the Add-on.

Versions of Firefox tested:

• Firefox Developer Edition 69.0b10 (64-bit)
• Firefox Stable 68.0.1 (64-bit)

Workarounds:

• setting apz.allow_zooming back to false restores previous functionality
• When pinned to overflow menu, the exrension's DOM elements render as expected

Comment 5

[mkatych]

Attachment: pinch zoom macos bug

Comment 6

[mkatych]

Comment on attachment 9084572 [details]
pinch zoom macos bug

https://user-images.githubusercontent.com/11223234/62527510-cd0f9b00-b82a-11e9-8cb1-c146d7ba8e03.png

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Botond, do you have any idea what the cause of this might be?

Comment 9

[Botond Ballo [:botond]]

I've spent several hours debugging this today, but haven't gotten very far.

I've traced the issue as far as the width and height of the resolved styles of popup's nsSubDocumentFrame being zero. What I'd like to do next is look at the corresponding <iframe> element in the Browser Toolbox and see where its resolved width and height are coming from in the Inspector, but it doesn't look like you can actually look at popup content in the Browser Toolbox (trying to interact with the toolbox causes the popup to close).

Comment 10

[Botond Ballo [:botond]]

I was able to trace the issue further, to nsDocumentViewer::GetContentSizeInternal() returning an empty size with desktop zooming enabled.

That in turn is due to this branch being taken in PresShell::ResizeReflow(). We early-exit here and never get to ResizeReflowIgnoreOverride, and therefore never give the popup's content document a chance to tell us the size it wants.

Conceptually, we are trying to give the popup document "as much space as it needs", and so ask it to compute its size without a viewport size as a starting point. However, with desktop zooming we use MobileViewportManager's sizing logic which requires a viewport size as a starting point.

Comment 11

[Botond Ballo [:botond]]

While we could try to tweak MVM's sizing logic, I think the simpler thing to do here is not to use MVM for the popup's content document. After all, we're not going to allow zooming the popup's content, at least not for now.

Comment 12

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=b02dd5e55e80efdf76c0b9bb27b9ffd81b205e67

Comment 13

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=ce58012882154cc3b38df40fd90789ad92e8aa98

Comment 14

[Botond Ballo [:botond]]

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

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=ce58012882154cc3b38df40fd90789ad92e8aa98

Somehow, I'm managing to leak the world with these patches...

Comment 15

[Botond Ballo [:botond]]

Oh, I'm using an API that expects to be used with getter_AddRefs.

Comment 16

[Botond Ballo [:botond]]

Attachment: Bug 1560770 - Don't use MobileViewportManager if we're not using APZ. r=tnikkel

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f96dbf78c2e1
Don't use MobileViewportManager if we're not using APZ. r=tnikkel

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f96dbf78c2e1

Comment 19

[Jeff Muizelaar [:jrmuizel]]

I still see this problem on Mac with 20191022094606 which includes this fix.

Comment 20

[Botond Ballo [:botond]]

Hmm. The fix was broken either during the process of fixing the leaks, or during the subsequent rebase.

Comment 21

[Botond Ballo [:botond]]

Attachment: Bug 1560770 - Use a method of getting the widget in UseMobileViewportManager() than does not require the frame tree to be constructed. r=tnikkel

GetNearestWidget() requires the frame tree to have been constructed, which
may not be the case at the case UseMobileViewportManager() is called.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92505a6990ee
Use a method of getting the widget in UseMobileViewportManager() than does not require the frame tree to be constructed. r=tnikkel

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/92505a6990ee

Comment 29

[Cristian Fogel [:cfogel]]

Confirmed issue with 69.0a1 (2019-06-23) on Windows 10.
Fix verified with 72.0b5 on Windows 10, macOS 10.15, Ubuntu 18.04