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.
Created attachment 144718 [details] Patch
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?
(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.
Comment on attachment 144718 [details] Patch Clearing flags on attachment: 144718 Committed r119023: <http://trac.webkit.org/changeset/119023>
All reviewed patches have been landed. Closing bug.