Bug 58656 - [Qt] X11: Text selection is causing oncopy event to be called
Summary: [Qt] X11: Text selection is causing oncopy event to be called
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Igor Trindade Oliveira
URL:
Keywords: Qt, QtTriaged
Depends on: 58998
Blocks: 58448
  Show dependency treegraph
 
Reported: 2011-04-15 06:43 PDT by Igor Trindade Oliveira
Modified: 2011-04-20 09:08 PDT (History)
3 users (show)

See Also:


Attachments
test using drt showing the bug (808 bytes, text/html)
2011-04-15 06:43 PDT, Igor Trindade Oliveira
no flags Details
Patch (1.76 KB, patch)
2011-04-15 11:06 PDT, Igor Trindade Oliveira
kling: review-
Details | Formatted Diff | Diff
Patch (2.24 KB, patch)
2011-04-17 19:44 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (4.00 KB, patch)
2011-04-18 05:24 PDT, Igor Trindade Oliveira
kling: review-
Details | Formatted Diff | Diff
Patch (4.18 KB, patch)
2011-04-18 06:31 PDT, Igor Trindade Oliveira
kling: review-
Details | Formatted Diff | Diff
Patch (4.45 KB, patch)
2011-04-18 12:40 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (4.45 KB, patch)
2011-04-18 13:00 PDT, Igor Trindade Oliveira
kling: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (4.48 KB, patch)
2011-04-19 08:49 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Igor Trindade Oliveira 2011-04-15 06:43:33 PDT
Created attachment 89777 [details]
test using drt showing the bug

when selecting a text on QtTestBrowser, QtWebkit calls oncopy event. firefox and chrome does not have this behavior.
Comment 1 Igor Trindade Oliveira 2011-04-15 11:06:19 PDT
Created attachment 89813 [details]
Patch

Fix bug. Now QtTestBrowser has the same behavior of chrome and firefox.
Comment 2 Andreas Kling 2011-04-15 11:19:54 PDT
Comment on attachment 89813 [details]
Patch

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

r- because of the empty ChangeLog.

> Source/WebKit/qt/Api/qwebpage.cpp:750
> -                focusFrame->editor()->copy();
> +                Pasteboard::generalPasteboard()->writeSelection(focusFrame->editor()->selectedRange().get(), focusFrame->editor()->canSmartCopyOrDelete(), focusFrame);

Editor::copy() will make a text-only copy if the selection is inside a text form control.
After this change, we always make multi-part copies (with rich-text metadata.)
Does this match the behavior of Chromium on Linux? I.e do copies made using X11 selection contain rich-text data?

> Source/WebKit/qt/ChangeLog:6
> +        [Qt] Text selection is causing oncopy event be called
> +        https://bugs.webkit.org/show_bug.cgi?id=58656

We need more explanation of what's being changed and why in the ChangeLog.
Comment 3 Igor Trindade Oliveira 2011-04-17 19:44:10 PDT
Created attachment 89983 [details]
Patch

updating patch changelog
Comment 4 Antonio Gomes 2011-04-17 22:27:04 PDT
Comment on attachment 89983 [details]
Patch

No tests?
Comment 5 Igor Trindade Oliveira 2011-04-18 05:24:49 PDT
Created attachment 90021 [details]
Patch

Adding test
Comment 6 Andreas Kling 2011-04-18 05:31:26 PDT
Comment on attachment 90021 [details]
Patch

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

> LayoutTests/editing/pasteboard/text-selection-expected.txt:2
> +This file tests if text selection is causing oncopy event to be called. This test requires DRT.
> +oncopy event called

While this output is correct AFAICT, it certainly looks wrong.
Please format the output in a way that makes it clear what is being tested and what the expected results are.

> Source/WebKit/qt/ChangeLog:13
> +        Always when a text is selected the oncopy event is called, this behavior is
> +        not presented by browsers such as firefox and chrome. Now, when selecting
> +        a text, QtWebkit is making multi part-copies (with rich text metadata), the multi-part
> +        data can be obtained by data transfer interface.
> +        Also, Copies to the clipboard of a selected image, is not supported by browsers
> +        like chrome and firefox and was removed from QtWebkit.

s/browsers such as//
s/firefox/Firefox/
s/chrome/Chrome/
s/a text/text/
s/QtWebkit/QtWebKit/

Can we actually obtain the multi-part selection via the data transfer interface in QtWebKit today?
Comment 7 Igor Trindade Oliveira 2011-04-18 06:31:06 PDT
Created attachment 90025 [details]
Patch

