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>
It seems that style support is not yet present in WK2 WebPopupItem.
See also: bug 8351.
Created attachment 193470 [details] Demonstrates the bug through use of classes to apply display:none Test-case from issue #8351.
Created attachment 193630 [details] Patch
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.
Created attachment 194037 [details] Patch
Which patch needs review exactly? Please obsolete the old one and make sure to provide ChangeLog entry for the one to review.
Created attachment 194060 [details] Patch
Philippe, Please this patch (id=194060). But im not sure about WebKit2.order changes as for linking for Mac builds.
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?
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.
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
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.
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.
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.
Created attachment 202313 [details] Patch
Any news?
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.
(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 :)).
Any update on the patch status?
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!
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 =)
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.
Created attachment 379552 [details] WIP Patch v2 This should make the Mac bots happy
Created attachment 382456 [details] WIP Patch v3 Let's see if third time is the charm and the Mac bots will be happy now :-)
Created attachment 387506 [details] WIP Patch v4
(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
(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> :)
(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>.
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.
(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!)