Bug 134684 - ServicesOverlayController menus show up in the wrong place
Summary: ServicesOverlayController menus show up in the wrong place
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brady Eidson
Keywords: InRadar
Depends on: 134783
  Show dependency treegraph
Reported: 2014-07-07 10:21 PDT by Brady Eidson
Modified: 2014-07-09 17:09 PDT (History)
3 users (show)

See Also:

Patch v1 (3.51 KB, patch)
2014-07-07 10:25 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v2 (3.41 KB, patch)
2014-07-07 11:40 PDT, Brady Eidson
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2014-07-07 10:21:12 PDT
ServicesOverlayController menus show up in the wrong place.

This is despite the fact that they use the context menu mechanism which does display context menus in the right place.

The difference is that context menu clicks undergo a hit test and have their coordinate translated to window-space.

We need to do the same for Services.

Comment 1 Brady Eidson 2014-07-07 10:25:25 PDT
Created attachment 234496 [details]
Patch v1
Comment 2 Tim Horton 2014-07-07 11:04:36 PDT
Comment on attachment 234496 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=234496&action=review

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:434
> +void ServicesOverlayController::handleClick(const WebCore::IntPoint& clickPoint)

This seems wrong. The incoming point is always in a consistent coordinate space (though I'm not sure at the moment which one), not dependent on what frame you click on. Am I wrong and this somehow actually works in subframes?
Comment 3 Brady Eidson 2014-07-07 11:31:46 PDT
Discussed with Tim in person - This patch wasn't *wrong*, it was just more work than was needed.

Simpler patch coming up, that also includes a fix for a subframe crasher that showed up in testing subframes.
Comment 4 Brady Eidson 2014-07-07 11:40:09 PDT
Created attachment 234499 [details]
Patch v2

Confirmed with various main frame scroll positions, subframe scroll positions, and page zoom situations, that this patch gives the same coordinates as the hit test patch.
Comment 5 Tim Horton 2014-07-07 11:41:50 PDT
Comment on attachment 234499 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=234499&action=review

> Source/WebKit2/WebProcess/WebPage/mac/ServicesOverlayController.mm:435
> +    Page* page = m_webPage->corePage();

Please use WebPage::mainFrameView() instead of all of this.
Comment 6 Brady Eidson 2014-07-07 11:51:28 PDT
Comment 7 WebKit Commit Bot 2014-07-09 16:57:27 PDT
Re-opened since this is blocked by bug 134783
Comment 8 Brady Eidson 2014-07-09 17:09:44 PDT
(In reply to comment #7)
> Re-opened since this is blocked by bug 134783

That was just a test of webkitbot.  All is well.