1684795 - getElementBoundingRect in BrowserUtils.jsm should convert the element rect by using BrowserChild's mChildToParentConversionMatrix

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

As of now, it's just applying mozInnerScreenX,Y, but instead we need to convert the rect by using BrowserChild::mChildToParentConversionMatrix just like event handling coordinates convertion so that it works fine with desktop zooming in fission

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Neil, can you please take this issue? You can see what the problem is by applying desktop zooming on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/select. Unfortunately as of now, you can't click easily the select element on the site with applying desktop zooming because of bug 1682200 and bug 1682197.

Comment 2

[Chris Peterson [:cpeterson]]

Tracking for Fission M6c milestone like bug 1682200

Comment 3

[Neil Deakin]

What issue am I supposed to be seeing? Can you be more specific about how to reproduce this?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

So a STR is;

1. Open https://developer.mozilla.org/en-US/docs/Web/HTML/Element/select with enabling fission
2. Zoom in the content (If you don't have devices that desktop zooming is available, you can use it by Alt + mouse wheel with setting mousewheel.with_alt.action to 5)
3. Press "-- Please choose an option --" drop down menu to open a popup window (the drop down menu is in an OOP iframe)

Comment 5

[Neil Deakin]

I don't see the select popup being positioned incorrectly. The only positioning issue I see is that on Windows the popup doesn't update when using the keyboard shortcuts to zoom, but that wouldn't be affected what this bug seems to want to change.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Zoom here means not full document zoom by Ctrl+"+", it's desktop zooming something like on pinch zoom in/out on mobile browsers.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

NI to me, I will attach a screen recording to see what's happening easily.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: A recording what a popup window position is wrongly located

This is a recording on my linux box with applying the patch for bug 1682200 to do hit-testing in OOP iframe works correctly. (Note that the patch for bug 1682200 just landed on autoland)

Neil, I believe this recording makes what the issue happens easily. As I mentioned earlier, we use mozInnerScreen{X,Y} for popup window in OOP iframes, with destkop zooming we also need to scale the popup target element position by the desktop zooming value, the value is included in the matrix returned by nsIWidget::WidgetToTopLevelWidgetTransform(), so I believe what we need to do is what we do for event coordinates conversion. NI to Neil again.

Comment 9

[Neil Deakin]

Attachment: Bug 1684795 - Account for desktop zooming when opening content popups. r?tnikkel

Comment 10

[Neil Deakin]

This is what I have so far, based on what Event::GetScreenCoords does and it seems to work, but I don't actually understand what is being calculated here. The ToScreenRect does a similar computation but the additional code that was in getElementBoundingRect to calculate the parent frame offset is also needed, and I'm not clear why this is.

I'd like to have this done as part of ToScreenRect, but don't know what to call for this.

Second, is there a way to test desktop zooming? That is, simulate a zoom and wait for it to occur?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Thank you Neil!

(In reply to Neil Deakin from comment #10)

This is what I have so far, based on what Event::GetScreenCoords does and it seems to work, but I don't actually understand what is being calculated here. The ToScreenRect does a similar computation but the additional code that was in getElementBoundingRect to calculate the parent frame offset is also needed, and I'm not clear why this is.

I think that is for cases where the given element is inside an iframe (regardless of whether it's in OOP or not) and there are ancestor iframes in the same process, then we need the offsets of those ancestor iframes, but I could be wrong.

I'd like to have this done as part of ToScreenRect, but don't know what to call for this.

ToScreenRect sounds reasonable to me.

Second, is there a way to test desktop zooming? That is, simulate a zoom and wait for it to occur?

If we can do the test as a mochitest-plain test, an easy way is to add it here, you can mostly copy-and-paste an existing test.

If we need to do as a mochitest-browser test, we need to call nsIDOMWindowUtils.setResolutionAndScaleTo and waitUntilApzStable before running the test. Thanks!

Comment 12

[Neil Deakin]

I've spent too much time on this. The patch here seems to work, but it isn't the way this should work. I would expect mozInnerScreenX/Y to just return the right value. There doesn't seem to be a method that properly accounts for all of things (normal zoom, pinch zoom, oop frames, transforms) and gets the screen coordinate of an element.

I don't know how to implement this correctly.

Comment 13

[Neha Kochar [:neha]]

Botond and Hiro, can one of you please fix this Fission zooming issue?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Neil Deakin from comment #12)

I've spent too much time on this. The patch here seems to work, but it isn't the way this should work. I would expect mozInnerScreenX/Y to just return the right value.

Neil, can you please clarify what you expect the mozInnerScreenX/Y of the iframe window? A problem here is element positions inside the iframe window. The positions are in the coordinate system of the iframe, I mean, it's not scaled by the desktop zoom value. mozInnerScreenX/Y are properly scaled by the desktop zoom.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

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

A problem here is element positions inside the iframe window. The positions are in the coordinate system of the iframe, I mean, it's not scaled by the desktop zoom value.

The positions I meant are obtained by getBoundingClientRect().

EDITED: s/getComputedStyle/getBoundingClientRect/

Comment 16

[Chris Peterson [:cpeterson]]

This bug needs an assignee and an ETA.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: element-position-in-iframe.png

Here is an illustration when desktop zoom happens.

Given that there is no browser UIs and there is an html having an <iframe> positioned at (100px, 100px) and the iframe has <select> positioned at (100px, 100px).

In the right image, the content is scaled by 1.5x, but the returned value from getBoundingClientRect() is in the iframe coords, so it's still (100px, 100px), thus we need to convert it into the screen coords.

I hope this helps to make it clear what the problem is and what we need to do.

Comment 18

[Neil Deakin]

I understand that distinction, but mozInnerScreenX/Y doesn't return the correct location in some cases. It seems correct (I think) with the desktop zoom, but when the page is zoomed (using View->Zoom) it doesn't. The value seems offset in the wrong direction.

Similarly, the existing getElementBoundingRect function (which has been moved to LayoutUtils) has some code to deal with frame offsets and borders and so on, but the screen coordinates should already be accounting for this.

It would be ideal to get rid of LayoutUtils.jsm and just use a screen coordinate getting function for any element, as XULElement does (which also uses nsFrame:GetScreenRect and doesn't work in some cases).

Note that the patch I wrote does work in all cases as far as I can tell, but it is quite confusing.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Neil Deakin from comment #18)

I understand that distinction, but mozInnerScreenX/Y doesn't return the correct location in some cases. It seems correct (I think) with the desktop zoom, but when the page is zoomed (using View->Zoom) it doesn't. The value seems offset in the wrong direction.

Can you please post a test case with STR? (or maybe you've already included the case as an automated test in the patch).

Similarly, the existing getElementBoundingRect function (which has been moved to LayoutUtils) has some code to deal with frame offsets and borders and so on, but the screen coordinates should already be accounting for this.

In cases of OOP iframes, I think it's been working (the off value is propagated into APZ then propagated into each BrowserChild, thus I think BrowserChild::mChildToParentConversionMatrix has the offset). Are you saying non-OOP iframe cases? That's a pre-existing long standing issue I guess (since bug 1262332?).

It would be ideal to get rid of LayoutUtils.jsm and just use a screen coordinate getting function for any element, as XULElement does (which also uses nsFrame:GetScreenRect and doesn't work in some cases).

To me, your patch D102219 looks a great step towards the ideal function. Can we do the further cleanup (I mean getting rid of the iframe border compensation) in a later bug?

Note that the patch I wrote does work in all cases as far as I can tell, but it is quite confusing.

What is your preferable way to step forward this bug?

Comment 20

[Hiroyuki Ikezoe (:hiro)]

I am going to steal his patch (D102219) and add some tests.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

I tried to write a mochitest based on browser_test_select_zoom.js, but turned out pinchZoomInWithTouch seems not to work for OOP iframes, it opens context menu instead somehow! So I am going to write a new browser mochitest with nsIDOMWindowUtils.setResolutionAndScaleTo instead of pinchZoomWithTouch. that said, we don't have much time until the date of Fission M6c. So I am going to review request the patch for now, and will add the new browser mochitest later (probably on Monday).

FWIW, here is a try run

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25c2a2f9c81f
Account for desktop zooming when opening content popups. r=tnikkel

Comment 23

[Timothy Nikkel (:tnikkel)]

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

I tried to write a mochitest based on browser_test_select_zoom.js, but turned out pinchZoomInWithTouch seems not to work for OOP iframes, it opens context menu instead somehow!

That test was annoying to write and pinchZoomInWithTouch seemed very finicky.

Comment 24

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/25c2a2f9c81f