Bug 96593

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.

(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.

Possibly related to bug #96541

(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.

(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.

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

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

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

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

Comment on attachment 163920 [details] Proposed fix: Same as previously, ChangeLog updated, passes Mac testing. Actually need to update the bug title.

Created attachment 163950 [details] Patch for landing

Comment on attachment 163950 [details] Patch for landing Clearing flags on attachment: 163950 Committed r128505: <http://trac.webkit.org/changeset/128505>

All reviewed patches have been landed. Closing bug.

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.

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.

(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.