Bug 81882 – [GTK] display:none has no effect on <option> element

• Status: ASSIGNED
• Component: CSS

[Antaryami Pandia (apandia)]

While walking through WK2 GTK port found that, the issue "display:none" on <option> elements is still present.

The issue was fixed in GTK WK1 in https://bugs.webkit.org/show_bug.cgi?id=72370

Reproduction for the issue:-
<select>
 <option>First choice</option>
 <option style="display: none">You must NOT see this</option>
 <option>Second choice</option>
</select>

[Antaryami Pandia (apandia)]

It seems that style support is not yet present in WK2 WebPopupItem.

[Alexey Proskuryakov]

See also: bug 8351.

[Oleg Smirnov]

Created attachment 193470 [details]
Demonstrates the bug through use of classes to apply display:none

Test-case from issue #8351.

[Oleg Smirnov]

Created attachment 193630 [details]
Patch

[Oleg Smirnov]

Alexey,
Could you help me about generation WebKit2.order file with Mac functions for XCode?
As you can see build is failed for Mac platforms.

Thanks.

[Oleg Smirnov]

Created attachment 194037 [details]
Patch

[Philippe Normand]

Which patch needs review exactly? Please obsolete the old one and make sure to provide ChangeLog entry for the one to review.

[Oleg Smirnov]

Created attachment 194060 [details]
Patch

[Oleg Smirnov]

Philippe,
Please this patch (id=194060).
But im not sure about WebKit2.order changes as for linking for Mac builds.

[Philippe Normand]

Comment on attachment 194060 [details]
Patch

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

Sorry I don't think I can review this entire patch, I'll CC an OWNER and a WK2/GTK reviewer. Is there a layout test fixed by this patch?

> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:71
> +        g_signal_connect(m_popup->platformMenu(), "unmap", G_CALLBACK(menuUnmapped), this);

Is this signal disconnected at some point?

[Oleg Smirnov]

Yes it is normal, disconnect is not needed. I made these changes like in WebCore/platform/gtk/PopupMenuGtk.cpp for WK1.
It's issue for WebPopupMenuProxyGtk.cpp, popup will be clossed immediately in this case.

[Oleg Smirnov]

And test case in first attachment:
https://bugs.webkit.org/attachment.cgi?id=193470

./run-launcher --gtk -2 https://bugs.webkit.org/attachment.cgi?id=193470

[Oleg Smirnov]

Hi folks,
How about helps with Mac build? After these canges all build tree will be green...Please help me, after this commit i will working for related to this issue.

[Alexey Proskuryakov]

This patch touches a lot of cross-platform code, so it should not have any bracketed prefixes in title. The bug currently has the prefix "[Gtk] [WK2]", which means that it only touches Gtk code in WebKit2 subproject, which is not accurate. The bracketed prefixes are meant to help people who work on other ports easily skip uninteresting patches.

I haven't looked into the patch deeply, but if you need to export a new function from WebCore to use it in WebKit on Mac, it needs to be added to WebCore.exp.in. Please don't modify the order files - changing them doesn't cause any direct harm, but is useless, as they will get regenerated from scratch by Apple engineers when needed.

[Oleg Smirnov]

Alexey, yes of cause this bug only for GTK WK2. Im wonder why you change title of issue from GTK WK2 to only GTK. As you know issue for GTK WK1 was already fixed in https://bugs.webkit.org/show_bug.cgi?id=72370.

For this issue changes should touches some code not from GTK WK2 directory.
Patch has no hard logic, only new classes for encode/decode and it is good way fix it by these changs.

I have no Mac for understanding linking stage. And ican not fing any info about exporting symbols for Mac. I think WebCore.exp.in not for these changes.

[Oleg Smirnov]

Created attachment 202313 [details]
Patch

[Oleg Smirnov]

Any news?

[Benjamin Poulain]

Comment on attachment 202313 [details]
Patch

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

r-. Rationale: You need a good explanation in the ChangeLog. You should have layout tests or an explanation why this is not tested.

> Source/WebCore/platform/PopupMenuStyle.h:43
> +        , m_textDirection(RTL)

Default to RTL? That's odd.

> Source/WebKit2/ChangeLog:9
> +        Added PopupMenuStyle as styling for WebPopupMenu.
> +        It's needed for support display:none property on GTK WK2 popu menu style.

Typo: "popu menu style".

This change log is not good enough. First, you need to explain what the bug is, why it happens, how it happens, etc. Then you explain how you solve it.
Finally you describe the implementation details, you can use the fields bellow to give information for each function.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:898
> +    encoder << static_cast<int>(font.letterSpacing());
> +    encoder << static_cast<int>(font.wordSpacing());

Do not convert floating point values to integer across the IPC, that is an invitation for bugs!

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:928
> +void ArgumentCoder<FontDescription>::encode(ArgumentEncoder& encoder, const FontDescription& fontDescription)
> +{
> +    // FIXME: Add support for styling the font.
> +}
> +
> +bool ArgumentCoder<FontDescription>::decode(ArgumentDecoder& decoder, FontDescription& fontDescription)
> +{
> +    fontDescription = FontDescription();
> +    return true;
> +}

Don't add code that does not do anything. You can add the font description later with a proper implementation in a follow up patch.

[Benjamin Poulain]

(In reply to comment #17)
> Any news?

Sorry this patch slipped through the cracks of the review queue. The review queue (http://webkit.org/pending-review) has a huge backlog and patches are easily ignored.

When one of your patch is ignored for more than a week, you should ping reviewers on IRC. It may seem impolite but that's the only thing that works (don't harass reviewers either :)).

