Summary: | Disabled input/textarea doesn't trigger selection change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Cheng <dcheng> | ||||||||||||
Component: | HTML Editing | Assignee: | Daniel Cheng <dcheng> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | danya.postfactum, dglazkov, gtk-ews, kaustubh.ra, mifenton, rniwa, shinyak, tkent, webkit.review.bot, xan.lopez | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
I think the root cause of this is WebViewImpl::caretOrSelectionRange returning false for selections inside disabled elements. Inside that function, we call TextIterator::getLocationAndLengthFromRange. Everything goes as expected until: // The critical assumption is that this only gets called with ranges that // concentrate on a given area containing the selection root. This is done // because of text fields and textareas. The DOM for those is not // directly in the document DOM, so ensure that the range does not cross a // boundary of one of those. if (range->startContainer() != scope && !range->startContainer()->isDescendantOf(scope)) return false; At that point: range->startContainer() == range->endContainer() == #text node in shadow DOM scope == selection->rootEditableElementOrDocumentElement() == HTML element, since there is no editable element in a readonly control. It seems like for the purposes of the selection code, we should be setting scope to put to a node inside the shadow DOM as well. Ugh... yeah, thanks for filing a bug on this. Created attachment 168878 [details]
Patch
I was working on the same issue filed on crbug.com/152552 The issue is in WebCore/page/EventHandler.cpp In function handleLongPressGesture() there is condition to check isContentEditable() which seems to be wrong as it will not select the text from disabled Input boxes. I have added a fix for the same to change it to canStartSelection(). Please review. Thanks. Comment on attachment 168878 [details]
Patch
We needs some LayoutTest for exercising this.
You can find selection related tests under LayoutTests/editing/selection.
Created attachment 168907 [details]
Patch
Comment on attachment 168907 [details] Patch Attachment 168907 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14385148 New failing tests: fast/events/touch/gesture/disabled-input-text-selection.html Comment on attachment 168907 [details]
Patch
This issue being little different than the one mentioned here - crbug.com/152552, as per Daniel, I have filed separate bug for the same at wkbug.com/99698. Please check there.
Created attachment 180486 [details]
WIP
Returning document seems wrong. Instead of it, I think we should return the root node of tree scope. But I don't know how we add a test here. Maybe run-api-tests? After writing a test, I'll upload the review-ready patch again. Comment on attachment 180486 [details] WIP Attachment 180486 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15445390 Comment on attachment 180486 [details] WIP Attachment 180486 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15455329 Any news here? I don't know whether it was said here, but textareas with readonly attribute have the same problem. Created attachment 184678 [details]
Patch
The previous WIP patch I uploaded before was for another Bug. I didn't noticed it.... sorry! Anyway, I've attacked this bug. Comment on attachment 184678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184678&action=review Makes sense. > LayoutTests/fast/forms/input-readonly-select.html:38 > +input.addEventListener('select', function() { > + selectEventFiredOnInput = true; > +}); Should we also check selectstart and selectionchange events? > Should we also check selectstart and selectionchange events? http://jsfiddle.net/wzbUE/ selectionchange is working even in disabled/readonly input/textarea. selectstart is only fired in contenteditable. Should it fired in input or textarea? Anyway, I'll land this patch after seeing bots green. (In reply to comment #17) > > Should we also check selectstart and selectionchange events? > > http://jsfiddle.net/wzbUE/ > > selectionchange is working even in disabled/readonly input/textarea. > selectstart is only fired in contenteditable. Should it fired in input or textarea? selectstart should fire. (In reply to comment #18) > (In reply to comment #17) > > > Should we also check selectstart and selectionchange events? > > > > http://jsfiddle.net/wzbUE/ > > > > selectionchange is working even in disabled/readonly input/textarea. > > selectstart is only fired in contenteditable. Should it fired in input or textarea? > > selectstart should fire. OK. But now I remember that we have a bug where a user cannot input any text in <input> or <textarea> if selectstart is cancelled somewhere in JS. Maybe it should be resolved first. Filed the bug in https://bugs.webkit.org/show_bug.cgi?id=108043 Comment on attachment 184678 [details] Patch Clearing flags on attachment: 184678 Committed r140936: <http://trac.webkit.org/changeset/140936> All reviewed patches have been landed. Closing bug. |
Created attachment 139543 [details] Test case I've attached a simple test case that illustrates this issue in Chromium.