[Qt] X11: Text selection is causing oncopy event to be called

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Igor Trindade Oliveira]

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.

[Igor Trindade Oliveira]

Created attachment 89813 [details]
Patch

Fix bug. Now QtTestBrowser has the same behavior of chrome and firefox.

[Andreas Kling]

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.

[Igor Trindade Oliveira]

Created attachment 89983 [details]
Patch

updating patch changelog

[Antonio Gomes]

Comment on attachment 89983 [details]
Patch

No tests?

[Igor Trindade Oliveira]

Created attachment 90021 [details]
Patch

Adding test

[Andreas Kling]

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?

[Igor Trindade Oliveira]

Created attachment 90025 [details]
Patch

Changelog fixes and updating test to check how many times the oncopy method is called.

[Igor Trindade Oliveira]

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

[Alexis Menard (darktears)]

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

[Alexis Menard (darktears)]

(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

[Andreas Kling]

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.

[Igor Trindade Oliveira]

Created attachment 90072 [details]
Patch

Updating test .

[Igor Trindade Oliveira]

Created attachment 90077 [details]
Patch

Some aesthetics fixes

[Andreas Kling]

Comment on attachment 90077 [details]
Patch

LGTM, fingers crossed that the test works everywhere :)

[WebKit Commit Bot]

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

[Diego Gonzalez]

Comment on attachment 90077 [details]
Patch

Let's try again

[WebKit Commit Bot]

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

[Igor Trindade Oliveira]

Created attachment 90208 [details]
Patch

updating patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 90208 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 90208 [details]
Patch

Clearing flags on attachment: 90208

Committed r84263: <http://trac.webkit.org/changeset/84263>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.