Bug 99698

Summary:

Unable to copy text on disabled input fields on long press gesture

Product:

WebKit

Reporter:

Kaustubh Atrawalkar <kaustubh.ra>

Component:

HTML Editing

Assignee:

Kaustubh Atrawalkar <kaustubh.ra>

Status:

REOPENED

Severity:

Critical

CC:

bassza00133, dcheng, dglazkov, enrica, Jackjimlobster147, karlcow, kaustubh.ra, morrita, rniwa, senorblanco, tonikitoo, webkit.review.bot

Priority:

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-

Kaustubh Atrawalkar

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

Attachments
Patch (4.37 KB, patch) 2012-10-18 02:03 PDT, Kaustubh Atrawalkar	no flags	Details Formatted Diff Diff
Patch (5.39 KB, patch) 2012-10-20 00:42 PDT, Kaustubh Atrawalkar	no flags	Details Formatted Diff Diff
Patch (5.44 KB, patch) 2012-10-22 00:36 PDT, Kaustubh Atrawalkar	no flags	Details Formatted Diff Diff
Patch (5.50 KB, patch) 2012-10-23 01:58 PDT, Kaustubh Atrawalkar	no flags	Details Formatted Diff Diff
Patch (6.16 KB, patch) 2012-10-30 03:48 PDT, Kaustubh Atrawalkar	no flags	Details Formatted Diff Diff
bassza00133@gmail.com (99 bytes, patch) 2024-01-12 13:04 PST, bass thiwat	bassza00133: review+ ews-feeder: commit-queue-	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Kaustubh Atrawalkar

Comment 1 2012-10-18 02:03:29 PDT

Created attachment 169369 [details] Patch

WebKit Review Bot

Comment 2 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

Ryosuke Niwa

Comment 3 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?

Ryosuke Niwa

Comment 4 2012-10-18 09:09:58 PDT

This is not specific to Chromium.

Ryosuke Niwa

Comment 5 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.

Kaustubh Atrawalkar

Comment 6 2012-10-20 00:42:27 PDT

Created attachment 169763 [details] Patch

WebKit Review Bot

Comment 7 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

Kaustubh Atrawalkar

Comment 8 2012-10-22 00:36:59 PDT

Created attachment 169839 [details] Patch

Shinya Kawanaka

Comment 9 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?

Kaustubh Atrawalkar

Comment 10 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.

Kaustubh Atrawalkar

Comment 11 2012-10-23 01:58:41 PDT

Created attachment 170084 [details] Patch

Ryosuke Niwa

Comment 12 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.

Kaustubh Atrawalkar

Comment 13 2012-10-30 03:48:25 PDT

Created attachment 171410 [details] Patch

Build Bot

Comment 14 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

WebKit Review Bot

Comment 15 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>

WebKit Review Bot

Comment 16 2012-11-04 01:20:41 PST

All reviewed patches have been landed. Closing bug.

Kaustubh Atrawalkar

Comment 17 2012-11-04 01:49:32 PST

Comment on attachment 171410 [details] Patch Trying again. Seems to be tree was red.

Stephen White

Comment 18 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.

Stephen White

Comment 19 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.

Stephen White

Comment 20 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

Ryosuke Niwa

Comment 21 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.

Ryosuke Niwa

Comment 22 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 :(

Ryosuke Niwa

Comment 23 2012-11-04 14:21:40 PST

The correct fix isn't obvious. We need to roll out this patch.

WebKit Review Bot

Comment 24 2012-11-04 14:24:42 PST

Re-opened since this is blocked by bug 101169

Ryosuke Niwa

Comment 25 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).

Ryosuke Niwa

Comment 26 2012-11-04 14:27:53 PST

Kaustubh Atrawalkar

Comment 27 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.

Ryosuke Niwa

Comment 28 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.

bass thiwat

Comment 29 2024-01-12 13:02:20 PST

bassza00133@hotmail.com

bass thiwat

Comment 30 2024-01-12 13:04:33 PST

Created attachment 469383 [details] bassza00133@gmail.com https://developers.google.com/privacy-sandbox/3pcd/chips?hl=th#cookie_partitioning_technical_design bassza00133@gmail.com

EWS

Comment 31 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.

Note You need to log in before you can comment on or make changes to this bug.