Disabled input/textarea doesn't trigger selection change

[Daniel Cheng]

Created attachment 139543 [details]
Test case

I've attached a simple test case that illustrates this issue in Chromium.

[Daniel Cheng]

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.

[Ryosuke Niwa]

Ugh... yeah, thanks for filing a bug on this.

[Kaustubh Atrawalkar]

Created attachment 168878 [details]
Patch

[Kaustubh Atrawalkar]

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.

[Hajime Morrita]

Comment on attachment 168878 [details]
Patch

We needs some LayoutTest for exercising this.
You can find selection related tests under LayoutTests/editing/selection.

[Kaustubh Atrawalkar]

Created attachment 168907 [details]
Patch

[WebKit Review Bot]

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

[Kaustubh Atrawalkar]

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.

[Shinya Kawanaka]

Created attachment 180486 [details]
WIP

[Shinya Kawanaka]

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.

[kov's GTK+ EWS bot]

Comment on attachment 180486 [details]
WIP

Attachment 180486 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15445390

[Build Bot]

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.

[Shinya Kawanaka]

Created attachment 184678 [details]
Patch

[Shinya Kawanaka]

The previous WIP patch I uploaded before was for another Bug. I didn't noticed it.... sorry!
Anyway, I've attacked this bug.

[Ryosuke Niwa]

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?

[Shinya Kawanaka]

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

[Ryosuke Niwa]

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

[Shinya Kawanaka]

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

[Shinya Kawanaka]

Filed the bug in https://bugs.webkit.org/show_bug.cgi?id=108043

[WebKit Review Bot]

Comment on attachment 184678 [details]
Patch

Clearing flags on attachment: 184678

Committed r140936: <http://trac.webkit.org/changeset/140936>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.