Audit ViewportUtils::GetVisualToLayoutTransform for fission
Categories
(Core :: Panning and Zooming, task, P2)
Tracking
()
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.
Comment 1•4 years ago
|
||
(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.
Updated•4 years ago
|
Updated•4 years ago
|
Comment 2•4 years ago
|
||
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.
Reporter | ||
Comment 3•4 years ago
|
||
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.
Comment 4•4 years ago
|
||
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.
Comment 5•4 years ago
|
||
This should also have been tracked for desktop-zoom-post, not sure how it was overlooked.
Updated•4 years ago
|
Assignee | ||
Comment 6•4 years ago
|
||
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)
Comment 7•4 years ago
|
||
(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.
Assignee | ||
Comment 8•4 years ago
|
||
(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.
Assignee | ||
Comment 9•4 years ago
|
||
(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.
Comment 10•4 years ago
|
||
(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.
Assignee | ||
Comment 11•4 years ago
|
||
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.
Assignee | ||
Comment 12•4 years ago
|
||
So, what I've understand so far are;
- 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)
- 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
- 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;
- 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)
- 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)
- 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.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 13•4 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
- 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.
Assignee | ||
Comment 14•4 years ago
|
||
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.
Description
•