Tooltip incorrectly appears when hovering over playing YouTube video |
|||||||||||
Issue descriptionREPRO STEPS: 1. Launch Chrome with or without --site-per-process flag 2. Navigate to https://www.khanacademy.org/computing/computer-science/cryptography/modern-crypt/v/diffie-hellman-key-exchange-part-1 3. Hover the mouse over the playing video. Observe whether a tooltip appears. 4. Start video playback (ignoring issue 808070 ) 5. Again hover the mouse over the playing video. Observe whether a tooltip appears (e.g. "Video: Public key cryptography: What is it?") EXPECTED BEHAVIOR: No tooltip at any point ACTUAL BEHAVIOR: - Without --site-per-process, there is no tooltip shown - With --site-per-process, a tooltip is shown in step 5 saying "Video: Public key cryptography: What is it?" NOTES: - I've reproed with a ToT Chromium build (at r529521)
,
Feb 7 2018
I am guessing that without OOPIFs, the FlatTreeTraversal::Parent in HitTestResult::Title (called from ChromeClient::SetToolTip) never goes over the iframe element. With OOPIFs, the parent RenderWidget gets a chance to process the mousemove and it processes the iframe's title. But... these are just unconfirmed guesses.
,
Feb 7 2018
,
Mar 6 2018
I was wondering if this bug should be considered ship blocking for Site Isolation (i.e. if we should add the Proj-SiteIsolation-LaunchBlocking label). On one hand, this bug is not blocking any functionality - users can still play videos. On the other hand, the tooltip appears even after entering fullscreen and it is rather annoying to see the tooltip while watching the video.-a FWIW, I've checked that the bug still repros with 66.0.3355.0.
,
Mar 29 2018
,
Mar 30 2018
I can repro in Windows 67.0.3383.0. I don't think it's launch blocking for Site Isolation, but I do think we should try to get it fixed in M67 if we can. James or Ken, can you help us find an owner for this, since it seems to involve input events?
,
Mar 30 2018
Also, James points out that this sounds similar to issue 815119 for PDFs, but he thinks that one is probably GuestView specific.
,
Apr 2 2018
Assigning to wjmaclean@ to help find an owner. This isn't launch blocking, but we should still try to fix in M67 if possible. Thanks!
,
Apr 9 2018
Ok, I had no idea who to give this to, so I just poked at it myself, and here's what I've found. The display of the iframe 'title' as the tooltip text does in fact occur for non-oopif iframes, but it's racey. A simple example: navigate to about:blank, and then using Inspector, add <iframe title="Fred"></iframe> inside the <body></body> tags. If you continually move the mouse in and out of the iframe, you will occasionally see "Fred" pop up as a tooltip. But more often than not, after the IPC is sent to make "Fred" the tooltip, the mouseenter() on the iframe's body goes through the same process, and sends and empty tooltip, which overrides the iframe's title. In an OOPIF, this doesn't happen, since the iframe's content is remote, and seems not to get the tooltip (or perhaps it does send an empty tooltip, but to a different RenderWidgetHost, and thus does not override the first one sent). In any case, it is easy enough in ChromeClient to detect if we hit an iframe with a non-empty tooltip, and if it's a remote frame just early out ... but that is technically a change in behaviour. I'll keep looking a bit more to see what's happening to the IPCs on the browser side.
,
Apr 9 2018
After a quick look on the browser side, it looks like everything there should work fine, but since the mouse event routes to only one renderer (presumably), we cannot recreate the same race condition in the OOPIF that occurs in the non-OOPIF case.
,
Apr 9 2018
Another couple of thoughts: 1) My perception of 'raciness' might be due to the fact that the empty iframe I was tesing with had a small border region, in which the iframe tooltip would be displayed, and then as the mouse moves fully into the iframe it gets replaced by the body's (or some other element's) tooltip text. 2) RenderWidgetHostInputEventRouter should be generating MouseEnter/Leave events for the OOPIF, but there's no longer any timing guarantee between when the mainframe and the oopif will respond, ... so the wrong result can presumably occur. Making these events somehow synchronous could help, but this might be onerous.
,
Apr 10 2018
A few observations from my side: I do see the tooltip in the about:blank (same process) case when hovering over the border of the iframe. I don't see it ever show up when the mouse goes further over the iframe itself, so yes, it might not be a race. The OOPIF case is seeming pretty deterministic to me if I add a title to the frame on http://csreis.github.io/tests/cross-site-iframe.html and click "Go cross-site (simple page)". It's possible there's a race between the main frame process and subframe process to set the tooltip, but if so I'm never seeing the subframe process win. It does seem like we should have a way for the subframe process's (empty) tooltip to win, though hopefully without a synchronous event. Is this something where the frame that wins the hit test should always win, regardless of what order they show up in? (No idea, just throwing out suggestions.)
,
Apr 10 2018
Ok, here's what I know now: 1) In many cases the child doesn't even *attempt* to send the empty tooltip, on account of: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/page/chrome_client_impl.h?l=248&ct=xref_jump_to_def&gsn=did_request_non_empty_tool_tip_ The child will *only* send an empty tooltip if it has previously sent an empty one. This fails if the mouse leaves the child frame, then re-enters, since it may in the meantime display a tooltip from the parent, and then the child won't send an empty one to clear it. *** We need a tri-state here: last_sent_was_empty, last_sent_was_non_empty, and last_sent_indeterminate. On MouseEnter the state should be set to last_sent_indeterminate, which instructs the child to always send the next tooltip text. 2) Regardless, we still need the browser to disregard tooltips from the parent since the ordering of tooltip messages is asynchronous when crossing the boundary, and so the tool-tip manager should only display whatever tooltip was requested by the view the mouse is currently over. I don't know if this is sufficient, but it certainly is necessary.
,
Apr 16 2018
I have a CL for this, just debugging a test to go with it.
,
Apr 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3fef335bdba3410b39b2d50a843e372e42bf01b7 commit 3fef335bdba3410b39b2d50a843e372e42bf01b7 Author: W. James MacLean <wjmaclean@chromium.org> Date: Fri Apr 20 06:31:32 2018 Improve tool-tip handling for OOPIFs. This CL changes to how tooltips are handled for OOPIFs. It uses CursorManager in the browser to only set the tooltip if it is received from a view that currently is controlling the mouse cursor. This prevents an iframe element in a parent renderer from overriding the tooltip sent by the child's renderer, keeping in mind this can happen primarily due to races in the responses to the last MouseMove in the parent and the first MouseMove in the child. Bug: 808074 Change-Id: I1bede992a1e8daa8025f3f2e392d721dbe701e95 Reviewed-on: https://chromium-review.googlesource.com/1012268 Commit-Queue: James MacLean <wjmaclean@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Ken Buchanan <kenrb@chromium.org> Cr-Commit-Position: refs/heads/master@{#552276} [modify] https://crrev.com/3fef335bdba3410b39b2d50a843e372e42bf01b7/content/browser/frame_host/render_widget_host_view_guest.cc [modify] https://crrev.com/3fef335bdba3410b39b2d50a843e372e42bf01b7/content/browser/renderer_host/cursor_manager.cc [modify] https://crrev.com/3fef335bdba3410b39b2d50a843e372e42bf01b7/content/browser/renderer_host/cursor_manager.h [modify] https://crrev.com/3fef335bdba3410b39b2d50a843e372e42bf01b7/content/browser/renderer_host/render_widget_host_view_aura.cc [modify] https://crrev.com/3fef335bdba3410b39b2d50a843e372e42bf01b7/content/browser/renderer_host/render_widget_host_view_aura.h [modify] https://crrev.com/3fef335bdba3410b39b2d50a843e372e42bf01b7/content/browser/renderer_host/render_widget_host_view_base.h [modify] https://crrev.com/3fef335bdba3410b39b2d50a843e372e42bf01b7/content/browser/renderer_host/render_widget_host_view_child_frame.cc [modify] https://crrev.com/3fef335bdba3410b39b2d50a843e372e42bf01b7/content/browser/renderer_host/render_widget_host_view_mac.h [modify] https://crrev.com/3fef335bdba3410b39b2d50a843e372e42bf01b7/content/browser/renderer_host/render_widget_host_view_mac.mm [modify] https://crrev.com/3fef335bdba3410b39b2d50a843e372e42bf01b7/content/browser/site_per_process_hit_test_browsertest.cc [modify] https://crrev.com/3fef335bdba3410b39b2d50a843e372e42bf01b7/testing/buildbot/filters/viz.content_browsertests.filter
,
Apr 20 2018
This should now be fixed. Please re-open if needed.
,
Apr 20 2018
,
Apr 20 2018
,
Apr 21 2018
Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 21 2018
Pls merge your change to M67 branch 3396 by 1:00 PM PT, Monday (04/23) so we can pick it up next M67 Dev/Beta release. Thank you.
,
Apr 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c81c7495773442be05fac4b1276b4af5771432bc commit c81c7495773442be05fac4b1276b4af5771432bc Author: W. James MacLean <wjmaclean@chromium.org> Date: Mon Apr 23 15:28:32 2018 Improve tool-tip handling for OOPIFs. This CL changes to how tooltips are handled for OOPIFs. It uses CursorManager in the browser to only set the tooltip if it is received from a view that currently is controlling the mouse cursor. This prevents an iframe element in a parent renderer from overriding the tooltip sent by the child's renderer, keeping in mind this can happen primarily due to races in the responses to the last MouseMove in the parent and the first MouseMove in the child. TBR=wjmaclean@chromium.org (cherry picked from commit 3fef335bdba3410b39b2d50a843e372e42bf01b7) Bug: 808074 Change-Id: I1bede992a1e8daa8025f3f2e392d721dbe701e95 Reviewed-on: https://chromium-review.googlesource.com/1012268 Commit-Queue: James MacLean <wjmaclean@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Ken Buchanan <kenrb@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#552276} Reviewed-on: https://chromium-review.googlesource.com/1023382 Reviewed-by: James MacLean <wjmaclean@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#212} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/c81c7495773442be05fac4b1276b4af5771432bc/content/browser/frame_host/render_widget_host_view_guest.cc [modify] https://crrev.com/c81c7495773442be05fac4b1276b4af5771432bc/content/browser/renderer_host/cursor_manager.cc [modify] https://crrev.com/c81c7495773442be05fac4b1276b4af5771432bc/content/browser/renderer_host/cursor_manager.h [modify] https://crrev.com/c81c7495773442be05fac4b1276b4af5771432bc/content/browser/renderer_host/render_widget_host_view_aura.cc [modify] https://crrev.com/c81c7495773442be05fac4b1276b4af5771432bc/content/browser/renderer_host/render_widget_host_view_aura.h [modify] https://crrev.com/c81c7495773442be05fac4b1276b4af5771432bc/content/browser/renderer_host/render_widget_host_view_base.h [modify] https://crrev.com/c81c7495773442be05fac4b1276b4af5771432bc/content/browser/renderer_host/render_widget_host_view_child_frame.cc [modify] https://crrev.com/c81c7495773442be05fac4b1276b4af5771432bc/content/browser/renderer_host/render_widget_host_view_mac.h [modify] https://crrev.com/c81c7495773442be05fac4b1276b4af5771432bc/content/browser/renderer_host/render_widget_host_view_mac.mm [modify] https://crrev.com/c81c7495773442be05fac4b1276b4af5771432bc/content/browser/site_per_process_hit_test_browsertest.cc [modify] https://crrev.com/c81c7495773442be05fac4b1276b4af5771432bc/testing/buildbot/filters/viz.content_browsertests.filter |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by lukasza@chromium.org
, Feb 7 2018