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

Source/web/ExternalPopupMenu.cpp

 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 36 matching lines @@
 using namespace blink;
 
 namespace blink {
 
 ExternalPopupMenu::ExternalPopupMenu(LocalFrame& frame, PopupMenuClient* popupMenuClient, WebViewImpl& webView)
     : m_popupMenuClient(popupMenuClient)
     , m_frameView(frame.view())
     , m_webView(webView)
     , m_dispatchEventTimer(this, &ExternalPopupMenu::dispatchEvent)
     , m_webExternalPopupMenu(0)
+    , m_itemCount(0)
 {
 }
 
 ExternalPopupMenu::~ExternalPopupMenu()
 {
 }
 
 void ExternalPopupMenu::show(const FloatQuad& controlPosition, const IntSize&, int index)
 {
     IntRect rect(controlPosition.enclosingBoundingBox());
@@ skipping 52 matching lines @@
 
 void ExternalPopupMenu::disconnectClient()
 {
     hide();
     m_popupMenuClient = 0;
 }
 
 void ExternalPopupMenu::didChangeSelection(int index)
 {
     if (m_popupMenuClient)
-        m_popupMenuClient->selectionChanged(index);
+        m_popupMenuClient->selectionChanged(toPopupMenuItemIndex(index));
 }
 
 void ExternalPopupMenu::didAcceptIndex(int index)
 {
     // Calling methods on the PopupMenuClient might lead to this object being
     // derefed. This ensures it does not get deleted while we are running this
     // method.
+    int popupMenuItemIndex = toPopupMenuItemIndex(index);
     RefPtr<ExternalPopupMenu> guard(this);
 
     if (m_popupMenuClient) {
         m_popupMenuClient->popupDidHide();
-        m_popupMenuClient->valueChanged(index);
+        m_popupMenuClient->valueChanged(popupMenuItemIndex);
     }
     m_webExternalPopupMenu = 0;
 }
 
 void ExternalPopupMenu::didAcceptIndices(const WebVector<int>& indices)
 {
     if (!m_popupMenuClient) {
         m_webExternalPopupMenu = 0;
         return;
     }
 
     // Calling methods on the PopupMenuClient might lead to this object being
     // derefed. This ensures it does not get deleted while we are running this
     // method.
     RefPtr<ExternalPopupMenu> protect(this);
 
     if (!indices.size())
         m_popupMenuClient->valueChanged(static_cast<unsigned>(-1), true);
     else {
         for (size_t i = 0; i < indices.size(); ++i)
-            m_popupMenuClient->listBoxSelectItem(indices[i], (i > 0), false, (i == indices.size() - 1));
+            m_popupMenuClient->listBoxSelectItem(toPopupMenuItemIndex(indices[i]), (i > 0), false, (i == indices.size() - 1));
     }
 
     // The call to valueChanged above might have lead to a call to
     // disconnectClient, so we might not have a PopupMenuClient anymore.
     if (m_popupMenuClient)
         m_popupMenuClient->popupDidHide();
 
     m_webExternalPopupMenu = 0;
 }
 
 void ExternalPopupMenu::didCancel()
 {
     // See comment in didAcceptIndex on why we need this.
     RefPtr<ExternalPopupMenu> guard(this);
 
     if (m_popupMenuClient)
         m_popupMenuClient->popupDidHide();
     m_webExternalPopupMenu = 0;
 }
 
 void ExternalPopupMenu::getPopupMenuInfo(WebPopupMenuInfo* info)
 {
     int itemCount = m_popupMenuClient->listSize();
+    m_itemCount = 0;
     WebVector<WebMenuItemInfo> items(static_cast<size_t>(itemCount));
     for (int i = 0; i < itemCount; ++i) {
-        WebMenuItemInfo& popupItem = items[i];
+        PopupMenuStyle style = m_popupMenuClient->itemStyle(i);
+        if (style.isDisplayNone())
+            continue;
+
+        WebMenuItemInfo& popupItem = items[m_itemCount++];
         popupItem.label = m_popupMenuClient->itemText(i);
         popupItem.toolTip = m_popupMenuClient->itemToolTip(i);
         if (m_popupMenuClient->itemIsSeparator(i))
             popupItem.type = WebMenuItemInfo::Separator;
         else if (m_popupMenuClient->itemIsLabel(i))
             popupItem.type = WebMenuItemInfo::Group;
         else
             popupItem.type = WebMenuItemInfo::Option;
         popupItem.enabled = m_popupMenuClient->itemIsEnabled(i);
         popupItem.checked = m_popupMenuClient->itemIsSelected(i);
-        PopupMenuStyle style = m_popupMenuClient->itemStyle(i);
         if (style.textDirection() == blink::RTL)
             popupItem.textDirection = WebTextDirectionRightToLeft;
         else
             popupItem.textDirection = WebTextDirectionLeftToRight;
         popupItem.hasTextDirectionOverride = style.hasTextDirectionOverride();
     }
 
     info->itemHeight = m_popupMenuClient->menuStyle().font().fontMetrics().height();
     info->itemFontSize = static_cast<int>(m_popupMenuClient->menuStyle().font().fontDescription().computedSize());
-    info->selectedIndex = m_popupMenuClient->selectedIndex();
+    info->selectedIndex = toExternalPopupMenuItemIndex(m_popupMenuClient->selectedIndex());
     info->rightAligned = m_popupMenuClient->menuStyle().textDirection() == blink::RTL;
     info->allowMultipleSelection = m_popupMenuClient->multiple();
+    if (m_itemCount < itemCount)
+        items.shrink(m_itemCount);
     info->items.swap(items);
 }
 
+int ExternalPopupMenu::toPopupMenuItemIndex(int externalPopupMenuItemIndex)
+{
+    ASSERT(m_popupMenuClient);
+    int itemCount = m_popupMenuClient->listSize();
+    if (itemCount == m_itemCount || externalPopupMenuItemIndex < 0)

[keishi, 2014/08/06 03:00:20]

I get what you're trying to do with itemCount == m_itemCount but I worry it
might make the behavior  hard to predict for web developers debugging their
apps.
This should only be called sparingly so maybe we don't need this.

[spartha, 2014/08/06 08:13:31]

Done.

+        return externalPopupMenuItemIndex;
+
+    int indexTracker = 0;
+    for (int i = 0; i < itemCount ; ++i) {
+        if (m_popupMenuClient->itemStyle(i).isDisplayNone())
+            continue;
+        if (indexTracker++ == externalPopupMenuItemIndex)
+            return i;
+    }
+    return -1;
 }
+
+int ExternalPopupMenu::toExternalPopupMenuItemIndex(int popupMenuItemIndex)
+{
+    ASSERT(m_popupMenuClient);
+    int itemCount = m_popupMenuClient->listSize();
+    if (itemCount == m_itemCount || popupMenuItemIndex < 0)
+        return popupMenuItemIndex;
+
+    int indexTracker = 0;
+    for (int i = 0; i < itemCount; ++i) {
+        if (m_popupMenuClient->itemStyle(i).isDisplayNone())
+            continue;
+        if (popupMenuItemIndex == i)
+            return indexTracker;
+        ++indexTracker;
+    }
+    return -1;
+}
+
+}