Bug 99698

Summary: Unable to copy text on disabled input fields on long press gesture
Product: WebKit Reporter: Kaustubh Atrawalkar <kaustubh.ra>
Component: HTML EditingAssignee: Kaustubh Atrawalkar <kaustubh.ra>
Status: REOPENED ---    
Severity: Critical CC: bassza00133, dcheng, dglazkov, enrica, karlcow, kaustubh.ra, morrita, rniwa, senorblanco, tonikitoo, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Android   
URL: http://code.google.com/p/chromium/issues/detail?id=152552
Bug Depends on: 101169    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
bassza00133@gmail.com bassza00133: review+, ews-feeder: commit-queue-

Description Kaustubh Atrawalkar 2012-10-18 01:39:40 PDT 
Device name: Samsung Galaxy S3
OS: ICS 4.0.4
Chrome: 18.0.1025308


When a webpage has a text field (input field) that is disabled, tap and hold on such field does not open clipboard functions menu to select/copy text from that field. Neither text selection nor copy to clipboard entire text is possible.
Comment 1 Kaustubh Atrawalkar 2012-10-18 02:03:29 PDT 
Created attachment 169369 [details]
Patch
Comment 2 WebKit Review Bot 2012-10-18 03:53:23 PDT 
Comment on attachment 169369 [details]
Patch

Attachment 169369 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14395946

