enum QWebPage::WebAction doesn't have value QWebPage::CopyImageUrl but obviously this is very frequent action performed by users. By now I workaround this gap, but I think that such notorious action should by supported by default in standard way like other actions which can be performed on the web page.
Please follow http://trac.webkit.org/wiki/QtWebKitBugs when reporing bug here. That is an easy patch to make if you want to add it yourself to WebKit ( https://trac.webkit.org/wiki/QtWebKitContrib )
Created attachment 80667 [details] patch Introduce QWebPage::CopyImageUrlToClipboard into QWebPage:WebAction.
Comment on attachment 80667 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=80667&action=review > Source/WebKit/qt/ChangeLog:8 > + Add QWebPage::CopyImageUrlToClipborad into QWebPage::WebAction. Typo, CopyImageUrlToClipboard. > Source/WebKit/qt/Api/qwebpage.cpp:1684 > + \value CopyImageUrlToClipboard Copy the highlighted image's url to the clipboard. url -> URL > Tools/ChangeLog:8 > + Add CopyImageUrlToClipborad into QtTestbrowser. Typo, CopyImageUrlToClipboard. > Tools/QtTestBrowser/webview.cpp:247 > + QAction* newTabAction = menu->addAction("Copy Image Address", webPage, SLOT(copyImageUrlToClipboard())); The 'newTabAction' variable name doesn't make sense here. > Tools/QtTestBrowser/webview.cpp:248 > + menu->insertAction(menu->actions().at(4), newTabAction); What's up with the at(4) here? How is the QMenu action list populated?
Created attachment 80756 [details] patch2 Based on comments to make some changes. For the last issue "at(4)", because we already have another 3 menu items for image, they are "Open Image, Save Image, and Copy Image", and I want to put "Copy Image Address" after them, that' why. By the way, the other 3 actions come from WebKit common code, currently "Copy Image Address" is only for Qt, so we have to manually add into ContextMenu like "Open in Default Browser" did.
Food for thought: should that be added to the default actions of the context menu?
> For the last issue "at(4)", because we already have another 3 menu items for image, they are "Open Image, Save Image, and Copy Image", and I want to put "Copy Image Address" after them, that' why. By the way, the other 3 actions come from WebKit common code, currently "Copy Image Address" is only for Qt, so we have to manually add into ContextMenu like "Open in Default Browser" did. It's always good to avoid hardcoded numbers. Can't we do anything like append or get the length of current items, etc.?
Let's make it more clear, currently, the ContextMenu like: Open Image Save Image Copy Image ---------- (seperator) Inspector I want to insert the new menu item after "Copy Image", because I think it should be group with "Open...". So, you see it is difficult to use "append" or "length" to find the position. :( Or, like Benjamin said put it into default actions. But I think then it will go into all the platform. Some platform, like Safari already have this, I think they suppose to add it manually like what I am doing now. If I put it into default, it maybe broke their behavior.
good explanation. :)
(In reply to comment #7) > Or, like Benjamin said put it into default actions. But I think then it will go into all the platform. Some platform, like Safari already have this, I think they suppose to add it manually like what I am doing now. If I put it into default, it maybe broke their behavior. Someone already changed the behavior of the menu for this release. There is now "select all" for input fields. If we change the menu, it is best to do it all at once and communicate about the changes (on the Qt blog for example). I have CCed Andrea, the lead of the Rekonq project. Maybe he will have an opinion on the question. The default menu is bad enough that hybrid and mobile redefine it anyway ;)
Comment on attachment 80756 [details] patch2 You should not have to create the action yourself like you do in QTestBrowser. QWebPage::action() should give you the action. You forgot to implement that part, that is why you need that code in QtTestBrowser.
(In reply to comment #10) > (From update of attachment 80756 [details]) > You should not have to create the action yourself like you do in QTestBrowser. > QWebPage::action() should give you the action. You forgot to implement that part, that is why you need that code in QtTestBrowser. And an autotest would be nice... :)
(In reply to comment #10) > (From update of attachment 80756 [details]) > You should not have to create the action yourself like you do in QTestBrowser. > QWebPage::action() should give you the action. You forgot to implement that part, that is why you need that code in QtTestBrowser. I believe QWebPage::action() only for customize the display string. What items should be display by default is decided by WebCore/page/ContextMenuController.cpp/populate(), if we put this action into default (send it into common code), then we don't need manually add it into menu.
(In reply to comment #12) > (In reply to comment #10) > > (From update of attachment 80756 [details] [details]) > > You should not have to create the action yourself like you do in QTestBrowser. > > QWebPage::action() should give you the action. You forgot to implement that part, that is why you need that code in QtTestBrowser. > > I believe QWebPage::action() only for customize the display string. No it is not, it is the way standard actions are created. As a user, you should not have to do the connect from the action to actionTriggered. Please check the code.....
Created attachment 80794 [details] patch3 Make some change based on comments, but I don't know how to create a auto test case (#11), do you have any example for it? Benjamin.
Comment on attachment 80794 [details] patch3 View in context: https://bugs.webkit.org/attachment.cgi?id=80794&action=review > Source/WebKit/qt/Api/qwebpage.cpp:2740 > + case CopyImageUrlToClipboard: > + text = tr("Copy Image Address"); > + break; This should be done through the platform strategy. We'll need a contextMenuItemTagCopyImageURLToClipboard(), though it can be #if PLATFORM(QT) specific.
Created attachment 81059 [details] patch4 Put this action into standard action but Qt specific.
Attachment 81059 [details] did not build on qt: Build output: http://queues.webkit.org/results/7693321
Comment on attachment 81059 [details] patch4 Like EWS says, this breaks WebKit2. And speaking of WebKit2, the CopyImageUrlToClipboard action should also be added to the QWKPage WebActions.
Created attachment 81090 [details] patch5 Weird, I didn't get any build error in my workspace. Anyway, I changed webkit2 code and make a clean build, it works fine.
Attachment 81090 [details] did not build on qt: Build output: http://queues.webkit.org/results/7691541
Created attachment 81211 [details] patch6 fix webkit2 build issue.
Comment on attachment 81211 [details] patch6 r=me
Comment on attachment 81211 [details] patch6 Clearing flags on attachment: 81211 Committed r77892: <http://trac.webkit.org/changeset/77892>
All reviewed patches have been landed. Closing bug.