1682743 - select drop down menu position is wrong in transformed OOP iframes

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case

It seems I missed the case in bug 1550800. And I am not sure yet where we need to incorporate the transform value.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

So a problem I noticed is that for dropdown (or some such) positions in OOP iframes use PuppetWidget::WidgetToScreenOffset (via mozInnerScreen{X,Y}) to get the coordinate origin of the document inside the iframe, but it's the origin, it doesn't convert positions inside the iframe by the outer transform values, in this test case, scale(2) is a transform in the parent document.

Presumably one of reasonable place to factor outer transforms is inside ViewportUtils::GetVisualToLayoutTransform.

Another possible solution I can think of is to introduce a new function in PuppetWidget (or BrowserChild) to convert a given position in the OOP iframe units (app units/css units) to screen coordinate system.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

Presumably one of reasonable place to factor outer transforms is inside ViewportUtils::GetVisualToLayoutTransform.

This is not going to work. The function is supposed to be a conversion in the same units, whereas for downdown position case, we do convert a given rect to screen coordinates only if |aInScreenCoords| is true.

So,

Another possible solution I can think of is to introduce a new function in PuppetWidget (or BrowserChild) to convert a given position in the OOP iframe units (app units/css units) to screen coordinate system.

We probably need to add this new function and expose it in JS.
CCing Neil.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Hmm I may be wrong. ViewportUtils::GetVisualToLayoutTransform is probably supposed to convert from in a coordinate system to a different coordinate system in the same unit?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Note that I've noticed this case doesn't work on Chrome on at least Linux, I haven't tried on other platforms.

Comment 5

[Neha Kochar [:neha]]

Hiro, can you please check this on Chrome with other platforms too, so we know how to prioritize fixing this? Thanks!

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Confirmed it doesn't work with Chrome on Window either. I also confirmed that Neil's patch in bug 1684795 fixes this issue too. I will duplicate this as the bug once after the bug 1684795 fixed, but for now I am going to make this bug block to bug 1684795.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1682743 - Drop setting apz.allow_zooming in browser_test_select_zoom.js. r?tnikkel

It's already set in browser.ini.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1682743 - Use hideSelectPopup in /browser/base/content/test/forms/head.js. r?tnikkel

The intension behind this change is that we want to factor out openSelectPopup
and hideSelectPopup in an independnt js file to reuse the functions for
different browser tests but unfortunately factoring out the hideSelectPopup
causes the following lint error;

ChromeUtils.import should not be called with (..., null) to retrieve the JSM
global object. Rely on explicit exports instead.
mozilla/reject-chromeutils-import-null (eslint)

As of now, there is an on-going work to fix ChromeUtils.import calls with null
(bug 1609271) and the reason why the same error doesn't happen in the head.js is
the file is excluded from lint targets [1]. Fixing the error properly requires
SelectChild.jsm modifications so we defer the issue to bug 1609271 and re-use
the head.js directly instead.

[1] https://searchfox.org/mozilla-central/rev/6a6a366031680829746b5d2362610b868fd9571a/.eslintrc.js#511

Depends on D104365

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1682743 - Add helper_browser_test_utils.js to use some popup related functions for other browser tests. r?tnikkel

Depends on D104366

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1682743 - A new test for popup window position in OOP iframe transformed by an ancestor element in the parent document. r?tnikkel

Depends on D104367

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1682743 - A new test for popup window position in OOP iframe scaled by the desktop zoom. r?tnikkel

Depends on D104368

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1682743 - Skip the test for popup window position in iframe transformed by an ancestor in the parent document. r?tnikkel

The issue will be handled in bug 1691346.

Depends on D104369

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c77e4c5e140
Drop setting apz.allow_zooming in browser_test_select_zoom.js. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/a10f4eaeaae8
Use hideSelectPopup in /browser/base/content/test/forms/head.js. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/67c83ae3b221
Add helper_browser_test_utils.js to use some popup related functions for other browser tests. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/3d7ef339f822
A new test for popup window position in OOP iframe transformed by an ancestor element in the parent document. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/659ca747e61d
A new test for popup window position in OOP iframe scaled by the desktop zoom. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/c88e15c44b91
Skip the test for popup window position in iframe transformed by an ancestor in the parent document. r=tnikkel

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4c77e4c5e140
https://hg.mozilla.org/mozilla-central/rev/a10f4eaeaae8
https://hg.mozilla.org/mozilla-central/rev/67c83ae3b221
https://hg.mozilla.org/mozilla-central/rev/3d7ef339f822
https://hg.mozilla.org/mozilla-central/rev/659ca747e61d
https://hg.mozilla.org/mozilla-central/rev/c88e15c44b91