Bug 15081

Summary: Plug-ins display despite having display: none
Product: WebKit Reporter: mitz
Component: Plug-insAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Major CC: andersca, ap, lyon.chen, mihnea, mike.capp, shadow2531, simon.fraser, staikos, tqsub, weihong.zeng
Priority: P2 Keywords: HasReduction, InRadar
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Test case
none
reduction: object with specific size to not display
none
Patch to fix bug 15081.
mjs: review-
New patch for bug 15081
mrowe: review-
Patch, testcases, changelog andersca: review+

Description mitz 2007-08-26 07:27:56 PDT
WebKit displays <object> and <embed> plug-ins even if their display is set to "none" (either initially or dynamically). Firefox and Opera do not.
Comment 1 mitz 2007-08-26 07:28:40 PDT
Created attachment 16124 [details]
Test case
Comment 2 mitz 2007-08-26 07:29:33 PDT
See also bug 14339.
Comment 3 Mark Rowe (bdash) 2007-08-26 23:29:04 PDT
<rdar://problem/5439208>
Comment 4 Alexey Proskuryakov 2008-06-18 22:34:20 PDT
*** Bug 19646 has been marked as a duplicate of this bug. ***
Comment 5 Alexey Proskuryakov 2008-06-18 22:34:23 PDT
*** Bug 10234 has been marked as a duplicate of this bug. ***
Comment 6 Alexey Proskuryakov 2008-06-18 22:35:32 PDT
The duplicates have many additional test cases.
Comment 7 Thom 2008-06-27 12:01:03 PDT
Shouldn't the "Component" be changed from Plug-ins to CSS instead since the plug-ins are working but the CSS style isn't working?

Definitely has reductions, so how about adding the HasReduction keyword?

This bug also appears with OS as Mac OS X 10.5, and in Versions 525.x (Safari 3.1) and the nightly build. Have also known this bug to be around since well before that, near the beginning of Safari.
Comment 8 Alexey Proskuryakov 2008-06-28 01:39:55 PDT
I think that Plug-ins is a more suitable component for this - not that it matters much.
Comment 9 Thom 2008-07-01 17:16:02 PDT
Created attachment 22036 [details]
reduction: object with specific size to not display

This test case is similar to one of the test cases in duplicate bug 19646 but reduced more. That one had the style declared inline as well as in a style tag. This reduction does not, so the style is declared only once.
Comment 10 Thom 2008-07-01 17:21:12 PDT
Kind of curious. When I right-click and inspect the element on the test page, it shows in the computed styles that the display property for the object tag has been successfully set to "none". This is also true in the other tests for when the style isn't inline. It would seem the display style is being accepted (not ignored entirely) for the object tag, but for some reason not acted on in a way that affects the display style of the content of the object tag.
Comment 11 Mike Capp 2008-09-08 10:26:54 PDT
This bug also manifests on Windows (testing with 2003 Server), both in Safari 3.1.1 and Chrome 0.2.149.27.

Raised http://code.google.com/p/chromium/issues/detail?id=1415 - I'll link it to here.
Comment 12 George Staikos 2008-11-04 08:26:50 PST
Smaller/simpler testcase:

<html><body><object style='display: none'></object>This should be at  
the TOP of the page</body></html>
Comment 13 Lyon Chen 2008-11-05 07:49:49 PST
Created attachment 24909 [details]
Patch to fix bug 15081.

This patch should fix the display:none style issue for html object tag.
Comment 14 Maciej Stachowiak 2008-11-05 08:11:46 PST
Comment on attachment 24909 [details]
Patch to fix bug 15081.

Issues with this patch:

1) There shouldn't be a test case in a comment in the code.
2) There *should* be a test case (and expected result) added to LayoutTests.
3) The path contains tabs - it should not. We use all spaces for formatting.
4) I think it would be better style to call HTMLPluginElement::rendererIsNeeded(style) at the end instead of checking style->display() directly. If this was done, then the function could be refactored to do the special case frame check first (if not using fallback or an image) and then call the base class, which would be more elegant.

However, the change looks substantively correct to me, given mitz's test case.

Please fix at least 1-3, 4 is optional.
Comment 15 Maciej Stachowiak 2008-11-05 08:13:15 PST
Comment on attachment 24909 [details]
Patch to fix bug 15081.

r=me assuming at least items 1-3 from my list are fixed before committing, and a ChangeLog is added.
Comment 16 Maciej Stachowiak 2008-11-05 08:13:56 PST
Comment on attachment 24909 [details]
Patch to fix bug 15081.

r- for now since a new patch is coming.
Comment 17 Lyon Chen 2008-11-06 09:27:40 PST
Created attachment 24944 [details]
New patch for bug 15081

Changes made following comments by Maciej Stachowiak and George Staikos.
Comment 18 Geoffrey Garen 2008-11-24 22:25:44 PST
Looks good, but:
1. Please "run-webkit-tests css1/classification/display_object.html" in order to generate expected results for your new test, and include the results in your patch. Otherwise, if somebody lands your patch, it will turn the buildbot red.
2. Please add a newline to the end of LayoutTests/css1/classification/display_object.html, to make our diffing tools happy.
Comment 19 Mark Rowe (bdash) 2009-01-30 05:16:08 PST
Comment on attachment 24944 [details]
New patch for bug 15081

Marking as r-.  A revised patch that addresses Geoff's two points in comment #18 will surely get r+d.
Comment 20 Simon Fraser (smfr) 2009-02-24 22:08:56 PST
*** Bug 23019 has been marked as a duplicate of this bug. ***
Comment 21 Simon Fraser (smfr) 2009-02-24 22:10:50 PST
Created attachment 27959 [details]
Patch, testcases, changelog

This patch fixes display:none for applet, embed and object.
Comment 22 Anders Carlsson 2009-02-24 22:15:43 PST
Comment on attachment 27959 [details]
Patch, testcases, changelog

r=me
Comment 23 Simon Fraser (smfr) 2009-02-24 22:18:34 PST
http://trac.webkit.org/changeset/41209
Comment 24 Simon Fraser (smfr) 2011-11-02 16:39:39 PDT
Bug 45049 plans to undo this.