Closed Bug 1649447 Opened 4 years ago Closed 3 years ago

Audit ViewportUtils::GetVisualToLayoutTransform for fission

Categories

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

Product:
Core
Component:
Panning and Zooming
Platform:
Unspecified
All
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Project Flags:
Fission Milestone M6c

People

(Reporter: kats, Assigned: hiro)

References

(Blocks 2 open bugs)

Details

ViewportUtils::GetVisualToLayoutTransform uses APZCCallbackHelper::GetRootContentDocumentPresShellForContent which in turn uses nsPresContext::GetToplevelContentDocumentPresContext which is not fission-friendly. This will probably impact correctness of hit-testing and event targeting in a fission + desktop zooming world.

I think in the past we discussed how to handle this and one of the options was to have the root content presShell notify all its OOP descendants of any resolution changes. That way all the presShells can be queried for the resolution in-process, and that will avoid have to send IPC messages to query the resolution for situations where this function is used. There might be alternative solutions too.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0)

There might be alternative solutions too.

Another option would be to just always use layout coordinates in OOP iframe processes. Everything in an OOP iframe is inside the zoom boundary, so there should not be a real need for visual coordinates.

However, this may be more involved from an implementation point of view.

Fission Milestone: --- → ?
Fission Milestone: ? → M6c

If and when zooming in desktop is intending to ship, please make sure it works with Fission.
For now, the only use is Geckoview, so putting this in Fission Future milestone for now. This should be tracked under fission-geckoview milestone when we create one.

Fission Milestone: M6c → Future

Relatedly, https://phabricator.services.mozilla.com/D86983 is adding some usages of GetInProcessRootContentDocumentPresContext() in ContentEventHandler.cpp that will also suffer from the same problem, and will need to get the layout-to-visual transform potentially from a different process.

This sounds like it may need to block M6c as it may affect event targeting for elements inside an OOP iframe on a page that has been zoomed in using desktop zooming.

Fission Milestone: Future → M6c

This should also have been tracked for desktop-zoom-post, not sure how it was overlooked.

Blocks: desktop-zoom-post
Priority: P3 → P2

I started looking at this issue, but I don't yet understand what cases are involved by this issue.

I tried the STR in bug 1658927 comment 0 which is an issue kats commented in comment 3 is one of the call sites of ViewportUtils::DocumentRelativeLayoutToVisual which ends up calling ViewportUtils::GetVisualToLayoutTransform on a site that I've been testing for fission stuff, this, https://hsivonen.fi/fission-host.html. Fortunately I don't see any problems there, the candidate popup window shows up at proper places. So at least for the case, it looks working properly, even in Responsive Design Mode.

Masayuki, you are the reviewer of bug 1658927, can you audit the usage in ContentEventHandler::OnQueryTextRectArray should work in fission world as it is, and/or can you please double-check the candidate window position is correct in fission world with desktop zooming?

Thanks!

(Assigning this bug to myself too)

Assignee: nobody → hikezoe.birchill
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)

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

a site that I've been testing for fission stuff, this, https://hsivonen.fi/fission-host.html

I see at least some bugginess on that site:

  • If I load the site with Fission disabled and pinch-zoom in, all of the page content (including iframe contents) is scaled and positioned correctly.
  • If I load the site with Fission enabled and pinch-zoom in, the content gets scaled correctly, but the contents of the iframe additionally get shifted, such that e.g. the text input element is partially hidden.

It's not clear to me yet if these symptoms are related to GetResolution() (bug 1518908), GetVisualToLayoutTransform() (this bug), or a combination.

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

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

a site that I've been testing for fission stuff, this, https://hsivonen.fi/fission-host.html

I see at least some bugginess on that site:

  • If I load the site with Fission disabled and pinch-zoom in, all of the page content (including iframe contents) is scaled and positioned correctly.
  • If I load the site with Fission enabled and pinch-zoom in, the content gets scaled correctly, but the contents of the iframe additionally get shifted, such that e.g. the text input element is partially hidden.

