72370 2011-11-15 02:58:28 -0800 [Gtk] display:none has no effect on <option> element 2011-11-23 01:42:55 -0800 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) PC Linux RESOLVED FIXED P2 Normal --- 0 xqb748 webkit-unassigned cgarcia mrobinson oldest_to_newest 502767 0 xqb748 2011-11-15 02:58:28 -0800 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. 502773 1 115134 xqb748 2011-11-15 03:05:12 -0800 Created attachment 115134 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. 502963 2 mrobinson 2011-11-15 09:24:39 -0800 This looks fine to me, but I'd also like cgarcia to take a look at it. 503127 3 115134 xan.lopez 2011-11-15 12:07:31 -0800 Comment on attachment 115134 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? 503131 4 115134 xan.lopez 2011-11-15 12:08:44 -0800 Comment on attachment 115134 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! 503586 5 115319 xqb748 2011-11-15 21:56:59 -0800 Created attachment 115319 Updated patch Updated patch with review comments. 503666 6 115319 cgarcia 2011-11-16 00:16:29 -0800 Comment on attachment 115319 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()); 503707 7 xqb748 2011-11-16 01:38:25 -0800 (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. 503716 8 cgarcia 2011-11-16 01:53:00 -0800 (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. 503923 9 115319 mrobinson 2011-11-16 08:42:55 -0800 Comment on attachment 115319 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. 504054 10 115319 webkit.review.bot 2011-11-16 10:56:23 -0800 Comment on attachment 115319 Updated patch Clearing flags on attachment: 115319 Committed r100470: <http://trac.webkit.org/changeset/100470> 115134 2011-11-15 03:05:12 -0800 2011-11-15 21:56:59 -0800 Proposed Patch 72370.txt text/plain 2325 xqb748 SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwMDI1OSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDExLTExLTE1ICBBbnRhcnlh bWkgUGFuZGlhICA8YW50YXJ5YW1pLnBhbmRpYUBtb3Rvcm9sYS5jb20+CisKKyAgICAgICAgW0d0 a10gZGlzcGxheTpub25lIGhhcyBubyBlZmZlY3Qgb24gPG9wdGlvbj4gZWxlbWVudC4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTcyMzcwCisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiAo T09QUyEpCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ndGsvR3RrUG9wdXBNZW51LmNwcDoKKyAgICAg ICAgKFdlYkNvcmU6Okd0a1BvcHVwTWVudTo6YXBwZW5kSXRlbSk6CisgICAgICAgICogcGxhdGZv cm0vZ3RrL1BvcHVwTWVudUd0ay5jcHA6CisgICAgICAgIChXZWJDb3JlOjpQb3B1cE1lbnVHdGs6 OmNyZWF0ZUd0a0FjdGlvbkZvck1lbnVJdGVtKToKKwogMjAxMS0xMS0xNSAgQWxwaGEgTGFtICA8 aGNsYW1AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFtjaHJvbWl1bV0gc2Nyb2xsIGRlbHRhcyBh cmUgY2xlYXJlZCBkdXJpbmcgY29tbWl0IHRvIHRoZSBtYWluIHRocmVhZApJbmRleDogU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3RrL0d0a1BvcHVwTWVudS5jcHAKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL0d0a1BvcHVwTWVudS5jcHAJKHJldmlzaW9uIDEwMDI1 NSkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2d0ay9HdGtQb3B1cE1lbnUuY3BwCSh3b3Jr aW5nIGNvcHkpCkBAIC02Nyw3ICs2Nyw5IEBAIHZvaWQgR3RrUG9wdXBNZW51OjphcHBlbmRJdGVt KEd0a0FjdGlvbioKICAgICBndGtfd2lkZ2V0X3NldF90b29sdGlwX3RleHQobWVudUl0ZW0sIGd0 a19hY3Rpb25fZ2V0X3Rvb2x0aXAoYWN0aW9uKSk7CiAgICAgZ19zaWduYWxfY29ubmVjdChtZW51 SXRlbSwgInNlbGVjdCIsIEdfQ0FMTEJBQ0soR3RrUG9wdXBNZW51OjpzZWxlY3RJdGVtQ2FsbGJh Y2spLCB0aGlzKTsKICAgICBndGtfbWVudV9zaGVsbF9hcHBlbmQoR1RLX01FTlVfU0hFTEwobV9w b3B1cC5nZXQoKSksIG1lbnVJdGVtKTsKLSAgICBndGtfd2lkZ2V0X3Nob3cobWVudUl0ZW0pOwor CisgICAgaWYgKGd0a19hY3Rpb25faXNfdmlzaWJsZShhY3Rpb24pKQorICAgICAgICBndGtfd2lk Z2V0X3Nob3cobWVudUl0ZW0pOwogfQogCiB2b2lkIEd0a1BvcHVwTWVudTo6cG9wVXAoY29uc3Qg SW50U2l6ZSYgbWVudVNpemUsIGNvbnN0IEludFBvaW50JiBtZW51UG9zaXRpb24sIGludCBpdGVt Q291bnQsIGludCBzZWxlY3RlZEl0ZW0sIGNvbnN0IEdka0V2ZW50KiBldmVudCkKSW5kZXg6IFNv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL2d0ay9Qb3B1cE1lbnVHdGsuY3BwCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2d0ay9Qb3B1cE1lbnVHdGsuY3BwCShyZXZpc2lvbiAx MDAyNTUpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ndGsvUG9wdXBNZW51R3RrLmNwcAko d29ya2luZyBjb3B5KQpAQCAtNTgsNiArNTgsNyBAQCBHdGtBY3Rpb24qIFBvcHVwTWVudUd0azo6 Y3JlYXRlR3RrQWN0aW9uCiAgICAgZ19vYmplY3Rfc2V0X2RhdGEoR19PQkpFQ1QoYWN0aW9uKSwg InBvcHVwLW1lbnUtYWN0aW9uLWluZGV4IiwgR0lOVF9UT19QT0lOVEVSKGl0ZW1JbmRleCkpOwog ICAgIGdfc2lnbmFsX2Nvbm5lY3QoYWN0aW9uLCAiYWN0aXZhdGUiLCBHX0NBTExCQUNLKG1lbnVJ dGVtQWN0aXZhdGVkKSwgdGhpcyk7CiAgICAgLy8gRklYTUU6IEFwcGx5IHRoZSBQb3B1cE1lbnVT dHlsZSBmcm9tIGNsaWVudCgpLT5pdGVtU3R5bGUoaSkKKyAgICBndGtfYWN0aW9uX3NldF92aXNp YmxlKGFjdGlvbiwgIWNsaWVudCgpLT5pdGVtU3R5bGUoaXRlbUluZGV4KS5pc0Rpc3BsYXlOb25l KCkpOwogICAgIGd0a19hY3Rpb25fc2V0X3NlbnNpdGl2ZShhY3Rpb24sIGNsaWVudCgpLT5pdGVt SXNFbmFibGVkKGl0ZW1JbmRleCkpOwogCiAgICAgcmV0dXJuIGFjdGlvbjsK 115319 2011-11-15 21:56:59 -0800 2011-11-16 10:56:23 -0800 Updated patch 72370-updated.txt text/plain 2345 xqb748 SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwMDQwNCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDExLTExLTE1ICBBbnRhcnlh bWkgUGFuZGlhICA8YW50YXJ5YW1pLnBhbmRpYUBtb3Rvcm9sYS5jb20+CisKKyAgICAgICAgW0d0 a10gZGlzcGxheTpub25lIGhhcyBubyBlZmZlY3Qgb24gPG9wdGlvbj4gZWxlbWVudC4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTcyMzcwCisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ndGsv R3RrUG9wdXBNZW51LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Okd0a1BvcHVwTWVudTo6YXBwZW5k SXRlbSk6CisgICAgICAgICogcGxhdGZvcm0vZ3RrL1BvcHVwTWVudUd0ay5jcHA6CisgICAgICAg IChXZWJDb3JlOjpQb3B1cE1lbnVHdGs6OmNyZWF0ZUd0a0FjdGlvbkZvck1lbnVJdGVtKToKKwog MjAxMS0xMS0xNSAgSmVmZiBUaW1hbnVzICA8dHdpekBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAg W2Nocm9taXVtXSBEdXJpbmcgdGVhciBkb3duLCBwcmV2ZW50IHRoZSBXZWJHTExheWVyQ2hyb21p dW0gaW5zdGFuY2UgZnJvbSBhdHRlbXB0aW5nIHRvIHN0b3AgYSB0aW1lciBmb3IgYSBOVUxMIGNv bnRleHQuCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ndGsvR3RrUG9wdXBNZW51LmNw cAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ndGsvR3RrUG9wdXBNZW51 LmNwcAkocmV2aXNpb24gMTAwMjU1KQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL0d0 a1BvcHVwTWVudS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTY3LDcgKzY3LDkgQEAgdm9pZCBHdGtQ b3B1cE1lbnU6OmFwcGVuZEl0ZW0oR3RrQWN0aW9uKgogICAgIGd0a193aWRnZXRfc2V0X3Rvb2x0 aXBfdGV4dChtZW51SXRlbSwgZ3RrX2FjdGlvbl9nZXRfdG9vbHRpcChhY3Rpb24pKTsKICAgICBn X3NpZ25hbF9jb25uZWN0KG1lbnVJdGVtLCAic2VsZWN0IiwgR19DQUxMQkFDSyhHdGtQb3B1cE1l bnU6OnNlbGVjdEl0ZW1DYWxsYmFjayksIHRoaXMpOwogICAgIGd0a19tZW51X3NoZWxsX2FwcGVu ZChHVEtfTUVOVV9TSEVMTChtX3BvcHVwLmdldCgpKSwgbWVudUl0ZW0pOwotICAgIGd0a193aWRn ZXRfc2hvdyhtZW51SXRlbSk7CisKKyAgICBpZiAoZ3RrX2FjdGlvbl9pc192aXNpYmxlKGFjdGlv bikpCisgICAgICAgIGd0a193aWRnZXRfc2hvdyhtZW51SXRlbSk7CiB9CiAKIHZvaWQgR3RrUG9w dXBNZW51Ojpwb3BVcChjb25zdCBJbnRTaXplJiBtZW51U2l6ZSwgY29uc3QgSW50UG9pbnQmIG1l bnVQb3NpdGlvbiwgaW50IGl0ZW1Db3VudCwgaW50IHNlbGVjdGVkSXRlbSwgY29uc3QgR2RrRXZl bnQqIGV2ZW50KQpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL1BvcHVwTWVudUd0 ay5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL1BvcHVwTWVu dUd0ay5jcHAJKHJldmlzaW9uIDEwMDI1NSkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2d0 ay9Qb3B1cE1lbnVHdGsuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC01OCw2ICs1OCw3IEBAIEd0a0Fj dGlvbiogUG9wdXBNZW51R3RrOjpjcmVhdGVHdGtBY3Rpb24KICAgICBnX29iamVjdF9zZXRfZGF0 YShHX09CSkVDVChhY3Rpb24pLCAicG9wdXAtbWVudS1hY3Rpb24taW5kZXgiLCBHSU5UX1RPX1BP SU5URVIoaXRlbUluZGV4KSk7CiAgICAgZ19zaWduYWxfY29ubmVjdChhY3Rpb24sICJhY3RpdmF0 ZSIsIEdfQ0FMTEJBQ0sobWVudUl0ZW1BY3RpdmF0ZWQpLCB0aGlzKTsKICAgICAvLyBGSVhNRTog QXBwbHkgdGhlIFBvcHVwTWVudVN0eWxlIGZyb20gY2xpZW50KCktPml0ZW1TdHlsZShpKQorICAg IGd0a19hY3Rpb25fc2V0X3Zpc2libGUoYWN0aW9uLCAhY2xpZW50KCktPml0ZW1TdHlsZShpdGVt SW5kZXgpLmlzRGlzcGxheU5vbmUoKSk7CiAgICAgZ3RrX2FjdGlvbl9zZXRfc2Vuc2l0aXZlKGFj dGlvbiwgY2xpZW50KCktPml0ZW1Jc0VuYWJsZWQoaXRlbUluZGV4KSk7CiAKICAgICByZXR1cm4g YWN0aW9uOwo=