The failure of `test_viewport_metrics_on_landscape_content.html` (among other things) in the most recent Try push has convinced me that the approach I've been pursuing here may not be workable. Let me summarize the current approach, why it may not be workable, and what some possible alternatives are. First, some background: * Gecko code that runs on the main thread (e.g. that works with the frame tree) typically works with coordinates that do not include the pinch-zoom resolution. For example, this is the case for the main thread hit-testing code (`nsLayoutUtils::GetFrameForPoint()`), and code that exposes values to web content (e.g. `window.scrollX`). * Such coordinates also do not include any offset of the visual viewport relative to the layout viewport that may have been introduced by pinch-zooming. (For example, `getBoundingClientRect()` returns coordinates relative to the layout viewport.) * Prior to rendering, coordinates in e.g. the frame tree need these transforms (the resolution, and the offset between the visual and layout viewports) applied to get coordinates in screen space. * Conversely, when handling input events, we start with coordinates in screen space, and then want to do things like main thread hit-testing or dispatching DOM events, so we have to apply the reverse of these transforms. (The reverse of these transforms is often referred to as the "callback transform" in the code, because historically it was applied by code in `APZCCallbackHelper`.) * Currently, we apply the callback transform when events cross the process boundary from the chrome process to a content process. * This is problematic because scrollbars are drawn by the content process, but we don't want the resolution to apply to them. * So far, we've dealt with this by fixing up the position of scrollbars for rendering purposes in the compositor. Notably, we've not had to deal with this for hit testing purposes, because scrollbars cannot be interacted with on mobile. * Now that we are trying to bring pinch-zooming to desktop, where scrollbars are draggable, we need to solve this problem for hit-testing purposes. The current envisioned solution, as discussed during the Whistler All Hands, is to move the place where apply the callback transform, from the process boundary, to the *async zoom container*. The async zoom container is a display item added during the [containerless scrolling refactor](https://bugzilla.mozilla.org/show_bug.cgi?id=1459312), which wraps the display items for scrolled content and fixed content, but not the display items for scrollbars. This way, scrolled and fixed content are subject to the resolution, but not scrollbars are not, as desired. In this bug, I've been trying to implement this move. Unfortunately, it hasn't been straightforward. One of the reasons for this is that coordinates are often expressed as being relative to a frame, but the async zoom container is a notion in the display list, not in the frame tree. I've been trying to map the notion of the resolution being "at the level of the async zoom container" onto the frame tree as follows: * the `ViewportFrame` is outside the resolution * the root scroll frame and its contents are inside the resolution * frames which are fixed to the viewport are inside the resolution In particular, I've had to make things so that whenever a coordinate is expressed relative to the `ViewportFrame`, it has the resolution applied, but whenever it's expressed relative to a frame representing scrolled or fixed content, it does not have the resolution applied. This has necessitated adding code to apply or unapply the resolution in at least the following places: * `nsDisplayAsyncZoomContainer::HitTest` * `ScrollFrameHelper::BuildDisplayList` * elsewhere in display list building code where we enter fixed content * `nsLayoutUtils::GetEventCoordinatesRelativeTo` * most recently, `nsLayoutUtils::TransformFrameRectToAncestor` With the most recent change to `TransformFrameRectToAncestor`, I've run into a significant snag: there is code like [`Element::GetBoundingClientRect()`](https://searchfox.org/mozilla-central/rev/97976753a21c1731e18177de9e5ce78ea3b3da2d/dom/base/Element.cpp#921) which computes coordinates relative to an element's containing block -- which, for e.g. the document element is the viewport frame -- and then exposes them to web content. With my current changes, asking for coordinates relative to the viewport frame will give you coordinates that have the resolution applied, but we definitely don't want that for e.g. `documentElement.getBoundingClientRect()`. It seems likely that this is one of many places where we compute coordinates relative to the viewport frame and then make the result available to web content, making the fact that the viewport frame is outside the resolution problematic. So, I'd like to take a step back, and evaluate our options / consider possible alternatives: 1. **Press on with the current approach.** This will mean modifying `Element::GetBoundingClientRect()`, and other places that ask for coordinates relative to the viewport frame in the process of computing a quantity exposed to web content. It's not clear what these modifications would look like, and there's a good chance that a lot of places will need to be modified. 2. **Represent the async zoom container as its own frame in the frame tree.** This sounds like a very scary change to me, because it's a pretty fundamental modification to the frame tree model. It would also likely mean changing many places in the code to replace uses of the viewport frame with uses of the new frame type where appropriate. 3. **Continue unapplying the resolution at the process boundary, but re-apply it when entering scrollbars.** This would largely keep things as-is prior to this bug, and solve the problem of hit-testing scrollbars in a more targeted way. I'm leaning towards the third option; while it's the hackiest of the three, it's also the least likely to involve large-surface-area modifications. It's not entirely clear to me yet how re-applying the resolution when entering scrollbars would work, but it at least seems like a change localized to scrollbar frames rather than mucking with the viewport frame. Matt / Timothy / Markus, I would appreciate any thoughts you have on this!
Bug 1556556 Comment 19 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Some of the failures in the most recent Try push have convinced me that the approach I've been pursuing here may not be workable. Let me summarize the current approach, why it may not be workable, and what some possible alternatives are. First, some background: * Gecko code that runs on the main thread (e.g. that works with the frame tree) typically works with coordinates that do not include the pinch-zoom resolution. For example, this is the case for the main thread hit-testing code (`nsLayoutUtils::GetFrameForPoint()`), and code that exposes values to web content (e.g. `window.scrollX`). * Such coordinates also do not include any offset of the visual viewport relative to the layout viewport that may have been introduced by pinch-zooming. (For example, `getBoundingClientRect()` returns coordinates relative to the layout viewport.) * Prior to rendering, coordinates in e.g. the frame tree need these transforms (the resolution, and the offset between the visual and layout viewports) applied to get coordinates in screen space. * Conversely, when handling input events, we start with coordinates in screen space, and then want to do things like main thread hit-testing or dispatching DOM events, so we have to apply the reverse of these transforms. (The reverse of these transforms is often referred to as the "callback transform" in the code, because historically it was applied by code in `APZCCallbackHelper`.) * Currently, we apply the callback transform when events cross the process boundary from the chrome process to a content process. * This is problematic because scrollbars are drawn by the content process, but we don't want the resolution to apply to them. * So far, we've dealt with this by fixing up the position of scrollbars for rendering purposes in the compositor. Notably, we've not had to deal with this for hit testing purposes, because scrollbars cannot be interacted with on mobile. * Now that we are trying to bring pinch-zooming to desktop, where scrollbars are draggable, we need to solve this problem for hit-testing purposes. The current envisioned solution, as discussed during the Whistler All Hands, is to move the place where apply the callback transform, from the process boundary, to the *async zoom container*. The async zoom container is a display item added during the [containerless scrolling refactor](https://bugzilla.mozilla.org/show_bug.cgi?id=1459312), which wraps the display items for scrolled content and fixed content, but not the display items for scrollbars. This way, scrolled and fixed content are subject to the resolution, but not scrollbars are not, as desired. In this bug, I've been trying to implement this move. Unfortunately, it hasn't been straightforward. One of the reasons for this is that coordinates are often expressed as being relative to a frame, but the async zoom container is a notion in the display list, not in the frame tree. I've been trying to map the notion of the resolution being "at the level of the async zoom container" onto the frame tree as follows: * the `ViewportFrame` is outside the resolution * the root scroll frame and its contents are inside the resolution * frames which are fixed to the viewport are inside the resolution In particular, I've had to make things so that whenever a coordinate is expressed relative to the `ViewportFrame`, it has the resolution applied, but whenever it's expressed relative to a frame representing scrolled or fixed content, it does not have the resolution applied. This has necessitated adding code to apply or unapply the resolution in at least the following places: * `nsDisplayAsyncZoomContainer::HitTest` * `ScrollFrameHelper::BuildDisplayList` * elsewhere in display list building code where we enter fixed content * `nsLayoutUtils::GetEventCoordinatesRelativeTo` * most recently, `nsLayoutUtils::TransformFrameRectToAncestor` With the most recent change to `TransformFrameRectToAncestor`, I've run into a significant snag: there is code like [`Element::GetBoundingClientRect()`](https://searchfox.org/mozilla-central/rev/97976753a21c1731e18177de9e5ce78ea3b3da2d/dom/base/Element.cpp#921) which computes coordinates relative to an element's containing block -- which, for e.g. the document element is the viewport frame -- and then exposes them to web content. With my current changes, asking for coordinates relative to the viewport frame will give you coordinates that have the resolution applied, but we definitely don't want that for e.g. `documentElement.getBoundingClientRect()`. It seems likely that this is one of many places where we compute coordinates relative to the viewport frame and then make the result available to web content, making the fact that the viewport frame is outside the resolution problematic. So, I'd like to take a step back, and evaluate our options / consider possible alternatives: 1. **Press on with the current approach.** This will mean modifying `Element::GetBoundingClientRect()`, and other places that ask for coordinates relative to the viewport frame in the process of computing a quantity exposed to web content. It's not clear what these modifications would look like, and there's a good chance that a lot of places will need to be modified. 2. **Represent the async zoom container as its own frame in the frame tree.** This sounds like a very scary change to me, because it's a pretty fundamental modification to the frame tree model. It would also likely mean changing many places in the code to replace uses of the viewport frame with uses of the new frame type where appropriate. 3. **Continue unapplying the resolution at the process boundary, but re-apply it when entering scrollbars.** This would largely keep things as-is prior to this bug, and solve the problem of hit-testing scrollbars in a more targeted way. I'm leaning towards the third option; while it's the hackiest of the three, it's also the least likely to involve large-surface-area modifications. It's not entirely clear to me yet how re-applying the resolution when entering scrollbars would work, but it at least seems like a change localized to scrollbar frames rather than mucking with the viewport frame. Matt / Timothy / Markus, I would appreciate any thoughts you have on this!
Some of the failures in the most recent Try push have convinced me that the approach I've been pursuing here may not be workable. Let me summarize the current approach, why it may not be workable, and what some possible alternatives are. First, some background: * Gecko code that runs on the main thread (e.g. that works with the frame tree) typically works with coordinates that do not include the pinch-zoom resolution. For example, this is the case for the main thread hit-testing code (`nsLayoutUtils::GetFrameForPoint()`), and code that exposes values to web content (e.g. `window.scrollX`). * Such coordinates also do not include any offset of the visual viewport relative to the layout viewport that may have been introduced by pinch-zooming. (For example, `getBoundingClientRect()` returns coordinates relative to the layout viewport.) * Prior to rendering, coordinates in e.g. the frame tree need these transforms (the resolution, and the offset between the visual and layout viewports) applied to get coordinates in screen space. * Conversely, when handling input events, we start with coordinates in screen space, and then want to do things like main thread hit-testing or dispatching DOM events, so we have to apply the reverse of these transforms. (The reverse of these transforms is often referred to as the "callback transform" in the code, because historically it was applied by code in `APZCCallbackHelper`.) * Currently, we apply the callback transform when events cross the process boundary from the chrome process to a content process. * This is problematic because scrollbars are drawn by the content process, but we don't want the resolution to apply to them. * So far, we've dealt with this by fixing up the size and position of scrollbars for rendering purposes in the compositor. Notably, we've not had to deal with this for hit testing purposes, because scrollbars cannot be interacted with on mobile. * Now that we are trying to bring pinch-zooming to desktop, where scrollbars are draggable, we need to solve this problem for hit-testing purposes. The current envisioned solution, as discussed during the Whistler All Hands, is to move the place where apply the callback transform, from the process boundary, to the *async zoom container*. The async zoom container is a display item added during the [containerless scrolling refactor](https://bugzilla.mozilla.org/show_bug.cgi?id=1459312), which wraps the display items for scrolled content and fixed content, but not the display items for scrollbars. This way, scrolled and fixed content are subject to the resolution, but not scrollbars are not, as desired. In this bug, I've been trying to implement this move. Unfortunately, it hasn't been straightforward. One of the reasons for this is that coordinates are often expressed as being relative to a frame, but the async zoom container is a notion in the display list, not in the frame tree. I've been trying to map the notion of the resolution being "at the level of the async zoom container" onto the frame tree as follows: * the `ViewportFrame` is outside the resolution * the root scroll frame and its contents are inside the resolution * frames which are fixed to the viewport are inside the resolution In particular, I've had to make things so that whenever a coordinate is expressed relative to the `ViewportFrame`, it has the resolution applied, but whenever it's expressed relative to a frame representing scrolled or fixed content, it does not have the resolution applied. This has necessitated adding code to apply or unapply the resolution in at least the following places: * `nsDisplayAsyncZoomContainer::HitTest` * `ScrollFrameHelper::BuildDisplayList` * elsewhere in display list building code where we enter fixed content * `nsLayoutUtils::GetEventCoordinatesRelativeTo` * most recently, `nsLayoutUtils::TransformFrameRectToAncestor` With the most recent change to `TransformFrameRectToAncestor`, I've run into a significant snag: there is code like [`Element::GetBoundingClientRect()`](https://searchfox.org/mozilla-central/rev/97976753a21c1731e18177de9e5ce78ea3b3da2d/dom/base/Element.cpp#921) which computes coordinates relative to an element's containing block -- which, for e.g. the document element is the viewport frame -- and then exposes them to web content. With my current changes, asking for coordinates relative to the viewport frame will give you coordinates that have the resolution applied, but we definitely don't want that for e.g. `documentElement.getBoundingClientRect()`. It seems likely that this is one of many places where we compute coordinates relative to the viewport frame and then make the result available to web content, making the fact that the viewport frame is outside the resolution problematic. So, I'd like to take a step back, and evaluate our options / consider possible alternatives: 1. **Press on with the current approach.** This will mean modifying `Element::GetBoundingClientRect()`, and other places that ask for coordinates relative to the viewport frame in the process of computing a quantity exposed to web content. It's not clear what these modifications would look like, and there's a good chance that a lot of places will need to be modified. 2. **Represent the async zoom container as its own frame in the frame tree.** This sounds like a very scary change to me, because it's a pretty fundamental modification to the frame tree model. It would also likely mean changing many places in the code to replace uses of the viewport frame with uses of the new frame type where appropriate. 3. **Continue unapplying the resolution at the process boundary, but re-apply it when entering scrollbars.** This would largely keep things as-is prior to this bug, and solve the problem of hit-testing scrollbars in a more targeted way. I'm leaning towards the third option; while it's the hackiest of the three, it's also the least likely to involve large-surface-area modifications. It's not entirely clear to me yet how re-applying the resolution when entering scrollbars would work, but it at least seems like a change localized to scrollbar frames rather than mucking with the viewport frame. Matt / Timothy / Markus, I would appreciate any thoughts you have on this!