Bug 81882

Summary: [GTK] display:none has no effect on <option> element
Product: WebKit Reporter: Antaryami Pandia (apandia) <antaryami.pandia>
Component: CSSAssignee: Adrian Perez <aperez>
Status: ASSIGNED ---    
Severity: Normal CC: achristensen, aestes, alexey.verlinger, aperez, ap, beidson, benjamin, bugs-noreply, calvaris, cdumez, cgarcia, clopez, darin, dino, ews-watchlist, mcatanzaro, mmaxfield, oleg_smirnov, pnormand, shanestephens, tregoning
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=8351
https://bugs.webkit.org/show_bug.cgi?id=203606
https://bugs.webkit.org/show_bug.cgi?id=206347
Bug Depends on: 81883    
Bug Blocks:    
Attachments:
Description Flags
Demonstrates the bug through use of classes to apply display:none
none
Patch
none
Patch
none
Patch
none
Patch
none
WIP Patch
none
WIP Patch v2
none
WIP Patch v3
none
WIP Patch v4 aperez: review?

Description Antaryami Pandia (apandia) 2012-03-22 01:57:57 PDT
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>
Comment 1 Antaryami Pandia (apandia) 2012-03-22 02:01:31 PDT
It seems that style support is not yet present in WK2 WebPopupItem.
Comment 2 Alexey Proskuryakov 2012-11-28 17:04:21 PST
See also: bug 8351.
Comment 3 Oleg Smirnov 2013-03-17 11:19:03 PDT
Created attachment 193470 [details]
Demonstrates the bug through use of classes to apply display:none

Test-case from issue #8351.
Comment 4 Oleg Smirnov 2013-03-18 12:43:12 PDT
Created attachment 193630 [details]
Patch
Comment 5 Oleg Smirnov 2013-03-19 00:17:56 PDT
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.
Comment 6 Oleg Smirnov 2013-03-20 06:28:11 PDT
Created attachment 194037 [details]
Patch
Comment 7 Philippe Normand 2013-03-20 07:48:40 PDT
Which patch needs review exactly? Please obsolete the old one and make sure to provide ChangeLog entry for the one to review.
Comment 8 Oleg Smirnov 2013-03-20 08:04:41 PDT
Created attachment 194060 [details]
Patch
Comment 9 Oleg Smirnov 2013-03-20 08:07:21 PDT
Philippe,
Please this patch (id=194060).
But im not sure about WebKit2.order changes as for linking for Mac builds.
Comment 10 Philippe Normand 2013-03-20 08:18:54 PDT
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?
Comment 11 Oleg Smirnov 2013-03-20 22:00:44 PDT
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.
Comment 12 Oleg Smirnov 2013-03-20 22:02:06 PDT
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
Comment 13 Oleg Smirnov 2013-03-29 22:34:51 PDT
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.
Comment 14 Alexey Proskuryakov 2013-05-14 09:36:42 PDT
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.
Comment 15 Oleg Smirnov 2013-05-20 13:10:48 PDT
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.
Comment 16 Oleg Smirnov 2013-05-20 13:21:48 PDT
Created attachment 202313 [details]
Patch
Comment 17 Oleg Smirnov 2014-08-02 14:02:46 PDT
Any news?
Comment 18 Benjamin Poulain 2014-08-02 14:17:48 PDT
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.
Comment 19 Benjamin Poulain 2014-08-02 14:22:50 PDT
(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 :)).
Comment 20 John Tregoning 2014-11-19 15:17:44 PST
Any update on the patch status?
Comment 21 Adrian Perez 2018-05-31 06:30:32 PDT
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!
Comment 22 Adrian Perez 2019-09-24 15:39:16 PDT
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 =)
Comment 23 Adrian Perez 2019-09-24 15:43:46 PDT
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.
Comment 24 Adrian Perez 2019-09-25 09:04:15 PDT
Created attachment 379552 [details]
WIP Patch v2

This should make the Mac bots happy
Comment 25 Adrian Perez 2019-10-31 07:24:00 PDT
Created attachment 382456 [details]
WIP Patch v3

Let's see if third time is the charm and the Mac
bots will be happy now :-)
Comment 26 Adrian Perez 2020-01-13 00:57:41 PST
Created attachment 387506 [details]
WIP Patch v4
Comment 27 Carlos Alberto Lopez Perez 2020-01-14 19:45:21 PST
(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
Comment 28 Adrian Perez 2020-01-16 01:50:07 PST
(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> :)
Comment 29 Adrian Perez 2020-01-16 04:40:53 PST
(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>.
Comment 30 Adrian Perez 2020-01-16 05:44:32 PST
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.
Comment 31 Adrian Perez 2020-01-16 09:04:53 PST
(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!)