Bug 96593 – [Chromium] REGRESSION(r127457): Context menu on textarea is displayed in the wrong place

• Component: UI Events
• Status: RESOLVED FIXED

[Julien Chaffraix]

Created attachment 163765 [details]
Screenshot of the issue

How to reproduce:

* Open http://www.google.com/transliterate on Chromium on Linux or Windows.
* Right click on the textarea, and check the context menu.

Issue:

The context menu doesn't come up next to the mouse cursor. Please see attached image. 

Rolling out r127457 (and the following refactoring) locally fixes the issue. Not sure if other platforms are impacted, for some reason Safari and Chromium Mac don't show the issue.

Allan, could you have a look?

[Allan Sandfeld Jensen]

I can not reproduce the issue on Qt either.

I have a guess on the cause though. EventHandler::hitTestResultAtPoint used to have a quirk/bug where it changed the result.point() so that after the function it was in the coordinates of the last frame it hit-tested. It no longer does that, if you use that position for the context menu have code that corrects for the old bug, it might be wrong now that it is no longer necessary.

[Allan Sandfeld Jensen]

(In reply to comment #1)
> I can not reproduce the issue on Qt either.
> 
> I have a guess on the cause though. EventHandler::hitTestResultAtPoint used to have a quirk/bug where it changed the result.point() so that after the function it was in the coordinates of the last frame it hit-tested. It no longer does that, if you use that position for the context menu have code that corrects for the old bug, it might be wrong now that it is no longer necessary.

No, I have checked the chromium code now, and the only place that uses the returned position hitTestResultAtPoint is WebFrameImpl::characterIndexForPoint, and that one should work more correctly now.

[Allan Sandfeld Jensen]

Possibly related to bug #96541

[Julien Chaffraix]

(In reply to comment #3)
> Possibly related to bug #96541

Tried the fix locally but it didn't solve the issue AFAICT.

(In reply to comment #2)
> (In reply to comment #1)
> > I can not reproduce the issue on Qt either.
> > 
> > I have a guess on the cause though. EventHandler::hitTestResultAtPoint used to have a quirk/bug where it changed the result.point() so that after the function it was in the coordinates of the last frame it hit-tested. It no longer does that, if you use that position for the context menu have code that corrects for the old bug, it might be wrong now that it is no longer necessary.

Toying with the example, this is exactly the issue. We seem to be converting the result of hitTestResultAtPoint to the window coordinates, therefore adding the iframe's position twice to the context menu's top left point.

> No, I have checked the chromium code now, and the only place that uses the returned position hitTestResultAtPoint is WebFrameImpl::characterIndexForPoint, and that one should work more correctly now.

After your change, anything that manipulates the returned position from hitTestResultAtPoint across frames can be wrong. I am not super happy that this was completely overlooked. Debugging the issue, ContextMenuController is doing exactly that and would be my bet for what is wrong.

[Allan Sandfeld Jensen]

(In reply to comment #4)
> (In reply to comment #3)
> > Possibly related to bug #96541
> 
> Tried the fix locally but it didn't solve the issue AFAICT.
> 
> (In reply to comment #2)
> > (In reply to comment #1)
> > > I can not reproduce the issue on Qt either.
> > > 
> > > I have a guess on the cause though. EventHandler::hitTestResultAtPoint used to have a quirk/bug where it changed the result.point() so that after the function it was in the coordinates of the last frame it hit-tested. It no longer does that, if you use that position for the context menu have code that corrects for the old bug, it might be wrong now that it is no longer necessary.
> 
> Toying with the example, this is exactly the issue. We seem to be converting the result of hitTestResultAtPoint to the window coordinates, therefore adding the iframe's position twice to the context menu's top left point.
> 
Okay, at least that is better than not knowing the source.

> > No, I have checked the chromium code now, and the only place that uses the returned position hitTestResultAtPoint is WebFrameImpl::characterIndexForPoint, and that one should work more correctly now.
> 
> After your change, anything that manipulates the returned position from hitTestResultAtPoint across frames can be wrong. I am not super happy that this was completely overlooked. Debugging the issue, ContextMenuController is doing exactly that and would be my bet for what is wrong.

It wasn't overlooked, I checked all the call sites I could find of the function, and in the places I found that used the position it would either not make a difference, or would actually fix rare corner cases. I will take a deeper look at the contextMenuController. In general though the point in hitTestResult should probably be deprecated, it is just a left-over from when HitTestPoint and HitTestResult were the same.

[Julien Chaffraix]

Created attachment 163909 [details]
Proposed fix. Tested on Chromium Linux, need testing on Chromium Mac. Would love to test that.

[Julien Chaffraix]

Created attachment 163920 [details]
Proposed fix: Same as previously, ChangeLog updated, passes Mac testing.

[Julien Chaffraix]

CC'ing some people who have reviewed this code in the past.

[Adam Barth]

Comment on attachment 163920 [details]
Proposed fix: Same as previously, ChangeLog updated, passes Mac testing.

ok

[Julien Chaffraix]

Comment on attachment 163920 [details]
Proposed fix: Same as previously, ChangeLog updated, passes Mac testing.

Actually need to update the bug title.

[Julien Chaffraix]

Created attachment 163950 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 163950 [details]
Patch for landing

Clearing flags on attachment: 163950

Committed r128505: <http://trac.webkit.org/changeset/128505>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

I believe from reading the source code that the webkit1 win-platform (I guess Safari on Windows?), have the same issue. It would be great if someone could confirm it, and we could fix it.

[Jessie Berlin]

http://trac.webkit.org/changeset/127457 also broke Apple Mac context menus, not just Chromium context menus.

The fix in http://trac.webkit.org/changeset/128505 only fixed Chromium.

[Julien Chaffraix]

(In reply to comment #15)
> http://trac.webkit.org/changeset/127457 also broke Apple Mac context menus, not just Chromium context menus.
> 
> The fix in http://trac.webkit.org/changeset/128505 only fixed Chromium.

Yes, my apologies for just fixing one platform without looking at the others. For the record, the fix in this bug will be rolled out along r127457 in bug 96830 as the approach has caused (at least) another regression.