[Forms] The option element should not be form associated element.
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Mar 2012 04:46:00 +0000 (04:46 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Mar 2012 04:46:00 +0000 (04:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=79764

Patch by Yoshifumi Inoue <yosin@chromium.org> on 2012-03-20
Reviewed by Kent Tamura.

Source/WebCore:

This patch changes base class of HTMLOptionELement to HTMLElement
from HTMLFormControlElement for saving memory space and iteration
time of extra "option" elements in HTMLFormElement::m_formAssociatedElements
and matching the HTML5 specification for ease of maintenance.

This patch changes behavior of handling of CSS pseudo classes "invalid"
and "valid". The "option" elements no longer use these CSS pseudo classes
as HTML5 specification. This bug was filed in https://bugs.webkit.org/show_bug.cgi?id=80088

Changes of TextIterator is lead by usage of isFormControlElement. This
changes will be replaced with more meaningful predicate as part of
https://bugs.webkit.org/show_bug.cgi?id=80381

No new tests but updated select-live-pseudo-selectors.html test.

* css/CSSStyleSelector.cpp:
(WebCore::CSSStyleSelector::canShareStyleWithElement): Added checking of the "option" element and returns false as HTMLFormControlElement.
* css/SelectorChecker.cpp:
(WebCore::SelectorChecker::checkOneSelector): Removed isFormControlElement for PseudoDisabled and PseudoChecked.
* html/HTMLKeygenElement.cpp:
(WebCore::HTMLKeygenElement::HTMLKeygenElement): Removed form parameter of call site of HTMLOptionElement::create.
* html/HTMLOptionElement.cpp:
(WebCore::HTMLOptionElement::HTMLOptionElement): Removed form parameter which no longer needed. Changed base class in initialization list. Added m_disabled initialization.
(WebCore::HTMLOptionElement::create): Removed form parameter which no longer needed.
(WebCore::HTMLOptionElement::attach): Changeid base class.
(WebCore::HTMLOptionElement::detach): Changed base class.
(WebCore::HTMLOptionElement::parseAttribute): Changed base class. Added "disabled" attribute handling.
(WebCore::HTMLOptionElement::childrenChanged): Changed base class.
(WebCore::HTMLOptionElement::insertedIntoTree): Changed base class.
* html/HTMLOptionElement.h:
(HTMLOptionElement): Added new member variable m_disabled which was in HTMLFormControlElement.
(WebCore::HTMLOptionElement::ownElementDisabled): Changed for using m_disabled.
* html/HTMLTagNames.in: Removed constructorNeedsFormElement for the "option" element, which was used for passing form parameter to create function.

LayoutTests:

This patch fixes a bug in select-live-pseudo-selectors.js, adds
assertions to improve coverage, and updates test expectation for
behavior changes (makes the "option" element uses CSS pseudo class
":valid".)

* fast/forms/resources/select-live-pseudo-selectors.js:
(mouseDownOnSelect): Copied from listbox-selection.html for replacing broken simulateClick which used position and size of the "option" element, but these values are zero. Note: five files use mouseDownOnSelect. We'll share this function in future tracked by https://bugs.webkit.org/show_bug.cgi?id=81496.
(backgroundOf): Added String parameter support for ease of writing test case.
* fast/forms/select-live-pseudo-selectors-expected.txt: Added check fo background color of the "selection" element. Changed expected color of the "option" element because the "option" element doesn't support CSS pseudo class ":valid". This also covers bug 80088.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@111497 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/forms/resources/select-live-pseudo-selectors.js
LayoutTests/fast/forms/select-live-pseudo-selectors-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/css/CSSStyleSelector.cpp
Source/WebCore/css/SelectorChecker.cpp
Source/WebCore/html/HTMLKeygenElement.cpp
Source/WebCore/html/HTMLOptionElement.cpp
Source/WebCore/html/HTMLOptionElement.h
Source/WebCore/html/HTMLTagNames.in

index 311264c2d1c9fcc015fb5a350478e8bebcdf19bc..d53ed2459e450bc60306a824705abb5b24cfa18e 100644 (file)
@@ -1,3 +1,20 @@
+2012-03-20  Yoshifumi Inoue  <yosin@chromium.org>
+
+        [Forms] The option element should not be form associated element.
+        https://bugs.webkit.org/show_bug.cgi?id=79764
+
+        Reviewed by Kent Tamura.
+
+        This patch fixes a bug in select-live-pseudo-selectors.js, adds
+        assertions to improve coverage, and updates test expectation for
+        behavior changes (makes the "option" element uses CSS pseudo class
+        ":valid".)
+
+        * fast/forms/resources/select-live-pseudo-selectors.js:
+        (mouseDownOnSelect): Copied from listbox-selection.html for replacing broken simulateClick which used position and size of the "option" element, but these values are zero. Note: five files use mouseDownOnSelect. We'll share this function in future tracked by https://bugs.webkit.org/show_bug.cgi?id=81496.
+        (backgroundOf): Added String parameter support for ease of writing test case.
+        * fast/forms/select-live-pseudo-selectors-expected.txt: Added check fo background color of the "selection" element. Changed expected color of the "option" element because the "option" element doesn't support CSS pseudo class ":valid". This also covers bug 80088.
+
 2012-03-20  Dan Bernstein  <mitz@apple.com>
 
         Skipped fast/selectors/selection-window-inactive.html in WebKit2 because of
index 6380bc29ddd67521d3ca1fa461739a109641f06b..35c3a5b4581846346006c218f59ea2749115a5d4 100644 (file)
@@ -6,17 +6,18 @@ document.body.appendChild(form);
 var nonForm = document.createElement('div');
 document.body.appendChild(nonForm);
 
-function simulateClick(element) {
-    var rect = element.getBoundingClientRect();
-    var x = rect.left + rect.width / 2;
-    var y = rect.top + rect.height / 2;
-
-    if (!window.eventSender) {
-        return;
+function mouseDownOnSelect(selId, index, modifier) {
+    var sl = document.getElementById(selId);
+    var itemHeight = Math.floor(sl.offsetHeight / sl.size);
+    var border = 1;
+    var y = border + index * itemHeight;
+
+    sl.focus();
+    if (window.eventSender) {
+        eventSender.mouseMoveTo(sl.offsetLeft + border, sl.offsetTop + y - window.pageYOffset);
+        eventSender.mouseDown(0, [modifier]);
+        eventSender.mouseUp(0, [modifier]);
     }
-    eventSender.mouseMoveTo(x, y);
-    eventSender.mouseDown();
-    eventSender.mouseUp();
 }
 
 function makeInvalid() {
@@ -50,6 +51,8 @@ function removeOption(option, select) {
 }
 
 function backgroundOf(el) {
+    if (typeof(el) == 'string')
+        el = document.getElementById(el);
     return document.defaultView.getComputedStyle(el, null).getPropertyValue('background-color');
 }
 
@@ -58,6 +61,7 @@ var invalidColor = 'rgb(255, 0, 0)';
 var normalColor = 'rgb(255, 255, 255)';
 var disabledColor = 'rgb(0, 0, 0)';
 var readOnlyColor = 'rgb(0, 255, 0)'
+var transparentColor = 'rgba(0, 0, 0, 0)';
 var validColor = 'rgb(0, 0, 255)';
 
 // --------------------------------
@@ -102,7 +106,7 @@ shouldBe(elBackground, 'invalidColor');
 // --------------------------------
 
 debug('Change the values of select elements without explicit initializing values by clicking:');
-form.innerHTML = '<select id="select-multiple" multiple required>' +
+form.innerHTML = '<select id="select-multiple" multiple required size="4">' +
 '  <option id="multiple-empty">empty</option>' +
 '  <option id="multiple-another">another</option>' +
 '</select>' +
@@ -110,14 +114,12 @@ form.innerHTML = '<select id="select-multiple" multiple required>' +
 '  <option id="size4-empty">empty</option>' +
 '  <option id="size4-another">another</option>' +
 '</select>';
-var selectMultiple = document.getElementById("multiple-empty");
-selectMultiple.focus();
-simulateClick(selectMultiple);
-var selectSize4 = document.getElementById("size4-empty");
-selectSize4.focus();
-simulateClick(selectSize4);
-shouldBe('backgroundOf(selectMultiple)', 'validColor');
-shouldBe('backgroundOf(selectSize4)', 'validColor');
+mouseDownOnSelect('select-multiple', 0);
+mouseDownOnSelect('select-size4', 0);
+shouldBe('backgroundOf("select-multiple")', 'validColor');
+shouldBe('backgroundOf("multiple-empty")', 'transparentColor');
+shouldBe('backgroundOf("select-size4")', 'validColor');
+shouldBe('backgroundOf("size4-empty")', 'transparentColor');
 
 debug('Change the value with a placeholder label option:');
 el = makeInvalid();
index 8197ac498f3d363fc56377e63249538da393b297..e2265367fa935f543eb9a755ef75bec0e9b7f119 100644 (file)
@@ -15,8 +15,10 @@ Inside/outside of a form:
 PASS backgroundOf(el) is invalidColor
 PASS backgroundOf(el) is invalidColor
 Change the values of select elements without explicit initializing values by clicking:
-PASS backgroundOf(selectMultiple) is validColor
-PASS backgroundOf(selectSize4) is validColor
+PASS backgroundOf("select-multiple") is validColor
+PASS backgroundOf("multiple-empty") is transparentColor
+PASS backgroundOf("select-size4") is validColor
+PASS backgroundOf("size4-empty") is transparentColor
 Change the value with a placeholder label option:
 PASS backgroundOf(el) is validColor
 PASS backgroundOf(el) is invalidColor
index 590d20d88c1afdce88f43632387652a4cd839ecc..1dbdb197d0519d697a29ac54628bcf7557852245 100644 (file)
@@ -1,3 +1,44 @@
+2012-03-20  Yoshifumi Inoue  <yosin@chromium.org>
+
+        [Forms] The option element should not be form associated element.
+        https://bugs.webkit.org/show_bug.cgi?id=79764
+
+        Reviewed by Kent Tamura.
+
+        This patch changes base class of HTMLOptionELement to HTMLElement
+        from HTMLFormControlElement for saving memory space and iteration
+        time of extra "option" elements in HTMLFormElement::m_formAssociatedElements
+        and matching the HTML5 specification for ease of maintenance.
+
+        This patch changes behavior of handling of CSS pseudo classes "invalid"
+        and "valid". The "option" elements no longer use these CSS pseudo classes
+        as HTML5 specification. This bug was filed in https://bugs.webkit.org/show_bug.cgi?id=80088
+
+        Changes of TextIterator is lead by usage of isFormControlElement. This
+        changes will be replaced with more meaningful predicate as part of
+        https://bugs.webkit.org/show_bug.cgi?id=80381
+
+        No new tests but updated select-live-pseudo-selectors.html test.
+
+        * css/CSSStyleSelector.cpp:
+        (WebCore::CSSStyleSelector::canShareStyleWithElement): Added checking of the "option" element and returns false as HTMLFormControlElement.
+        * css/SelectorChecker.cpp:
+        (WebCore::SelectorChecker::checkOneSelector): Removed isFormControlElement for PseudoDisabled and PseudoChecked.
+        * html/HTMLKeygenElement.cpp:
+        (WebCore::HTMLKeygenElement::HTMLKeygenElement): Removed form parameter of call site of HTMLOptionElement::create.
+        * html/HTMLOptionElement.cpp:
+        (WebCore::HTMLOptionElement::HTMLOptionElement): Removed form parameter which no longer needed. Changed base class in initialization list. Added m_disabled initialization.
+        (WebCore::HTMLOptionElement::create): Removed form parameter which no longer needed.
+        (WebCore::HTMLOptionElement::attach): Changeid base class.
+        (WebCore::HTMLOptionElement::detach): Changed base class.
+        (WebCore::HTMLOptionElement::parseAttribute): Changed base class. Added "disabled" attribute handling.
+        (WebCore::HTMLOptionElement::childrenChanged): Changed base class.
+        (WebCore::HTMLOptionElement::insertedIntoTree): Changed base class.
+        * html/HTMLOptionElement.h:
+        (HTMLOptionElement): Added new member variable m_disabled which was in HTMLFormControlElement.
+        (WebCore::HTMLOptionElement::ownElementDisabled): Changed for using m_disabled.
+        * html/HTMLTagNames.in: Removed constructorNeedsFormElement for the "option" element, which was used for passing form parameter to create function.
+
 2012-03-20  Xiaomei Ji  <xji@chromium.org>
 
         Crash introduced in r110965.
index 97178ea028affaf619a8273b05b49327abf58f0f..7a2a54f867fc92ed49be4ca6c3e7d11bbd08c010 100644 (file)
@@ -65,6 +65,7 @@
 #include "HTMLElement.h"
 #include "HTMLInputElement.h"
 #include "HTMLNames.h"
+#include "HTMLOptionElement.h"
 #include "HTMLProgressElement.h"
 #include "HTMLStyleElement.h"
 #include "HTMLTextAreaElement.h"
@@ -1344,6 +1345,18 @@ bool CSSStyleSelector::canShareStyleWithElement(StyledElement* element) const
     }
 #endif
 
+    if (element->hasTagName(optionTag)) {
+        if (!m_element->hasTagName(optionTag))
+            return false;
+
+        HTMLOptionElement* thisOptionElement = static_cast<HTMLOptionElement*>(element);
+        HTMLOptionElement* otherOptionElement = static_cast<HTMLOptionElement*>(m_element);
+        if (thisOptionElement->isEnabledFormControl() != otherOptionElement->isEnabledFormControl())
+            return false;
+        if (thisOptionElement->selected() != otherOptionElement->selected())
+            return false;
+    }
+
     bool isControl = element->isFormControlElement();
 
     if (isControl != m_element->isFormControlElement())
index 086491593dd3c2c8261b883dd1c73d33f720931c..bf3ffd56c981ea1382f56cd7c480a38a760d813b 100644 (file)
@@ -1052,7 +1052,7 @@ bool SelectorChecker::checkOneSelector(const SelectorCheckingContext& context, P
         case CSSSelector::PseudoDefault:
             return element && element->isDefaultButtonForForm();
         case CSSSelector::PseudoDisabled:
-            if (element && element->isFormControlElement())
+            if (element && (element->isFormControlElement() || element->hasTagName(optionTag)))
                 return !element->isEnabledFormControl();
             break;
         case CSSSelector::PseudoReadOnly:
@@ -1079,7 +1079,7 @@ bool SelectorChecker::checkOneSelector(const SelectorCheckingContext& context, P
             return (element->willValidate() && !element->isValidFormControlElement()) || element->hasUnacceptableValue();
         case CSSSelector::PseudoChecked:
             {
-                if (!element || !element->isFormControlElement())
+                if (!element)
                     break;
                 // Even though WinIE allows checked and indeterminate to co-exist, the CSS selector spec says that
                 // you can't be both checked and indeterminate. We will behave like WinIE behind the scenes and just
index 80534419e731e5f90c1d9457fb2d4253c5a44a69..3094c93523e563dd17b9219445ec729edf8d6413 100644 (file)
@@ -81,7 +81,7 @@ inline HTMLKeygenElement::HTMLKeygenElement(const QualifiedName& tagName, Docume
     RefPtr<HTMLSelectElement> select = KeygenSelectElement::create(document);
     ExceptionCode ec = 0;
     for (size_t i = 0; i < keys.size(); ++i) {
-        RefPtr<HTMLOptionElement> option = HTMLOptionElement::create(document, this->form());
+        RefPtr<HTMLOptionElement> option = HTMLOptionElement::create(document);
         select->appendChild(option, ec);
         option->appendChild(Text::create(document, keys[i]), ec);
     }
index 1696e9bec4fc72dc098f3a3f1ba38108d40f94ce..a9be7c0619176fb313a18342cd3f232761b9af20 100644 (file)
@@ -37,6 +37,7 @@
 #include "NodeRenderStyle.h"
 #include "NodeRenderingContext.h"
 #include "RenderMenuList.h"
+#include "RenderTheme.h"
 #include "ScriptElement.h"
 #include "Text.h"
 #include <wtf/StdLibExtras.h>
@@ -47,21 +48,22 @@ namespace WebCore {
 
 using namespace HTMLNames;
 
-HTMLOptionElement::HTMLOptionElement(const QualifiedName& tagName, Document* document, HTMLFormElement* form)
-    : HTMLFormControlElement(tagName, document, form)
+HTMLOptionElement::HTMLOptionElement(const QualifiedName& tagName, Document* document)
+    : HTMLElement(tagName, document)
+    , m_disabled(false)
     , m_isSelected(false)
 {
     ASSERT(hasTagName(optionTag));
 }
 
-PassRefPtr<HTMLOptionElement> HTMLOptionElement::create(Document* document, HTMLFormElement* form)
+PassRefPtr<HTMLOptionElement> HTMLOptionElement::create(Document* document)
 {
-    return adoptRef(new HTMLOptionElement(optionTag, document, form));
+    return adoptRef(new HTMLOptionElement(optionTag, document));
 }
 
-PassRefPtr<HTMLOptionElement> HTMLOptionElement::create(const QualifiedName& tagName, Document* document, HTMLFormElement* form)
+PassRefPtr<HTMLOptionElement> HTMLOptionElement::create(const QualifiedName& tagName, Document* document)
 {
-    return adoptRef(new HTMLOptionElement(tagName, document, form));
+    return adoptRef(new HTMLOptionElement(tagName, document));
 }
 
 PassRefPtr<HTMLOptionElement> HTMLOptionElement::createForJSConstructor(Document* document, const String& data, const String& value,
@@ -89,13 +91,13 @@ void HTMLOptionElement::attach()
 {
     if (parentNode()->renderStyle())
         setRenderStyle(styleForRenderer());
-    HTMLFormControlElement::attach();
+    HTMLElement::attach();
 }
 
 void HTMLOptionElement::detach()
 {
     m_style.clear();
-    HTMLFormControlElement::detach();
+    HTMLElement::detach();
 }
 
 bool HTMLOptionElement::supportsFocus() const
@@ -109,12 +111,6 @@ bool HTMLOptionElement::isFocusable() const
     return supportsFocus() && renderStyle() && renderStyle()->display() != NONE;
 }
 
-const AtomicString& HTMLOptionElement::formControlType() const
-{
-    DEFINE_STATIC_LOCAL(const AtomicString, option, ("option"));
-    return option;
-}
-
 String HTMLOptionElement::text() const
 {
     Document* document = this->document();
@@ -191,7 +187,15 @@ int HTMLOptionElement::index() const
 
 void HTMLOptionElement::parseAttribute(Attribute* attr)
 {
-    if (attr->name() == selectedAttr) {
+    if (attr->name() == disabledAttr) {
+        bool oldDisabled = m_disabled;
+        m_disabled = !attr->isNull();
+        if (oldDisabled != m_disabled) {
+            setNeedsStyleRecalc();
+            if (renderer() && renderer()->style()->hasAppearance())
+                renderer()->theme()->stateChanged(renderer(), EnabledState);
+        }
+    } else if (attr->name() == selectedAttr) {
         // FIXME: This doesn't match what the HTML specification says.
         // The specification implies that removing the selected attribute or
         // changing the value of a selected attribute that is already present
@@ -200,7 +204,7 @@ void HTMLOptionElement::parseAttribute(Attribute* attr)
         // case; we'd need to do the other work from the setSelected function.
         m_isSelected = !attr->isNull();
     } else
-        HTMLFormControlElement::parseAttribute(attr);
+        HTMLElement::parseAttribute(attr);
 }
 
 String HTMLOptionElement::value() const
@@ -247,7 +251,7 @@ void HTMLOptionElement::childrenChanged(bool changedByParser, Node* beforeChange
 {
     if (HTMLSelectElement* select = ownerSelectElement())
         select->optionElementChildrenChanged();
-    HTMLFormControlElement::childrenChanged(changedByParser, beforeChange, afterChange, childCountDelta);
+    HTMLElement::childrenChanged(changedByParser, beforeChange, afterChange, childCountDelta);
 }
 
 HTMLSelectElement* HTMLOptionElement::ownerSelectElement() const
@@ -315,7 +319,7 @@ void HTMLOptionElement::insertedIntoTree(bool deep)
         select->scrollToSelection();
     }
 
-    HTMLFormControlElement::insertedIntoTree(deep);
+    HTMLElement::insertedIntoTree(deep);
 }
 
 String HTMLOptionElement::collectOptionInnerText() const
index 2eedadf1f14c45a297738ae7db34bf97f2c5fbd5..1433a82c818c409723cb81d86ded9146674cdf4a 100644 (file)
 #ifndef HTMLOptionElement_h
 #define HTMLOptionElement_h
 
-#include "HTMLFormControlElement.h"
+#include "HTMLElement.h"
 
 namespace WebCore {
 
 class HTMLSelectElement;
 
-class HTMLOptionElement : public HTMLFormControlElement {
+class HTMLOptionElement : public HTMLElement {
 public:
-    static PassRefPtr<HTMLOptionElement> create(Document*, HTMLFormElement*);
-    static PassRefPtr<HTMLOptionElement> create(const QualifiedName&, Document*, HTMLFormElement*);
+    static PassRefPtr<HTMLOptionElement> create(Document*);
+    static PassRefPtr<HTMLOptionElement> create(const QualifiedName&, Document*);
     static PassRefPtr<HTMLOptionElement> createForJSConstructor(Document*, const String& data, const String& value,
        bool defaultSelected, bool selected, ExceptionCode&);
 
@@ -54,7 +54,8 @@ public:
     String label() const;
     void setLabel(const String&);
 
-    bool ownElementDisabled() const { return HTMLFormControlElement::disabled(); }
+    virtual bool isEnabledFormControl() const OVERRIDE { return !disabled(); }
+    bool ownElementDisabled() const { return m_disabled; }
 
     virtual bool disabled() const;
 
@@ -63,7 +64,7 @@ public:
     void setSelectedState(bool);
 
 private:
-    HTMLOptionElement(const QualifiedName&, Document*, HTMLFormElement* = 0);
+    HTMLOptionElement(const QualifiedName&, Document*);
 
     virtual bool supportsFocus() const;
     virtual bool isFocusable() const;
@@ -72,8 +73,6 @@ private:
     virtual void detach();
     virtual void setRenderStyle(PassRefPtr<RenderStyle>);
 
-    virtual const AtomicString& formControlType() const;
-
     virtual void parseAttribute(Attribute*) OVERRIDE;
 
     virtual void insertedIntoTree(bool);
@@ -87,6 +86,7 @@ private:
 
     String m_value;
     String m_label;
+    bool m_disabled;
     bool m_isSelected;
     RefPtr<RenderStyle> m_style;
 };
index 74e2408620ecaf4ada65a66b6534dcd28f757266..4997653aa661cd488ce04af40629ebee17b5c90d 100644 (file)
@@ -92,7 +92,7 @@ nolayer interfaceName=HTMLElement
 object constructorNeedsFormElement, constructorNeedsCreatedByParser
 ol interfaceName=HTMLOListElement
 optgroup interfaceName=HTMLOptGroupElement
-option constructorNeedsFormElement
+option
 output constructorNeedsFormElement
 shadow interfaceName=HTMLShadowElement, conditional=SHADOW_DOM, runtimeConditional=shadowDOM
 p interfaceName=HTMLParagraphElement