Bug 216005

Created attachment 407686 [details]
Patch

Comment on attachment 407686 [details]
Patch

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

> Source/WebCore/html/shadow/DateTimeFieldElement.cpp:64
> +        if (!isFieldOwnerDisabled() && !isFieldOwnerReadOnly()) {

`readonly` inputs are still able to receive events.  They basically are the same as regular inputs except that typing doesn't update the `value`.  Is that still possible with this?  Am I reading this code wrong?

(In reply to Devin Rousso from comment #2)
> Comment on attachment 407686 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407686&action=review
> 
> > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:64
> > +        if (!isFieldOwnerDisabled() && !isFieldOwnerReadOnly()) {
> 
> `readonly` inputs are still able to receive events.  They basically are the
> same as regular inputs except that typing doesn't update the `value`.  Is
> that still possible with this?  Am I reading this code wrong?

Keyboard events are still dispatched on the element. This code just prevents editability – `handleKeyboardEvent` is a virtual method that the editable fields use to respond to keyboard events.

Comment on attachment 407686 [details]
Patch

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

r=me, nice works!

> Source/WebCore/css/html.css:520
> +input[disabled]::-webkit-datetime-edit-year-field,

NIT: should these be `input[type="date"]`?

> Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:318
> +    return element()->isDisabledFormControl();

NIT: should this have `ASSERT(element())` too?

> Source/WebCore/html/shadow/DateTimeFieldElement.cpp:119
> +bool DateTimeFieldElement::isFocusable() const

(for my own knowledge) is this what's used to determine whether or not the `<input type="date">` is mouse-interactable?  I ask because `disabled` should prevent all interaction but `readonly` should still allow mouse interaction.

Created attachment 407818 [details]
Patch for landing

(In reply to Devin Rousso from comment #4)
> Comment on attachment 407686 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407686&action=review
> 
> r=me, nice works!

Thanks for the review!
 
> > Source/WebCore/css/html.css:520
> > +input[disabled]::-webkit-datetime-edit-year-field,
> 
> NIT: should these be `input[type="date"]`?

They probably should. I've added a FIXME to address this in a follow-up, since there are existing instances that should also be updated.
 
> > Source/WebCore/html/BaseChooserOnlyDateAndTimeInputType.cpp:318
> > +    return element()->isDisabledFormControl();
> 
> NIT: should this have `ASSERT(element())` too?

Added an assertion.
 
> > Source/WebCore/html/shadow/DateTimeFieldElement.cpp:119
> > +bool DateTimeFieldElement::isFocusable() const
> 
> (for my own knowledge) is this what's used to determine whether or not the
> `<input type="date">` is mouse-interactable?  I ask because `disabled`
> should prevent all interaction but `readonly` should still allow mouse
> interaction.

Partially.

If by mouse-interaction, you mean the ability to select the individual components of the control using a mouse, the answer is yes.

If by mouse-interaction, you mean the dispatching of mouse events, that is handled in existing code in Node::handleLocalEvents, which does not dispatch events if Element::isDisabledFormControl is true.

Committed r266514: <https://trac.webkit.org/changeset/266514>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407818 [details].

<rdar://problem/68278191>