Bug 92833 – REGRESSION(r102741): [Forms] In selects, when disabled, browser skips first option if not in optgroup, then selects first option in optgroup

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

The select element should pick up not "disable" option element for selected option when there is no "selected" option.

HTMLSelectElement::recalcListItems() does this by checking HTMLOptionElement::option().

It seems implementation of HTMLOPtionElement::option() isn't matched to HTML5 specification[1]:

  An option element is disabled if its disabled attribute is present or 
  if it is a child of an optgroup element whose disabled attribute is present.

However, current implementation checks "disabled" attribute regardless element tag name:
  bool HTMLOptionElement::disabled() const
  {
      return ownElementDisabled() || (parentNode() && parentNode()->isHTMLElement() && static_cast<HTMLElement*>(parentNode())->disabled());
  }

In sample URI[2], the parent element is the "select" element having "disabled" attribute. So, HTMLOptionElement::disable() return true
rather than false. This is the reason we pick up the first "option" element in the first "optgroup" element.

= References =
[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-option-element
[2] http://jsfiddle.net/UZBKd/

This bug is imported from http://crbug.com/139823


The sample URI, http://jsfiddle.net/UZBKd/, should display "1" as other browsers, FireFox, IE, and Opera.

Created attachment 155978 [details]
Patch 1

Comment on attachment 155978 [details]
Patch 1

Could you review this patch?
Thanks in advance.

[Kent Tamura]

Comment on attachment 155978 [details]
Patch 1

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

Need ChangeLog improvement

> Source/WebCore/ChangeLog:9
> +        This patch changes implementation of HTMLOptionElement::disabled() to
> +        follow HTML5 specification, the option element is disabled if option

This is confusing.  It's unclear that what part of the HTML5 specification it is.
You should write ":disabled pseudo class" or add a link to the concept "disabled" [1] because "disabled" IDL attribute is different from it.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#concept-option-disabled

> Source/WebCore/ChangeLog:15
> +        Before this patch, HTMLSelectElement::recalcListItems() didn't pick

What do you mean by 'pick'?

Created attachment 155986 [details]
Patch 2

Comment on attachment 155986 [details]
Patch 2

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Update Source/WebCore/ChangeLog as suggested.

[Kent Tamura]

Comment on attachment 155986 [details]
Patch 2

looks good.

Comment on attachment 155986 [details]
Patch 2

Clearing flags on attachment: 155986

Committed r124416: <http://trac.webkit.org/changeset/124416>

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

This causes fast/forms/basic-selects.html to fail on Mac ports.

I see the commit log says "Note: We need to rebaseline for all ports expect for Chromium-Linux.". Did this ever happen?

(In reply to comment #10)
> This causes fast/forms/basic-selects.html to fail on Mac ports.
> 
> I see the commit log says "Note: We need to rebaseline for all ports expect for Chromium-Linux.". Did this ever happen?

No, I updated for Chromium ports, but not for others.
Could you update expected texts and images for Mac port?
Thanks in advance.