Bug 92833 - REGRESSION(r102741): [Forms] In selects, when disabled, browser skips first option if not in optgroup, then selects first option in optgroup
Summary: REGRESSION(r102741): [Forms] In selects, when disabled, browser skips first o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL: http://jsfiddle.net/UZBKd/
Keywords:
Depends on: 93538
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-31 20:46 PDT by yosin
Modified: 2012-08-08 20:50 PDT (History)
8 users (show)

See Also:


Attachments
Patch 1 (89.60 KB, patch)
2012-08-01 23:11 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (89.83 KB, patch)
2012-08-02 00:02 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-07-31 20:46:53 PDT
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/
Comment 1 yosin 2012-07-31 20:53:18 PDT
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.
Comment 2 yosin 2012-08-01 23:11:39 PDT
Created attachment 155978 [details]
Patch 1
Comment 3 yosin 2012-08-01 23:14:13 PDT
Comment on attachment 155978 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 4 Kent Tamura 2012-08-01 23:55:30 PDT
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'?
Comment 5 yosin 2012-08-02 00:02:51 PDT
Created attachment 155986 [details]
Patch 2
Comment 6 yosin 2012-08-02 00:03:56 PDT
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.
Comment 7 Kent Tamura 2012-08-02 00:12:56 PDT
Comment on attachment 155986 [details]
Patch 2

looks good.
Comment 8 yosin 2012-08-02 00:22:58 PDT
Comment on attachment 155986 [details]
Patch 2

Clearing flags on attachment: 155986

Committed r124416: <http://trac.webkit.org/changeset/124416>
Comment 9 yosin 2012-08-02 00:23:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Dean Jackson 2012-08-08 15:22:45 PDT
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?
Comment 11 yosin 2012-08-08 20:50:03 PDT
(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.