New failing tests:
fast/events/touch/gesture/disabled-input-text-selection.html
Comment 3 Ryosuke Niwa 2012-10-18 09:09:30 PDT 
Comment on attachment 169369 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2579
> -    if (!result.isLiveLink() && innerNode && (innerNode->isContentEditable() || innerNode->isTextNode())) {
> +    if (!result.isLiveLink() && innerNode && innerNode->canStartSelection()) {

It appears that sendContextMenuEvent has the same issue.

> LayoutTests/fast/events/touch/gesture/disabled-input-text-selection.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Please use HTML5 style DOCTYPE: <!DOCTYPE html>

> LayoutTests/fast/events/touch/gesture/disabled-input-text-selection.html:9
> +<div id="result">FAIL</div>

If you used shouldBe, then you wouldn't need this.

> LayoutTests/fast/events/touch/gesture/disabled-input-text-selection.html:26
> +    if (eventSender.gestureLongPress) {
> +        eventSender.gestureLongPress(x, y);
> +    } else {

No curly brackets around a single line statement.

> LayoutTests/fast/events/touch/gesture/disabled-input-text-selection.html:32
> +    if (window.getSelection() == "Disabled")
> +        document.getElementById("result").innerHTML = "PASS";

Why don't you use shouldBe() instead?
Comment 4 Ryosuke Niwa 2012-10-18 09:09:58 PDT 
This is not specific to Chromium.
Comment 5 Ryosuke Niwa 2012-10-18 09:11:09 PDT 
Comment on attachment 169369 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Disabled input/textarea doesn't trigger selection change

I just renamed the bug title. Please update change logs to reflect that when you upload a new patch.

> LayoutTests/ChangeLog:3
> +        Disabled input/textarea doesn't trigger selection change

Ditto.
Comment 6 Kaustubh Atrawalkar 2012-10-20 00:42:27 PDT 
Created attachment 169763 [details]
Patch
Comment 7 WebKit Review Bot 2012-10-20 01:33:41 PDT 
Comment on attachment 169763 [details]
Patch

Attachment 169763 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14456840

New failing tests:
fast/events/touch/gesture/disabled-input-text-selection.html
Comment 8 Kaustubh Atrawalkar 2012-10-22 00:36:59 PDT 
Created attachment 169839 [details]
Patch
Comment 9 Shinya Kawanaka 2012-10-22 00:59:32 PDT 
Comment on attachment 169839 [details]
Patch

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

> LayoutTests/platform/chromium/TestExpectations:3143
> +webkit.org/b/99698 fast/events/touch/gesture/disabled-input-text-selection.html [ Failure Pass ]

Why is this marked as flaky?
Comment 10 Kaustubh Atrawalkar 2012-10-22 01:41:15 PDT 
(In reply to comment #9)
> (From update of attachment 169839 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169839&action=review
> 
> > LayoutTests/platform/chromium/TestExpectations:3143
> > +webkit.org/b/99698 fast/events/touch/gesture/disabled-input-text-selection.html [ Failure Pass ]
> 
> Why is this marked as flaky?

Because Long touch gesture selecting word is not a valid test case for chromium port. I am thinking, that long press should be passed as double click in case of chromium port. Correct me if wrong.
Comment 11 Kaustubh Atrawalkar 2012-10-23 01:58:41 PDT 
Created attachment 170084 [details]
Patch
Comment 12 Ryosuke Niwa 2012-10-25 00:09:55 PDT 
Comment on attachment 170084 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2579
> -    if (!result.isLiveLink() && innerNode && (innerNode->isContentEditable() || innerNode->isTextNode())) {
> +    if (!result.isLiveLink() && innerNode && innerNode->canStartSelection()) {

Why don't you use canMouseDownStartSelect here? Also, I said, EventHandler::sendContextMenuEvent has a similar code. Why is it okay not to change that?

> LayoutTests/fast/events/touch/gesture/disabled-input-text-selection.html:21
> +if (!window.eventSender)
> +    return;

return?? It doesn't do anything useful here.

> LayoutTests/fast/events/touch/gesture/disabled-input-text-selection.html:27
> +    return;

Ditto.
Comment 13 Kaustubh Atrawalkar 2012-10-30 03:48:25 PDT 
Created attachment 171410 [details]
Patch
Comment 14 Build Bot 2012-10-30 04:21:10 PDT 
Comment on attachment 171410 [details]
Patch

Attachment 171410 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14640468

New failing tests:
editing/selection/5354455-2.html
Comment 15 WebKit Review Bot 2012-11-04 01:20:37 PST 
Comment on attachment 171410 [details]
Patch

Clearing flags on attachment: 171410

Committed r133416: <http://trac.webkit.org/changeset/133416>
Comment 16 WebKit Review Bot 2012-11-04 01:20:41 PST 
All reviewed patches have been landed.  Closing bug.
Comment 17 Kaustubh Atrawalkar 2012-11-04 01:49:32 PST 
Comment on attachment 171410 [details]
Patch

Trying again. Seems to be tree was red.
Comment 18 Stephen White 2012-11-04 06:55:18 PST 
This seems to have regressed editing/selection/5354455-2.html on Mac:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&showLargeExpectations=true&tests=editing%2Fselection%2F5354455-2.html

If I'm reading the test correctly, my suspicion is that a right-click outside of a text element no longer cancels the selection.
Comment 19 Stephen White 2012-11-04 07:49:46 PST 
(In reply to comment #18)
> This seems to have regressed editing/selection/5354455-2.html on Mac:
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&showLargeExpectations=true&tests=editing%2Fselection%2F5354455-2.html
> 
> If I'm reading the test correctly, my suspicion is that a right-click outside of a text element no longer cancels the selection.

OK, I tested this manually (http://trac.webkit.org/export/133422/trunk/LayoutTests/editing/selection/5354455-2.html) with a ToT build, and it seems that a right-click outside the box (text area) now causes the first word in the paragraph to be selected.  So I think this is a real bug.
Comment 20 Stephen White 2012-11-04 12:04:57 PST 
Also causing a failure in browser_tests RenderViewImplTest.ContextMenu: 

http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/14387
Comment 21 Ryosuke Niwa 2012-11-04 13:17:51 PST 
Comment on attachment 171410 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2586
> -    if (!result.isLiveLink() && innerNode && (innerNode->isContentEditable() || innerNode->isTextNode())) {
> +    if (!result.isLiveLink() && innerNode && canMouseDownStartSelect(innerNode)) {

Ah, we're now missing innerNode->isTextNode() check.

> Source/WebCore/page/EventHandler.cpp:2703
> -        && (m_frame->selection()->isContentEditable() || (mev.targetNode() && mev.targetNode()->isTextNode()))) {
> +        && canMouseDownStartSelect(mev.targetNode())) {

Ditto.
Comment 22 Ryosuke Niwa 2012-11-04 13:49:57 PST 
Comment on attachment 171410 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2703
>> +        && canMouseDownStartSelect(mev.targetNode())) {
> 
> Ditto.

Or rather, this line shouldn't have been changed :(
Comment 23 Ryosuke Niwa 2012-11-04 14:21:40 PST 
The correct fix isn't obvious. We need to roll out this patch.
Comment 24 WebKit Review Bot 2012-11-04 14:24:42 PST 
Re-opened since this is blocked by bug 101169
Comment 25 Ryosuke Niwa 2012-11-04 14:27:15 PST 
Here's a problem. The bug says we need to select the closest word even when hit test target isn't an editable or text node. However, this behavior isn't desirable outside of an input field. Perhaps what we want to do is to special case textarea and input elements. But that won't work for any "component" implemented by shadow DOM API.

Perhaps we want to do this only if we're on the right of some text (in English. which means on the right of RTL text, and on the bottom of vertically written top-down text or on the top of vertically written bottom-up text).
Comment 26 Ryosuke Niwa 2012-11-04 14:27:53 PST 
a
Comment 27 Kaustubh Atrawalkar 2012-11-05 01:09:46 PST 
As far i see, the issue is only related to Chromium on android. So keeping Context menu conditions in tact will be better. I will just need to change the condition for Android case leaving other context menu condition as it is.
Comment 28 Ryosuke Niwa 2012-11-05 10:02:11 PST 
(In reply to comment #27)
> As far i see, the issue is only related to Chromium on android. So keeping Context menu conditions in tact will be better. I will just need to change the condition for Android case leaving other context menu condition as it is.

No, that won’t work. Your patch will enable long press anywhere on the page and will select the wrong word. Open editing/selection/5354455-2.html and try it yourself.
Comment 29 bass thiwat 2024-01-12 13:02:20 PST 
bassza00133@hotmail.com
Comment 31 EWS 2024-01-12 13:04:50 PST 
bassza00133@gmail.com does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

Rejecting attachment 469383 [details] from commit queue.