Bug 72370 – [Gtk] display:none has no effect on <option> element

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antaryami Pandia]

The display:none style has no effect on <option> elements.

<select>
 <option>First choice</option>
 <option style="display: none">You must NOT see this</option>
 <option>Second choice</option>
</select>

Bug 8351 is the master bug of this.
Bug 49169 fixes the chromium port. This bug will fix the Gtk port.

[Antaryami Pandia]

Created attachment 115134 [details]
Proposed Patch

Proposed patch.
Not adding any test cases as the test cases added in bug 49169 (WebCore/manual-tests/display-none-option.html) is also valid for this bug.

[Martin Robinson]

This looks fine to me, but I'd also like cgarcia to take a look at it.

[Xan Lopez]

Comment on attachment 115134 [details]
Proposed Patch

This looks good to me, but I think you'll have to remove the "No new tests. (OOPS!)" thing or the patch will fail to land?

[Xan Lopez]

Comment on attachment 115134 [details]
Proposed Patch

Marking r- per the ChangeLog. cgarcia's comment is still welcome, but if you upload a new patch I think we can just land this!

[Antaryami Pandia]

Created attachment 115319 [details]
Updated patch

Updated patch with review comments.

[Carlos Garcia Campos]

Comment on attachment 115319 [details]
Updated patch

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

> Source/WebCore/platform/gtk/GtkPopupMenu.cpp:72
> -    gtk_widget_show(menuItem);
> +
> +    if (gtk_action_is_visible(action))
> +        gtk_widget_show(menuItem);

Instead of creating a new menu item that will never be shown (because display:none can't change while the popup menu is shown, right?), I think it would be better to simply not create the menu item, In PopupMenuGtk::show() you could do something like:

GRefPtr<GtkAction> action = adoptGRef(createGtkActionForMenuItem(i));
if (gtk_action_is_visible(action.get())
            m_popup->appendItem(action.get());

[Antaryami Pandia]

(In reply to comment #6)
> (From update of attachment 115319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115319&action=review
> 
> > Source/WebCore/platform/gtk/GtkPopupMenu.cpp:72
> > -    gtk_widget_show(menuItem);
> > +
> > +    if (gtk_action_is_visible(action))
> > +        gtk_widget_show(menuItem);
> 
> Instead of creating a new menu item that will never be shown (because display:none can't change while the popup menu is shown, right?), I think it would be better to simply not create the menu item, In PopupMenuGtk::show() you could do something like:
> 
> GRefPtr<GtkAction> action = adoptGRef(createGtkActionForMenuItem(i));
> if (gtk_action_is_visible(action.get())
>             m_popup->appendItem(action.get());

Yes.The thought behind the change was that the menu item is part of the select box and only its display property is none.For such case we just don't show the item.

And in fact I have also explored the option suggested by you earlier.But the problem was, after this piece code is executed we call m_popup->popUp(...) with the "size" parameter we got using client()->listSize(). This causes a mismatch and results in crash.

One way to resolve this is, we maintain a counter for number of items those are added and we pass this value.

const int counter = 0;
if (gtk_action_is_visible(action.get())) {
    m_popup->appendItem(action.get());
    counter++;
}
.......
m_popup->popUp(..., counter, ...);

Please provide your thought.I will modify accordingly.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 115319 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=115319&action=review
> > 
> > > Source/WebCore/platform/gtk/GtkPopupMenu.cpp:72
> > > -    gtk_widget_show(menuItem);
> > > +
> > > +    if (gtk_action_is_visible(action))
> > > +        gtk_widget_show(menuItem);
> > 
> > Instead of creating a new menu item that will never be shown (because display:none can't change while the popup menu is shown, right?), I think it would be better to simply not create the menu item, In PopupMenuGtk::show() you could do something like:
> > 
> > GRefPtr<GtkAction> action = adoptGRef(createGtkActionForMenuItem(i));
> > if (gtk_action_is_visible(action.get())
> >             m_popup->appendItem(action.get());
> 
> Yes.The thought behind the change was that the menu item is part of the select box and only its display property is none.For such case we just don't show the item.
> 
> And in fact I have also explored the option suggested by you earlier.But the problem was, after this piece code is executed we call m_popup->popUp(...) with the "size" parameter we got using client()->listSize(). This causes a mismatch and results in crash.

I see, in that case I think it's better to keep the item, even hidden, since it's part of the option menu after all. Your patch looks good to me too.

[Martin Robinson]

Comment on attachment 115319 [details]
Updated patch

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

> Source/WebCore/ChangeLog:11
> +        * platform/gtk/GtkPopupMenu.cpp:
> +        (WebCore::GtkPopupMenu::appendItem):
> +        * platform/gtk/PopupMenuGtk.cpp:
> +        (WebCore::PopupMenuGtk::createGtkActionForMenuItem):

In the future, please remember to fill these in.

[WebKit Review Bot]

Comment on attachment 115319 [details]
Updated patch

Clearing flags on attachment: 115319

Committed r100470: <http://trac.webkit.org/changeset/100470>