Option Groups with display: none should not show the children option elements

Source/core/rendering/RenderMenuList.cpp

 /*
  * This file is part of the select element renderer in WebCore.
  *
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  *               2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 484 matching lines @@
         // Try to retrieve the style of an option element we know exists (index 0).
         listIndex = 0;
     }
     HTMLElement* element = listItems[listIndex];
 
     Color itemBackgroundColor;
     bool itemHasCustomBackgroundColor;
     getItemBackgroundColor(listIndex, itemBackgroundColor, itemHasCustomBackgroundColor);
 
     RenderStyle* style = element->renderStyle() ? element->renderStyle() : element->computedStyle();
+    bool displayNone = style && style->display() == NONE;

[keishi, 2014/08/01 02:01:11]

This nested display:none case is already handled correctly for listboxes, so we
just need to fix the menu list popup.

So I think this change to RenderMenuList.cpp should be all you need. You
shouldn't need to change HTMLSelectElement.cpp and HTMLOptionElement.cpp.

But there is a problem on Mac. ExternalPopupMenu ignores
PopupMenuStyle::isDisplayNone. We would need to fix that too.

[spartha, 2014/08/01 09:29:16]

Thanks for the review! Even I felt that it would be sufficient. But the
RenderMenuList change will affect only when the popup is shown. But the test
case in the patch will fail without the HTMLSelectElement.cpp and
HTMLOptionElement.cpp changes. The test case just sets the focus on the select
and uses keyup/down to select an option. 
I haven't tried the patch on Mac. Let me try it on the bots first (I don't have
access to a Mac :( )

+    if (!displayNone) {
+        HTMLElement* parent = Traversal<HTMLElement>::firstAncestor(*element);
+        ASSERT(parent);
+        if (parent->hasTagName(optgroupTag)) {
+            RenderStyle* parentStyle = parent->renderStyle() ? parent->renderStyle() : parent->computedStyle();
+            if (parentStyle && parentStyle->display() == NONE)
+                displayNone = true;
+        }
+    }
     return style ? PopupMenuStyle(resolveColor(style, CSSPropertyColor), itemBackgroundColor, style->font(), style->visibility() == VISIBLE,
-        style->display() == NONE, style->textIndent(), style->direction(), isOverride(style->unicodeBidi()),
+        displayNone, style->textIndent(), style->direction(), isOverride(style->unicodeBidi()),
         itemHasCustomBackgroundColor ? PopupMenuStyle::CustomBackgroundColor : PopupMenuStyle::DefaultBackgroundColor) : menuStyle();
 }
 
 void RenderMenuList::getItemBackgroundColor(unsigned listIndex, Color& itemBackgroundColor, bool& itemHasCustomBackgroundColor) const
 {
     const WillBeHeapVector<RawPtrWillBeMember<HTMLElement> >& listItems = selectElement()->listItems();
     if (listIndex >= listItems.size()) {
         itemBackgroundColor = resolveColor(CSSPropertyBackgroundColor);
         itemHasCustomBackgroundColor = false;
         return;
@@ skipping 86 matching lines @@
     HTMLElement* element = listItems[listIndex];
     return isHTMLOptionElement(*element) && toHTMLOptionElement(*element).selected();
 }
 
 void RenderMenuList::setTextFromItem(unsigned listIndex)
 {
     setTextFromOption(selectElement()->listToOptionIndex(listIndex));
 }
 
 }