Yeah, I noticed it and I haven't filed a bug for the case yet. In my environments, it only happens without WebRender and happened only for the first iframe which is not transformed, thus I think it's somewhat non-WebRender specific issue, not related to this issue. Anyway, yesterday I did't have enough time to dig into detail the issue, I will have to take a look again and will file a new bug for the issue.

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

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

  • If I load the site with Fission enabled and pinch-zoom in, the content gets scaled correctly, but the contents of the iframe additionally get shifted, such that e.g. the text input element is partially hidden.

Yeah, I noticed it and I haven't filed a bug for the case yet. In my environments, it only happens without WebRender and happened only for the first iframe which is not transformed, thus I think it's somewhat non-WebRender specific issue, not related to this issue. Anyway, yesterday I did't have enough time to dig into detail the issue, I will have to take a look again and will file a new bug for the issue.

Filed bug 1682197.

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

Masayuki, you are the reviewer of bug 1658927, can you audit the usage in ContentEventHandler::OnQueryTextRectArray should work in fission world as it is, and/or can you please double-check the candidate window position is correct in fission world with desktop zooming?

I tested with the Henri's testcase, but I don't see any problem in Nightly on per-monitor DPI environment.

Flags: needinfo?(masayuki)

Thank you, Masayuki!

I've noticed that one of the use cases of the function is still problematic, the use case is positioning of select dropdown menu. You can see the problem in https://developer.mozilla.org/en-US/docs/Web/HTML/Element/select. And I've already fixed it locally with tweaking ViewportUtils::GetVisualToLayoutTransform as kats suggested in comment 0, but it regressed the candidate window position. :/ There is at least one inconsistency among the use cases unfortunately.

So, what I've understand so far are;

  1. BrowserChiild::mCHildToParentConversionMatrix is including the desktop zoom value if the BrowserChild is for OOP iframe (IMPORTANT NOTE: the top level content's BrowserChild doesn't include the desktop zoom value)
  2. For IME popups in OOP iframes we convert the coordinate by using the matrix so that it works fine in the fission world with desktop zooming
  3. For IME popups in the top level content document, we apply the desktop zoom value as kats fixed it in bug 1658927 so that it works fine with desktop zooming

what I am currently thinking are;

  1. We don't need to change ViewportUtils::GetVisualToLayoutTransform, it should work as it is (i.e. we don't need to apply the desktop zoom value for OOP iframes in the function)
  2. Select dropdown popup positions in OOP iframes are still wrong, we need to convert the select element rect by using the transform matrix instead of applying mozInnerScreenX,Y (I will file a new bug for this case)
  3. Related to 2), form validation popup positions are also wrong, I've filed bug 1684792 for it, maybe 2) and 3) might be solved at once

I am changing this bug title. I think we can close this bug now, but still I am thinking whether we should add some comments in GetVisualToLayoutTransform about fission case. I am also wondering whether we could provide a new handy function to coordinates regardless of whether it's in OOP iframe, whether there is the desktop zoom value being applied or not.

Summary: Make ViewportUtils::GetVisualToLayoutTransform fission-friendly → Audit ViewportUtils::GetVisualToLayoutTransform for fission
See Also: → 1684795

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

  1. Select dropdown popup positions in OOP iframes are still wrong, we need to convert the select element rect by using the transform matrix instead of applying mozInnerScreenX,Y (I will file a new bug for this case)

Filed bug 1684795.

Closing since we won't factor the desktop zoom value into the GetVisualToLayoutTransform function in case where it's called in OOP iframe documents. For OOP iframe's documents where we want to know element positions in the documents in screen coords, we should use BrowserChild::GetChildToParentConversionMatrix (or nsIWidget::WidgetToTopLevelWidgetTransform). For the popup of select dropdown menu item position issue, I've filed bug 1684795 and it's already been assigned so will fix soon.

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.