Summary: | Plug-ins display despite having display: none | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||||||||||
Component: | Plug-ins | Assignee: | 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
mitz
2007-08-26 07:27:56 PDT
Created attachment 16124 [details]
Test case
*** Bug 19646 has been marked as a duplicate of this bug. *** *** Bug 10234 has been marked as a duplicate of this bug. *** The duplicates have many additional test cases. 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. I think that Plug-ins is a more suitable component for this - not that it matters much. 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. 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. 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. Smaller/simpler testcase: <html><body><object style='display: none'></object>This should be at the TOP of the page</body></html> Created attachment 24909 [details] Patch to fix bug 15081. This patch should fix the display:none style issue for html object tag. 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 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 on attachment 24909 [details] Patch to fix bug 15081. r- for now since a new patch is coming. Created attachment 24944 [details] New patch for bug 15081 Changes made following comments by Maciej Stachowiak and George Staikos. 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 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. *** Bug 23019 has been marked as a duplicate of this bug. *** Created attachment 27959 [details]
Patch, testcases, changelog
This patch fixes display:none for applet, embed and object.
Comment on attachment 27959 [details]
Patch, testcases, changelog
r=me
Bug 45049 plans to undo this. |