Bug 40983 – [Qt] - Context menu needs "Copy" as well when the selected text is a link.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Karsten Heimrich]

Description from the original issue:

If a selection contains a link, the context menu contains, "Open Link, Open Link in New Tab, and Copy Link" If the selection does not contain a link, I get, "Copy". 100% of the time, when I have a link selected, I'm just trying to copy a prototype or example to a source file. The link menus do absolutely nothing but waste my time. I just get i.e "qthelp://com.trolltech.qt.460/qdoc/qwidget.html#focusPreviousChild". Add a "copy" action to the first element of the context menu so that I can just copy the text and not the link. Maybe the link information is useful to someone, but I doubt it is useful to any developers using Assistant.

[Karsten Heimrich]

http://bugreports.qt.nokia.com/browse/QTBUG-11478

[Benjamin Poulain]

Please follow http://trac.webkit.org/wiki/QtWebKitBugs when reporing bugs (missing Qt keyword).

[Aparna Nandyal]

Created attachment 81202 [details]
Patch for review

Modified to have Copy as a default option in context menu when the selection is a url.

[Aparna Nandyal]

Created attachment 81203 [details]
Patch for review with changelogs

Adding Copy to context menu

[Benjamin Poulain]

Comment on attachment 81203 [details]
Patch for review with changelogs

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

Shouldn't this code be part of ContextMenu.cpp in WebCore?

> Source/WebKit/qt/ChangeLog:11
> +        selection is a url. Adding this option to QWebPage 

Dot at the end of the sentence.

[Benjamin Poulain]

Comment on attachment 81203 [details]
Patch for review with changelogs

r-
Unless there is a good reason, this kind of code should be in a place where other ports can use it. It can be #ifdef for Qt for now.

[Aparna Nandyal]

Created attachment 81251 [details]
Patch with corrected code

Added the fix in underlying layer.

[Benjamin Poulain]

Comment on attachment 81251 [details]
Patch with corrected code

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

Almost there I think.

> Source/WebCore/ChangeLog:5
> +        Copy menu option to be shown in context menu

?

> Source/WebCore/ChangeLog:11
> +        a link. Added a test in tst_qwebpage.cpp

Dot at the end of the sentence or Kenneth will kill you. ;)

> Source/WebCore/page/ContextMenuController.cpp:714
> +#if PLATFORM(QT)
> +            if (m_hitTestResult.isSelected()) 
> +                appendItem(CopyItem, m_contextMenu.get());
> +#endif

This seems reasonable. I did not remember ContextMenuController was so messy. :)

> Source/WebKit/qt/ChangeLog:12
> +        selects a link and opens context menu 

<Kenneth>Missing dot.</Kenneth> :)

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2770
> +    QContextMenuEvent event(QContextMenuEvent::Mouse, QPoint(71, 15));
> +    view.page()->swallowContextMenuEvent(&event);
> +    view.page()->updatePositionDependentActions(QPoint(71, 15));

I don't like the hard coded position. That can make the test flaky depending on other conditions.
Why not used QWebElement's rect to find the center of the link?

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2777
> +    foreach (QObject* child, view.children()) {
> +        contextMenu = qobject_cast<QMenu*>(child);
> +        if (contextMenu)
> +            break;
> +    }

Can't you use view.findChildren<QMenu*>() here?

[Aparna Nandyal]

Created attachment 81492 [details]
Patch for review

Added full stop in ChangeLog and edited test file.

[Andreas Kling]

Comment on attachment 81492 [details]
Patch for review

r=me :)

[WebKit Commit Bot]

Comment on attachment 81492 [details]
Patch for review

Clearing flags on attachment: 81492

Committed r77906: <http://trac.webkit.org/changeset/77906>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.