| Summary: | need mechanism to detect (partially) hidden blocked plugins | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gordon Sheridan <gordon_sheridan> | ||||||||||||
| Component: | Plug-ins | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | bdakin, commit-queue, dino, esprehn+autocc, glenn, simon.fraser | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | OS X 10.8 | ||||||||||||||
| Attachments: |
|
||||||||||||||
Comment on attachment 205411 [details] Patch to provide indication of (partially) hidden blocked plugin replacement elements Rejecting attachment 205411 [details] from review queue. gordon_sheridan@apple.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights. Comment on attachment 205411 [details] Patch to provide indication of (partially) hidden blocked plugin replacement elements Rejecting attachment 205411 [details] from commit-queue. gordon_sheridan@apple.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. Comment on attachment 205411 [details] Patch to provide indication of (partially) hidden blocked plugin replacement elements View in context: https://bugs.webkit.org/attachment.cgi?id=205411&action=review > Source/WebCore/rendering/RenderEmbeddedObject.cpp:286 > + TextRun run(""); Does this have to be initialized with an empty string? > Source/WebCore/rendering/RenderEmbeddedObject.h:54 > + bool replacementFitsInContentBoxRect() const; It's not clear from the interface what "replacement" means. Is there a better term to use? > Source/WebKit2/Shared/Plugins/Netscape/PluginInformation.cpp:123 > +PassRefPtr<ImmutableDictionary> createPluginInformationDictionary(const PluginModuleInfo& plugin, const String& frameURLString, const String& mimeType, const String& pageURLString, const String& pluginspageAttributeURLString, const String& pluginURLString, bool replacementMayBePartiallyHidden) You could provide a default value of false for replacementMayBePartiallyHidden to avoid mysterious 'false' at call sites. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:556 > + replacementMayBePartiallyHidden = renderObject->style()->visibility() != VISIBLE || !renderObject->replacementFitsInContentBoxRect(); It's a bit odd to have this visibility check here. It's one of many reasons why the plug-in may not be visible, and, if we were to make this check more complex, putting more code here would not make sense. We could add a new function to RenderEmbeddedObject, or even something more general on, say, RenderBox. Created attachment 205926 [details]
Patch to provide indication of (partially) hidden blocked plugin replacement elements
I’ve tested the following scenarios:
normal size/placement/visibility blocked plugin
off the right side of the screen
off the bottom of the screen
off the top of the screen
off the left of the screen
covered by another element
too small to display the replacement text
low opacity (< 0.1)
visibility: hidden
I set a conditional breakpoint that correctly triggers in all but the first three scenarios. In scenarios #2 and #3, I’m able to scroll the element on screen where it can then be clicked.
Comment on attachment 205926 [details] Patch to provide indication of (partially) hidden blocked plugin replacement elements View in context: https://bugs.webkit.org/attachment.cgi?id=205926&action=review In general I think this is ok - I can't really think of many better ways to do it. I worry about a lot of edge cases though: like an element animating over the plugin, pointer-events, an overlay that completely obscures the element but itself is invisible to pointer events (and thus will not hit test in your algorithm), etc. > Source/WebCore/rendering/RenderEmbeddedObject.cpp:304 > + if (style->opacity() < 0.1) > + return true; I'm confused by this: if any of the ancestors have less than 10% opacity, then this element is obscured? I assume you're trying to find if something up the parent chain is making this object mostly transparent. Maybe you should accumulate (multiply) the opacity as you go? 0.5 * 0.5 * 0.5 * 0.5 * 0.5 is getting pretty hard to see. > Source/WebCore/rendering/RenderEmbeddedObject.cpp:332 > + hit = docRenderer->hitTest(request, location, result); I think this will fail if something up the chain has set pointer-events to none. Comment on attachment 205926 [details] Patch to provide indication of (partially) hidden blocked plugin replacement elements View in context: https://bugs.webkit.org/attachment.cgi?id=205926&action=review > Source/WebCore/rendering/RenderEmbeddedObject.cpp:295 > +bool RenderEmbeddedObject::isReplacementObscured() const I think WebKit uses the term "replaced content" or "fallback content". I'll add the check for accumulated opacity, and try checking for 'pointer-events: none' as well. > > Source/WebCore/rendering/RenderEmbeddedObject.cpp:332
> > + hit = docRenderer->hitTest(request, location, result);
>
> I think this will fail if something up the chain has set pointer-events to none.
But that's the behavior you want.
Created attachment 205972 [details]
Patch to provide indication of (partially) hidden blocked plugin replacement elements #3
The check for 'pointer-events: none' turned out to be redundant with hitTest(), so I didn't incorporate it. I also tested that specific scenario to be sure.
Comment on attachment 205972 [details] Patch to provide indication of (partially) hidden blocked plugin replacement elements #3 View in context: https://bugs.webkit.org/attachment.cgi?id=205972&action=review Sorry for the red herring on pointer events. Although it still could be the case that something obscures the plugin visually that has pointer-events: none. That should be rare though. > Source/WebCore/ChangeLog:15 > + Added public method to determine if the EMBED element used for a blocked plugin is inaccessible to the user. Technically it could be embed or object, but not worth revving for that change. Comment on attachment 205972 [details] Patch to provide indication of (partially) hidden blocked plugin replacement elements #3 Rejecting attachment 205972 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 205972, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: out of 1 hunk FAILED -- saving rejects to file Source/WebKit2/Shared/Plugins/Netscape/PluginInformation.h.rej patching file Source/WebKit2/UIProcess/WebPageProxy.cpp patching file Source/WebKit2/UIProcess/WebPageProxy.h patching file Source/WebKit2/UIProcess/WebPageProxy.messages.in patching file Source/WebKit2/WebProcess/WebPage/WebPage.cpp Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Dean Jackson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/1016568 Created attachment 206029 [details]
Patch to provide indication of (partially) hidden blocked plugin replacement elements #4 (resolving conflicts)
This patch resolves the ChangeLog conflicts, and reverts to Jessie's original spelling of PlugIn for the the key and wrapper functions.
Created attachment 206030 [details]
Patch to provide indication of (partially) hidden blocked plugin replacement elements #4 (content type=patch)
Mark check box "patch".
Comment on attachment 206030 [details] Patch to provide indication of (partially) hidden blocked plugin replacement elements #4 (content type=patch) Clearing flags on attachment: 206030 Committed r152382: <http://trac.webkit.org/changeset/152382> All reviewed patches have been landed. Closing bug. |
Created attachment 205411 [details] Patch to provide indication of (partially) hidden blocked plugin replacement elements We'd like to be able to detect when the replacement elements for a blocked plugin are hidden or partially obscured, making it difficult for the user to identify why the page might not be working as expected.