[John Tregoning]

Any update on the patch status?

[Adrian Perez]

Oleg, would you be open to rebase and re-upload the patch with
the review comments solved? If you don't have the time, I would
like to continue where you left it, if you are okay with that.
Thanks!

[Adrian Perez]

Created attachment 379506 [details]
WIP Patch


I took the patch made by Oleg years ago and brought it up-to-date; with
some parts like the WebPopupMenuProxyGtk code written anew because many
things changed a lot since 2014.

This still needs some layout test, which I have yet to figure out how to
do, but other than that I think the code is ready for reviewers to start
taking a look at =)

[Adrian Perez]

Also, do we want to add a new WebPopupItemStyle instead of using
PopupMenuStyle directly as attempted in bug #81883? If yes, I could
bring the patch in that related bug up-to-data as well.

[Adrian Perez]

Created attachment 379552 [details]
WIP Patch v2

This should make the Mac bots happy

[Adrian Perez]

Created attachment 382456 [details]
WIP Patch v3

Let's see if third time is the charm and the Mac
bots will be happy now :-)

[Adrian Perez]

Created attachment 387506 [details]
WIP Patch v4

[Carlos Alberto Lopez Perez]

(In reply to Adrian Perez from comment #22)
> 
> This still needs some layout test, which I have yet to figure out how to
> do, but other than that I think the code is ready for reviewers to start
> taking a look at =)

Something like this should work:

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

# reftest:
<select size="4">
 <option>First choice</option>
 <option>Second choice</option>
</select>


# It just sets the select with 4 visible options elements, and displays 3.. so it can be easily tested as a reftest.
This works both on chrome and firefox ... But it doesn't seem to work here with this patch, so there is something to fix here still (I guess).


There its also a WPT test that tests a similar thing (I believe). It's the test:

layout-test: imported/w3c/web-platform-tests/css/css-display/select-4-option-optgroup-display-none.html
in WPT: http://wpt.live/css/css-display/select-4-option-optgroup-display-none.html

This test its currently marked as failing and assigned to bug 203606

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #27)
> (In reply to Adrian Perez from comment #22)
> > 
> > This still needs some layout test, which I have yet to figure out how to
> > do, but other than that I think the code is ready for reviewers to start
> > taking a look at =)
> 
> Something like this should work:
> 
> # test:
> <select size="4">
>  <option>First choice</option>
>  <option style="display: none">You must NOT see this</option>
>  <option>Second choice</option>
> </select>
> 
> # reftest:
> <select size="4">
>  <option>First choice</option>
>  <option>Second choice</option>
> </select>
>
> # It just sets the select with 4 visible options elements, and displays 3..
> so it can be easily tested as a reftest.
> This works both on chrome and firefox ... But it doesn't seem to work here
> with this patch, so there is something to fix here still (I guess).

Thanks, I will add a test based on this!
 
> There its also a WPT test that tests a similar thing (I believe). It's the
> test:
> 
> layout-test:
> imported/w3c/web-platform-tests/css/css-display/select-4-option-optgroup-
> display-none.html
> in WPT:
> http://wpt.live/css/css-display/select-4-option-optgroup-display-none.html
> 
> This test its currently marked as failing and assigned to bug 203606

Running this WPT test I see that it passed partially: the <option> elements
are correctly hidden, but “display:none” is not respected for <optgroup>.
That is to be expected because this patch only fixes the issue for <option>.

I think we can land this with a reftest added for <option>, and create
a new bug to work on fixing <optgroup> :)

[Adrian Perez]

(In reply to Adrian Perez from comment #28)

> [...]
>
> I think we can land this with a reftest added for <option>, and create
> a new bug to work on fixing <optgroup> :)

Done, filed bug #206347 for <optgroup>.

[Adrian Perez]

While working on a test case, I noticed that there is one case in
which “display:none” is still not working, even with the WIP patch
applied. If the “size” property of the enclosing <select> element is
higher than 1 (causing the control to be a list instead of a popup),
the <option> is still shown. In the following example:

  <select size="2">
     <option>visible</option>
     <option style="display:none">invisible</option>
  </select>

both options will be shown in the selection box. Note that the patch
does work with <select size="1">. I will take a look and see what
needs to make the test above pass.

[Adrian Perez]

(In reply to Adrian Perez from comment #30)
> While working on a test case, I noticed that there is one case in
> which “display:none” is still not working, even with the WIP patch
> applied. If the “size” property of the enclosing <select> element is
> higher than 1 (causing the control to be a list instead of a popup),
> the <option> is still shown. In the following example:
> 
>   <select size="2">
>      <option>visible</option>
>      <option style="display:none">invisible</option>
>   </select>
> 
> both options will be shown in the selection box. Note that the patch
> does work with <select size="1">. I will take a look and see what
> needs to make the test above pass.

Some investigation: the RenderListBox::paintObject() and ::paintItem()
is where the selection box is taken care of. The whole painting logic
there seems to assume that all the child elements are to be painted,
without accounting those hidden ones (e.g. using “display:none”).

Some observations, so I don't have to wrap my head around them next
time I look at the code:

 - RenderListBox::numItems() returns the child count of HTMLSelectElement
   regardless of whether they are to be painted or not (i.e. display:none).

 - RenderListBox::numVisibleItems() returns the amount of items which can
   be painted inside the box occupied by rendered element, *NOT* the number
   of items of the HTMLSelectElement which are to be painted (gotcha!)