Bug 54633 – [Gtk] webkit_web_view_popup_menu_handler should call SelectionController::localCaretRect

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

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.

[Xan Lopez]

The a11y code uses boundsForVisiblePositionRange to get the text extents, Ryosuke says that's likely wrong too. CCing Mario.

[Martin Robinson]

Created attachment 82842 [details]
Patch

[Martin Robinson]

Ryosuke, Xan asked me to ping you for the review for this issue.

[Ryosuke Niwa]

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.

[Martin Robinson]

Created attachment 84426 [details]
Patch with rniwa's suggestions

[Martin Robinson]

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

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

Xan, can you review GTK part of it so that Martin can land this patch?

[Martin Robinson]

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

[Martin Robinson]

Created attachment 86992 [details]
Patch simplifying context menu margin and modifying comment

[Xan Lopez]

Comment on attachment 86992 [details]
Patch simplifying context menu margin and modifying comment

r=me

[Martin Robinson]

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>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.