Closed Bug 1566422 Opened 5 years ago Closed 4 years ago

[Fission] Make content link tooltip handling work correctly in Fission

Categories

(Core :: DOM: Navigation, task, P3)

68 Branch
task

Tracking

()

RESOLVED FIXED
81 Branch
Fission Milestone M6b
Tracking Status
firefox73 --- wontfix
firefox81 --- fixed

People

(Reporter: Gijs, Assigned: u608768)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

The current code relies on document/docshell hierarchies that we have no guarantees about in fission. It should be using browsing contexts instead. Effectively, when the child notifies the parent that it wants to display a tooltip (which currently happens over IPC) then it should be clear to the parent which browsingcontext includes the node that triggered that tooltip. Hopefully, instead of handling this in the child, we can delegate to the parent the work of hiding the tooltip when the browsing context goes away.

Nika, is there an existing way (event/message) to know that a browsing context is going away in the parent process?

I'd like to avoid creating JS actors for each browsing context here, as I expect the overhead to be significant for something that is relatively trivial.

Flags: needinfo?(nika)

Is it possible for browsing context not to show the tooltip when it is hidden instead? Or this operation is scheduled in future and forgotten?

(In reply to Revertron from comment #1)

Is it possible for browsing context not to show the tooltip when it is hidden instead? Or this operation is scheduled in future and forgotten?

I don't understand the question, but this likely isn't the right place. This is a technical bug for compatibility with fission (process-per-frame), which is turned off everywhere so likely not relevant to what you're seeing. Bug 1550637 covers user-visible issues with tooltips being mistakenly shown / not hidden at the right time.

I mean, that maybe you are trying to fix it on another layer, not on that where the problem rises.

We don't currently send out notifications visible to JS when this happens, but yes there are places where you can observe a BrowsingContext going away.

There are a couple of ways a BrowsingContext can become hidden:

  1. It is discarded. This happens when the BrowsingContext is completely trashed and won't be coming back. The parent process will have BrowsingContext::Detach called when this happens: https://searchfox.org/mozilla-central/rev/07f7390618692fa4f2a674a96b9b677df3a13450/docshell/base/BrowsingContext.cpp#292-326
  2. It is cached. This happens when a toplevel BrowsingContext enters the BFCache. Currently the easiest places to see this happen are in BrowsingContextGroup::CacheContext and CacheContexts: https://searchfox.org/mozilla-central/rev/07f7390618692fa4f2a674a96b9b677df3a13450/docshell/base/BrowsingContextGroup.cpp#88-97
  3. An ancestor is cached. When a BrowsingContext is an iframe, and its ancestor enters the BFCache, it won't have CacheContext called on it. You can walk up the tree of parents to figure out if any parents are being cached though.

If you want to add observer notifications to these methods, ask :farre to double-check it's a reasonable idea, and ask him for review too :-)

Flags: needinfo?(nika)
Priority: -- → P3

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Fission Milestone: --- → ?

Gijs, what are the steps to reproduce this bug? Is this bug about the <a> element's title attribute? title tooltips work for me with Fission.

Are tooltips totally broken, in the wrong position, or not cleared?

Tracking for Fission Nightly (M6)

Fission Milestone: ? → M6
Flags: needinfo?(gijskruitbosch+bugs)
Summary: [Fission] Make content link tooltip handling work correctly in fission → [Fission] Make content link tooltip handling work correctly in Fission

(In reply to Chris Peterson [:cpeterson] from comment #6)

Gijs, what are the steps to reproduce this bug? Is this bug about the <a> element's title attribute? title tooltips work for me with Fission.

Are tooltips totally broken, in the wrong position, or not cleared?

Tracking for Fission Nightly (M6)

This was filed in response to an IRC discussion with Nika and the comments in https://phabricator.services.mozilla.com/D35503 . I would have thought that there might be an issue with navigating a cross-process iframe and its tooltip not going away, but I can't reproduce such an issue off-hand (tested on macOS). Unfortunately, it's been 6 months and I don't really remember the full details. I also know a lot more about browsingcontexts now than 6 months ago so I'm struggling to follow my own comments...

In any case, it should be possible to improve the architecture here by using the browsing context tree, though I'm not sure whether there's a concrete problem here at this point - by rights, there should be, given https://searchfox.org/mozilla-central/rev/c61720a7d0c094d772059f9d6a7844eb7619f107/docshell/base/nsDocShellTreeOwner.cpp#1207,1221 - but I can't seem to reproduce one in practice. Sorry I'm not more help here.

Flags: needinfo?(gijskruitbosch+bugs)

Almost all this code is in docshell, so I'm going to move this, and make it block fission rather than fission-frontend.

Blocks: fission
No longer blocks: fission-frontend
Component: General → DOM: Navigation
Product: Firefox → Core

This bug is a Fission Nightly blocker (milestone M6b).

Fission Milestone: M6 → M6b
Attached file toplevel.html
Attached file toplevel2.html

Hovering over the links in https://bug1566422.bmoattachments.org/attachment.cgi?id=9166002 / https://bug1566422.bmoattachments.org/attachment.cgi?id=9166003 should show mispositioned title tooltips. It seems like we don't consider vertical/horizontal offsets applied to the frames when positioning their tooltips.

Assignee: nobody → kmadan
Status: NEW → ASSIGNED

We position the tooltip in ChromeTooltipListener::sTooltipCallback using both the mouse coordinates and the widget's screen offset. The offset is computed in PuppetWidget::WidgetToScreenOffset, which was updated in bug 1550800 to work with Fission. Reverting those changes fixes the bug in comment 13, but that's probably not the solution here.

Some logging shows that WidgetToTopLevelWidgetTransform().TransformPoint(LayoutDevicePoint()) is different depending on if we're dealing with an in-process vs out-of-process frame (it looks like for out-of-process frames, the offset is relative to the toplevel window?):

Fission

[Child 589102, Main Thread] WARNING: In PuppetWidget::WidgetToScreenOffset: file mozilla-central/widget/PuppetWidget.cpp, line 1135
[Child 589102, Main Thread] WARNING: RelativeToWindow = (502.000000, 84.000000): file mozilla-central/widget/PuppetWidget.cpp, line 1142
[Child 589102, Main Thread] WARNING: WindowPosition = (0, 64): file mozilla-central/widget/PuppetWidget.cpp, line 1143
[Child 589102, Main Thread] WARNING: mMouseScreenX = 550, mMouseScreenY = 164, screenDot.x = 502, screenDot.y = 148: file mozilla-central/docshell/base/nsDocShellTreeOwner.cpp, line 1300
[Child 589102, Main Thread] WARNING: ShowTooltip "TITLE" at (48, 16): file mozilla-central/docshell/base/nsDocShellTreeOwner.cpp, line 1155

Non-fission:

[Child 588817, Main Thread] WARNING: In PuppetWidget::WidgetToScreenOffset: file mozilla-central/widget/PuppetWidget.cpp, line 1135
[Child 588817, Main Thread] WARNING: RelativeToWindow = (0.000000, 74.000000): file mozilla-central/widget/PuppetWidget.cpp, line 1142
[Child 588817, Main Thread] WARNING: WindowPosition = (0, 64): file mozilla-central/widget/PuppetWidget.cpp, line 1143
[Child 588817, Main Thread] WARNING: mMouseScreenX = 548, mMouseScreenY = 168, screenDot.x = 0, screenDot.y = 138: file mozilla-central/docshell/base/nsDocShellTreeOwner.cpp, line 1300
[Child 588817, Main Thread] WARNING: ShowTooltip "TITLE" at (548, 30): file mozilla-central/docshell/base/nsDocShellTreeOwner.cpp, line 1155

Emilio, do you have an idea of what to do here?

Flags: needinfo?(emilio)

So the coordinates there all make sense to me so far:

  • The non-fission code is going to the top level content document and getting the screen offset, which for you is (0, 138).
  • But the fission screen offset is (502, 148), which is also fine.

So they're both computing proper widget-relative coordinates, which is what they're trying to compute. The issue is that that is a lie, because those coordinates go straight to xulBrowserWindow here, which ends up here.

So this all seems a bit silly, because the first thing showTooltip does is going back to screen coordinates here.

So I think the easiest thing to do is removing all the "find the nearest widget to compute widget-relative coordinates", and send the screen-relative coordinates to begin with, then avoid all these conversions.

Does that make sense? I could take this if needed, just ni? me back, though this should be relatively straight-forward if I'm right.

Flags: needinfo?(emilio)

So, basically, what that is trying to compute is browser-relative coordinates, but then they're turned into screen-relative ones. So we may as well just send the screen-relative ones, maybe.

Just to clarify: both the fission and the non-fission window were open in the (exact) same position on my screen. I hovered around the same area as well in both windows. Is there still expected to be a difference in the offsets?

Flags: needinfo?(emilio)

Yes, because the widget in the fission iframe is in the iframe, while in the non-fission one is in the tab.

Flags: needinfo?(emilio)

We currently start with screen-relative coordinates, translate them to
widget-relative coordinates, and then translate them back to screen-relative
coordinates when actually showing the tooltip in XULBrowserWindow.showTooltip().
There's no reason for the extra conversions, so we can just send screen-relative
coordinates directly.

Since the widget origin for out-of-process frames is the origin of the frame
itself (instead of the tab, which is the case for in-process frames), the
screen-to-widget conversion was incorrect, and was causing a bug in how the
tooltip was being positioned. Avoiding that conversion altogether also fixes
that bug.

Pushed by kmadan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b17a5aeded6
Avoid unnecessary coordinate conversions when showing tooltips, r=emilio
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Regressions: 1676020
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: