Bug 87380 – Form controls in <fieldset disabled> should not be focusable.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

1. Load the following document.
<fieldset disabled>
  <input>
</fieldset>

2. Click the text field.

EXPECTED: Nothing happens.
ACTUAL: The text field gets focus and the focus ring is drawn.

[Kent Tamura]

Created attachment 144718 [details]
Patch

[Darin Adler]

Comment on attachment 144718 [details]
Patch

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

> Source/WebCore/html/HTMLFormControlElement.cpp:322
>  bool HTMLFormControlElement::supportsFocus() const
>  {
> -    return !m_disabled;
> +    return !disabled();
>  }

This code change shows that our naming in this class is not good! It's terrible to be in a situation where m_disabled is one thing and disabled() another, and switching from one to the other fixes a bug. The names should reflect differences between the two.

Our longer term direction should be to give good, clear names to this class's members. If the names from the DOM standard are causing us problems then we should think about solutions to that; maybe we could start putting prefixes on some names used in DOM bindings to make sure no one is tempted to use them inside WebKit itself.

Does this code change have any other effects? Is being wrapped inside a disabled fieldset the only difference between disabled() and m_disabled. Are there tests showing these other effects are OK?

[Kent Tamura]

(In reply to comment #2)
> This code change shows that our naming in this class is not good! It's terrible to be in a situation where m_disabled is one thing and disabled() another, and switching from one to the other fixes a bug. The names should reflect differences between the two.
> 
> Our longer term direction should be to give good, clear names to this class's members. If the names from the DOM standard are causing us problems then we should think about solutions to that; maybe we could start putting prefixes on some names used in DOM bindings to make sure no one is tempted to use them inside WebKit itself.

I agree with renaming.
 * rename m_disabled to m_reflectedDisabled, m_bareDisabled, m_rawDisabled, m_disabledAttributeValue, or
 * rename disabled() to effectiveDisabled() or something?

The specification says 'disabled' for our disabled().

> Does this code change have any other effects? Is being wrapped inside a disabled fieldset the only difference between disabled() and m_disabled. Are there tests showing these other effects are OK?

I think we have no more bugs about <fieldset disabled>.  But the test coverage might be not good.

[WebKit Review Bot]

Comment on attachment 144718 [details]
Patch

Clearing flags on attachment: 144718

Committed r119023: <http://trac.webkit.org/changeset/119023>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.