Bug 28696 - Middle click doesn't trigger onpaste event in Chromium
Summary: Middle click doesn't trigger onpaste event in Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Steve VanDeBogart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-24 17:14 PDT by Steve VanDeBogart
Modified: 2009-09-08 10:58 PDT (History)
4 users (show)

See Also:


Attachments
Handle middle click in Chromium like QT (5.39 KB, patch)
2009-08-24 17:16 PDT, Steve VanDeBogart
eric: review-
Details | Formatted Diff | Diff
Handle middle click in Chromium like QT (try 2) (6.41 KB, patch)
2009-09-01 16:50 PDT, Steve VanDeBogart
no flags Details | Formatted Diff | Diff
Layout test to test that middle click triggers the onpaste event (2.30 KB, patch)
2009-09-01 16:51 PDT, Steve VanDeBogart
no flags Details | Formatted Diff | Diff
Handle middle click in Chromium like QT (including updated layout test) (10.57 KB, patch)
2009-09-02 13:03 PDT, Steve VanDeBogart
fishd: review-
eric: commit-queue-
Details | Formatted Diff | Diff
Handle middle click in Chromium like QT (10.45 KB, patch)
2009-09-03 15:26 PDT, Steve VanDeBogart
no flags Details | Formatted Diff | Diff
Handle middle click in Chromium like QT (10.45 KB, patch)
2009-09-03 16:02 PDT, Steve VanDeBogart
fishd: review+
pkasting: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve VanDeBogart 2009-08-24 17:14:10 PDT
Chromium doesn't treat middle click on Linux as a paste event. (http://code.google.com/p/chromium/issues/detail?id=18792)  The attached patch is part of an attempt to fix this.  The remaining parts are at (http://codereview.chromium.org/174367)
Comment 1 Steve VanDeBogart 2009-08-24 17:16:46 PDT
Created attachment 38515 [details]
Handle middle click in Chromium like QT
Comment 2 Eric Seidel (no email) 2009-08-31 03:46:14 PDT
Comment on attachment 38515 [details]
Handle middle click in Chromium like QT

This needs a layout test.

Also, funny spacing in the ChangeLog:
+        Handle middle click in Chromium like QT
+
+        https://bugs.webkit.org/show_bug.cgi?id=28696

Seems wrong:
+#if PLATFORM(QT) || PLATFORM(CHROMIUM)
     bool isSelectionMode() const;
     void setSelectionMode(bool selectionMode);
 #endif

You only want this on Chromium Linux, or?

Enums are almost always clearer than bools:
+        static bool clipboardIsFormatAvailable(PasteboardPrivate::ClipboardFormat, bool);
Comment 3 Steve VanDeBogart 2009-09-01 16:50:24 PDT
Created attachment 38897 [details]
Handle middle click in Chromium like QT (try 2)
Comment 4 Steve VanDeBogart 2009-09-01 16:51:08 PDT
Created attachment 38898 [details]
Layout test to test that middle click triggers the onpaste event
Comment 5 Steve VanDeBogart 2009-09-01 16:53:31 PDT
Address Eric's comments.  (missed the obsoletes box for the WebCore patch)
Comment 6 Eric Seidel (no email) 2009-09-02 02:53:00 PDT
The patch seems fine.  I generally try to write new layout test as "js tests" aka" DOM only tests" our documentation for such is rather poor though:
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

That would save you some typing, as you get things like "debug" and "testPassed" and "shouldBe" functions all for free.

Why does this test use edting.js?  Seems it doesn't actually want to use any of that framework, no?

The test and change should ideally be in one patch so they can be landed together by the bot.
Comment 7 Steve VanDeBogart 2009-09-02 13:03:24 PDT
Created attachment 38938 [details]
Handle middle click in Chromium like QT (including updated layout test)

Change the test to a "DOM only test" and merge the patches to one file.
Comment 8 Eric Seidel (no email) 2009-09-02 13:46:02 PDT
Comment on attachment 38938 [details]
Handle middle click in Chromium like QT (including updated layout test)

Looks fine.

You coudl have written:
 23     shouldBe("1", stringify(pasteCount));

as shouldBe("pasteCount", "1");

shouldBe evals each half.
Comment 9 Eric Seidel (no email) 2009-09-02 13:51:15 PDT
Comment on attachment 38938 [details]
Handle middle click in Chromium like QT (including updated layout test)

Rejecting patch 38938 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=38938 from bug 28696 failed to download and apply.
Comment 10 Eric Seidel (no email) 2009-09-02 14:33:50 PDT
Comment on attachment 38938 [details]
Handle middle click in Chromium like QT (including updated layout test)

