Bug 117998 – need mechanism to detect (partially) hidden blocked plugins

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Gordon Sheridan]

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.

[WebKit Commit Bot]

Comment on attachment 205411 [details]
Patch to provide indication of (partially) hidden blocked plugin replacement elements

Rejecting attachment 205411 [details] from review queue.

[e-mail] 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.

[WebKit Commit Bot]

Comment on attachment 205411 [details]
Patch to provide indication of (partially) hidden blocked plugin replacement elements

Rejecting attachment 205411 [details] from commit-queue.

[e-mail] 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.

[Simon Fraser (smfr)]

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.

[Gordon Sheridan]

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.

[Dean Jackson]

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.

[Dean Jackson]

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".

[Gordon Sheridan]

I'll add the check for accumulated opacity, and try checking for 'pointer-events: none' as well.

[Simon Fraser (smfr)]

> > 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.

[Gordon Sheridan]

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.

[Dean Jackson]

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.

[WebKit Commit Bot]

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

[Gordon Sheridan]

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.

[Gordon Sheridan]

Created attachment 206030 [details]
Patch to provide indication of (partially) hidden blocked plugin replacement elements #4 (content type=patch)

Mark check box "patch".

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.