Changelog fixes and updating test to check how many times the oncopy method is called.
Comment 8 Igor Trindade Oliveira 2011-04-18 06:32:45 PDT
> Can we actually obtain the multi-part selection via the data transfer interface in QtWebKit today?

Right now it is not supported so in changelog i put "... when supported by QtWebKit".
Comment 9 Alexis Menard (darktears) 2011-04-18 10:17:48 PDT
Comment on attachment 90025 [details]
Patch

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

> Source/WebKit/qt/ChangeLog:9
> +        not presented by Firefox and Chrome. Now, when selecting a text, QtWebKit 

, this behavior is not present in Firefox or Chrome.

> Source/WebKit/qt/ChangeLog:11
> +        data can be obtained by data transfer interface when supported by QtWebKit.

when it will be supported by QtWebKit.

> Source/WebKit/qt/ChangeLog:12
> +        Also, Copies to the clipboard of a selected image, is not supported by Chrome and 

Copies -> copies
Comment 10 Alexis Menard (darktears) 2011-04-18 10:18:42 PDT
(In reply to comment #9)
> (From update of attachment 90025 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90025&action=review
> 
> > Source/WebKit/qt/ChangeLog:9
> > +        not presented by Firefox and Chrome. Now, when selecting a text, QtWebKit 
> 
> , this behavior is not present in Firefox or Chrome.

or this behavior does not exist in Firefox or Chrome.

> 
> > Source/WebKit/qt/ChangeLog:11
> > +        data can be obtained by data transfer interface when supported by QtWebKit.
> 
> when it will be supported by QtWebKit.
> 
> > Source/WebKit/qt/ChangeLog:12
> > +        Also, Copies to the clipboard of a selected image, is not supported by Chrome and 
> 
> Copies -> copies
Comment 11 Andreas Kling 2011-04-18 10:49:32 PDT
Comment on attachment 90025 [details]
Patch

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

> LayoutTests/editing/pasteboard/text-selection-expected.txt:2
> +This file tests if text selection is causing oncopy event to be called. This test requires DRT.
> +oncopy event called 1 time

Please explain that the text below should read that the oncopy event was triggered only once.
The output could be something like:

This file tests if text selection causes oncopy events. This test requires DRT.

Selecting text with mouse, no copy events should occur:
Event count: 0

Copying text, 1 copy event should occur:
Event count: 1

> LayoutTests/editing/pasteboard/text-selection.html:37
> +    eventSender.mouseMoveTo(0, 0);
> +    for (var i = 0; i < 3; i++) {
> +        eventSender.mouseDown();
> +        eventSender.mouseUp();
> +    }

This is making a triple-click to select the whole line?
Will it work on other platforms? This test will be run on the Mac, Chromium, Gtk ports, etc. I don't know if they all support triple-click selection.
It would be cleaner to do a normal selection via mouseDown, mouseMove, mouseUp over the text.
Comment 12 Igor Trindade Oliveira 2011-04-18 12:40:00 PDT
Created attachment 90072 [details]
Patch

Updating test .
Comment 13 Igor Trindade Oliveira 2011-04-18 13:00:45 PDT
Created attachment 90077 [details]
Patch

Some aesthetics fixes
Comment 14 Andreas Kling 2011-04-18 14:01:08 PDT
Comment on attachment 90077 [details]
Patch

LGTM, fingers crossed that the test works everywhere :)
Comment 15 WebKit Commit Bot 2011-04-18 17:03:35 PDT
Comment on attachment 90077 [details]
Patch

Rejecting attachment 90077 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 1

Last 500 characters of output:
autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://queues.webkit.org/results/8468190
Comment 16 Diego Gonzalez 2011-04-19 06:22:37 PDT
Comment on attachment 90077 [details]
Patch

Let's try again
Comment 17 WebKit Commit Bot 2011-04-19 06:26:16 PDT
Comment on attachment 90077 [details]
Patch

Rejecting attachment 90077 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 1

Last 500 characters of output:
autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://queues.webkit.org/results/8470333
Comment 18 Igor Trindade Oliveira 2011-04-19 08:49:17 PDT
Created attachment 90208 [details]
Patch

updating patch
Comment 19 WebKit Commit Bot 2011-04-19 10:12:14 PDT
The commit-queue encountered the following flaky tests while processing attachment 90208 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 20 WebKit Commit Bot 2011-04-19 10:15:47 PDT
Comment on attachment 90208 [details]
Patch

Clearing flags on attachment: 90208

Committed r84263: <http://trac.webkit.org/changeset/84263>
Comment 21 WebKit Commit Bot 2011-04-19 10:15:54 PDT
All reviewed patches have been landed.  Closing bug.