Patch looks malformed:
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html
|===================================================================
|--- LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html	(revision 0)
|+++ LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html	(working copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
patch -p0 "LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html" returned 1.  Pass --force to ignore patch failures.

Index: LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html
===================================================================
--- LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html	(revision 47870)	(from ../../../../../../../http:/svn.webkit.org/repository/webkit/trunk/LayoutTests/traversal/resources/TEMPLATE.html:47870)
+++ LayoutTests/platform/gtk/editing/pasteboard/resources/TEMPLATE.html	(working copy)
Comment 11 Darin Fisher (:fishd, Google) 2009-09-02 14:59:49 PDT
Comment on attachment 38938 [details]
Handle middle click in Chromium like QT (including updated layout test)

> Index: WebCore/platform/chromium/ChromiumBridge.h
...
> +        static String clipboardReadPlainText(PasteboardPrivate::TargetClipboard);
>          static void clipboardReadHTML(String*, KURL*);

So, I guess it is implied that the target is the system clipboard for all
of the methods that do not have a TargetClipboard parameter?  It might be
good to write a comment saying that.


> Index: WebCore/platform/chromium/ClipboardChromium.cpp
...
>  #include "HTMLNames.h"
>  #include "NamedAttrMap.h"
>  #include "MIMETypeRegistry.h"
> +#include "Pasteboard.h"
>  #include "markup.h"
>  #include "NamedNodeMap.h"
>  #include "PlatformString.h"

nit: includes should be in alphabetical order


> Index: WebCore/platform/chromium/PasteboardChromium.cpp
...
> +bool Pasteboard::isSelectionMode() const
> +{
> +      return m_selectionMode;

nit: indentation should be 4 white spaces


> +void Pasteboard::setSelectionMode(bool selectionMode)
> +{
> +      m_selectionMode = selectionMode;

ditto


>  PassRefPtr<DocumentFragment> Pasteboard::documentFragment(Frame* frame, PassRefPtr<Range> context, bool allowPlainText, bool& chosePlainText)
>  {
>      chosePlainText = false;
> +    PasteboardPrivate::TargetClipboard target = m_selectionMode ?  PasteboardPrivate::PrimarySelection : PasteboardPrivate::Clipboard;

nit: only one space after "?"


> Index: WebCore/platform/chromium/PasteboardPrivate.h
...
> +        enum TargetClipboard {
> +            Clipboard,
> +            PrimarySelection,
> +        };

Please change TargetClipboard to ClipboardTarget to match the naming
convention already in use (like ClipboardFormat).

I would also change the enum values to something like the following:

  SystemTarget
  SelectionTarget

Where SystemTarget refers to the system clipboard and SelectionTarget
refers to the selection clipboard.
Comment 12 Steve VanDeBogart 2009-09-03 15:26:40 PDT
Created attachment 39014 [details]
Handle middle click in Chromium like QT

Address Darin's comments.
Comment 13 Eric Seidel (no email) 2009-09-03 15:45:35 PDT
Comment on attachment 39014 [details]
Handle middle click in Chromium like QT

While we're fixing things, I would prefer "PASS 1 is 1" be something nicer. :)  But if Darin finds the rest of this OK, it's fine to land. :)

 23     shouldBe("1", stringify(pasteCount));

shouldBe(pasteCount, '1');
Comment 14 Steve VanDeBogart 2009-09-03 16:02:31 PDT
Created attachment 39016 [details]
Handle middle click in Chromium like QT

Oops, fix the layout test too.
Comment 15 Eric Seidel (no email) 2009-09-03 16:04:47 PDT
Comment on attachment 39016 [details]
Handle middle click in Chromium like QT

As far as I can tell you have addressed all of Darin and my concerns.  LGTM.
Comment 16 Eric Seidel (no email) 2009-09-03 16:23:11 PDT
Comment on attachment 39016 [details]
Handle middle click in Chromium like QT

Clearing flags on attachment: 39016

Committed r48035: <http://trac.webkit.org/changeset/48035>
Comment 17 Eric Seidel (no email) 2009-09-03 16:23:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Dimitri Glazkov (Google) 2009-09-04 10:00:26 PDT
This broke Chromium build. Rolling out. Please do not commit two-sided patches like this.
Comment 19 Dimitri Glazkov (Google) 2009-09-04 10:07:18 PDT
Rolled out in http://trac.webkit.org/changeset/48059.
Comment 20 Dimitri Glazkov (Google) 2009-09-04 10:07:41 PDT
Reopening.
Comment 21 Darin Fisher (:fishd, Google) 2009-09-04 15:30:58 PDT
This is commit-queue- because it is waiting for the corresponding chromium change:  http://codereview.chromium.org/174367
Comment 22 Steve VanDeBogart 2009-09-04 16:49:38 PDT
This is now ready to be committed
Comment 23 Eric Seidel (no email) 2009-09-05 01:17:30 PDT
A non-committer (like steve) setting commit-queue+ will sadly just hang the queue.  bug 28605.  I need to fix that so that it will properly cq- bugs in that situation. :)
Comment 24 Steve VanDeBogart 2009-09-05 08:42:43 PDT
Oops.  Still getting the hang of this system. I guess I meant to set commit-queue? ?
Comment 25 Eric Seidel (no email) 2009-09-08 10:17:20 PDT
(In reply to comment #24)
> Oops.  Still getting the hang of this system. I guess I meant to set
> commit-queue? ?

Yeah.  It's also totally my fault that cq+ by a non-committer hangs things.  I intend to fix that (bug 28605) today.
Comment 26 Nate Chapin 2009-09-08 10:58:30 PDT
Landed as r48168.