Update <select> when any of its <option> children has "display: none"

Source/core/rendering/RenderListBox.cpp

Index: Source/core/rendering/RenderListBox.cpp
diff --git a/Source/core/rendering/RenderListBox.cpp b/Source/core/rendering/RenderListBox.cpp
index 62ea8e322834d54801574a75c7bcb723efb72a75..17804e8d3676ea2159f77a7cc840f2cce79ac04e 100644
--- a/Source/core/rendering/RenderListBox.cpp
+++ b/Source/core/rendering/RenderListBox.cpp
@@ -119,14 +119,20 @@ inline HTMLSelectElement* RenderListBox::selectElement() const
 void RenderListBox::updateFromElement()
 {
     FontCachePurgePreventer fontCachePurgePreventer;
-
     if (m_optionsChanged) {
         const Vector<HTMLElement*>& listItems = selectElement()->listItems();
-        int size = numItems();
+        int size = listItems.size();
 
         float width = 0;
+
+        m_listItemIndex.clear();
         for (int i = 0; i < size; ++i) {
             HTMLElement* element = listItems[i];
+            RenderStyle* listItemStyle = element->renderStyle();
+            if (listItemStyle && listItemStyle->display() == NONE)
+                continue;
+
+            m_listItemIndex.append(i);
             String text;
             Font itemFont = style()->font();
             if (isHTMLOptionElement(*element)) {
@@ -202,8 +208,9 @@ void RenderListBox::scrollToRevealSelection()
 
     m_scrollToRevealSelectionAfterLayout = false;
 
-    int firstIndex = select->activeSelectionStartListIndex();
-    if (firstIndex >= 0 && !listIndexIsVisible(select->activeSelectionEndListIndex()))
+    int firstIndex = static_cast<int>(m_listItemIndex.find(select->activeSelectionStartListIndex()));
+    int lastIndex = static_cast<int>(m_listItemIndex.find(select->activeSelectionEndListIndex()));

[esprehn, 2014/03/21 20:43:39]

Why can't we just iterate the set of items and only consider the visible ones?

[spartha, 2014/03/21 21:29:03]

As against using the vector? We can do that I suppose. But then we would have to
loop through the item list each time we need to get the element from the display
order (which may not be the same as the item list in Select). It comes down to
the choice between speed v/s space. I have yet to come across select with huge
number of items. So if space is a concern I can try the other approach.
Thoughts? 

+    if (firstIndex >= 0 && !listIndexIsVisible(lastIndex))
         scrollToRevealElementAtListIndex(firstIndex);
 }
 
@@ -262,7 +269,7 @@ int RenderListBox::numVisibleItems() const
 
 int RenderListBox::numItems() const
 {
-    return selectElement()->listItems().size();
+    return m_listItemIndex.size();
 }
 
 LayoutUnit RenderListBox::listHeight() const
@@ -351,7 +358,7 @@ void RenderListBox::addFocusRingRects(Vector<IntRect>& rects, const LayoutPoint&
     int size = numItems();
     const Vector<HTMLElement*>& listItems = select->listItems();
     for (int i = 0; i < size; ++i) {
-        HTMLElement* element = listItems[i];
+        HTMLElement* element = listItems[m_listItemIndex[i]];

[esprehn, 2014/03/21 20:43:39]

ditto.

         if (isHTMLOptionElement(*element) && !element->isDisabledFormControl()) {
             rects.append(pixelSnappedIntRect(itemBoundingBoxRect(additionalOffset, i)));
             return;
@@ -408,7 +415,7 @@ void RenderListBox::paintItemForeground(PaintInfo& paintInfo, const LayoutPoint&
     HTMLSelectElement* select = selectElement();
 
     const Vector<HTMLElement*>& listItems = select->listItems();
-    HTMLElement* element = listItems[listIndex];
+    HTMLElement* element = listItems[m_listItemIndex[listIndex]];
 
     RenderStyle* itemStyle = element->renderStyle();
     if (!itemStyle)
@@ -457,7 +464,7 @@ void RenderListBox::paintItemForeground(PaintInfo& paintInfo, const LayoutPoint&
 void RenderListBox::paintItemBackground(PaintInfo& paintInfo, const LayoutPoint& paintOffset, int listIndex)
 {
     const Vector<HTMLElement*>& listItems = selectElement()->listItems();
-    HTMLElement* element = listItems[listIndex];
+    HTMLElement* element = listItems[m_listItemIndex[listIndex]];
 
     Color backColor;
     if (isHTMLOptionElement(*element) && ((toHTMLOptionElement(*element).selected() && selectElement()->suggestedIndex() < 0) || listIndex == selectElement()->suggestedIndex())) {
@@ -589,9 +596,9 @@ void RenderListBox::autoscroll(const IntPoint&)
         m_inAutoscroll = true;
 
         if (!select->multiple())
-            select->setActiveSelectionAnchorIndex(endIndex);
+            select->setActiveSelectionAnchorIndex(m_listItemIndex[endIndex]);
 
-        select->setActiveSelectionEndIndex(endIndex);
+        select->setActiveSelectionEndIndex(m_listItemIndex[endIndex]);
         select->updateListBoxSelection(!select->multiple());
         m_inAutoscroll = false;
     }
@@ -718,7 +725,7 @@ bool RenderListBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& re
 
     for (int i = 0; i < size; ++i) {

[esprehn, 2014/03/21 20:43:39]

Since we're walking all of them anyway, why can't we just skip display: none
ones?

         if (itemBoundingBoxRect(adjustedLocation, i).contains(locationInContainer.point())) {
-            if (Element* node = listItems[i]) {
+            if (Element* node = listItems[m_listItemIndex[i]]) {
                 result.setInnerNode(node);
                 if (!result.innerNonSharedNode())
                     result.setInnerNonSharedNode(node);