webkit_web_view_popup_menu_handler currently obtains selection start's coordinates by manually accessing selection start's renderer. But it should instead call SelectionController::localCaretRect.
The a11y code uses boundsForVisiblePositionRange to get the text extents, Ryosuke says that's likely wrong too. CCing Mario.
Created attachment 82842 [details] Patch
Ryosuke, Xan asked me to ping you for the review for this issue.
Comment on attachment 82842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82842&action=review > Source/WebKit/gtk/webkit/webkitwebview.cpp:387 > + if (!selection->start().node() || !selection->end().node() You must be using an old checkout. Position::node() no longer exists. What you want to do here is probably selection->selection()->isNonOrphanedCaretOrRange() since when start() or end() is orphaned, meaning that they're detached from the document, it doesn't make sense to query the firstRectForRange on that selection. > Source/WebKit/gtk/webkit/webkitwebview.cpp:397 > + IntRect firstRect = frame->editor()->firstRectForRange(selection->selection().firstRange().get()); firstRange() will return 0 if selection->isNone(). You probably need to check this condition above as well (fortunately, if isNonOrphanedCaretOrRange returns true, then isNone is false so the suggested change above will fix this as well). > Source/WebKit/gtk/webkit/webkitwebview.cpp:417 > - location = view->contentsToWindow(location) + IntSize(0, -1); > - if (location.y() < 0) > - location.setY(contextMenuMargin); > - else if (location.y() > view->height()) > - location.setY(view->height() - contextMenuMargin); > - if (location.x() < 0) > - location.setX(contextMenuMargin); > - else if (location.x() > view->width()) > - location.setX(view->width() - contextMenuMargin); > - IntPoint global(globalPointForClientPoint(gtk_widget_get_window(widget), location)); > - > - PlatformMouseEvent event(location, global, RightButton, MouseEventPressed, 0, false, false, false, false, gtk_get_current_event_time()); > + return FALSE; > > + // Never let the context menu abut very edge of the view. > + location = view->contentsToWindow(location); > + location.setY(max(gContextMenuMargin, location.y())); > + location.setY(min(view->height() - gContextMenuMargin, location.y())); > + location.setX(max(gContextMenuMargin, location.x())); > + location.setX(min(view->width() - gContextMenuMargin, location.x())); > + IntPoint globalPoint(globalPointForClientPoint(gtk_widget_get_window(widget), location)); > + PlatformMouseEvent event(location, globalPoint, RightButton, MouseEventPressed, 0, false, false, false, false, gtk_get_current_event_time()); I can't review these changes. Xan or someone else should do.
Created attachment 84426 [details] Patch with rniwa's suggestions
(In reply to comment #4) Thanks so much for the review! > (From update of attachment 82842 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82842&action=review > > > Source/WebKit/gtk/webkit/webkitwebview.cpp:387 > > + if (!selection->start().node() || !selection->end().node() > > You must be using an old checkout. Position::node() no longer exists. What you want to do here is probably selection->selection()->isNonOrphanedCaretOrRange() since when start() or end() is orphaned, meaning that they're detached from the document, it doesn't make sense to query the firstRectForRange on that selection. This patch was written before the change to ::node, I think. Made the fix you suggested here. > > > Source/WebKit/gtk/webkit/webkitwebview.cpp:397 > > + IntRect firstRect = frame->editor()->firstRectForRange(selection->selection().firstRange().get()); > > firstRange() will return 0 if selection->isNone(). You probably need to check this condition above as well (fortunately, if isNonOrphanedCaretOrRange returns true, then isNone is false so the suggested change above will fix this as well). Done!
Comment on attachment 84426 [details] Patch with rniwa's suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=84426&action=review Sorry for the delayed response. I didn't realize you had uploaded a new patch. The editing part of the change looks good to me. > Source/WebKit/gtk/webkit/webkitwebview.cpp:388 > + || (selection->selection().isCaret() && !selection->selection().isContentEditable())) { Don't you want to avoid doing this check when caret navigation is on?
Xan, can you review GTK part of it so that Martin can land this patch?
Comment on attachment 84426 [details] Patch with rniwa's suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=84426&action=review > Source/WebKit/gtk/webkit/webkitwebview.cpp:413 > + // Never let the context menu abut very edge of the view. I will change this to "Never let the context menu touch the very edge of the view."
Created attachment 86992 [details] Patch simplifying context menu margin and modifying comment
Comment on attachment 86992 [details] Patch simplifying context menu margin and modifying comment r=me
Comment on attachment 86992 [details] Patch simplifying context menu margin and modifying comment Clearing flags on attachment: 86992 Committed r82831: <http://trac.webkit.org/changeset/82831>
All reviewed patches have been landed. Closing bug.