Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5275007:
Add the ability to remove blocked plugin containers from the page. For now,... (Closed)
|
Created:
10 years ago by Chris Evans Modified:
9 years, 6 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, brettw-cc_chromium.org, darin (slow to review) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd an "x" to remove the blocked plug-in placeholder. The feature already
exists in the context menu, but now it works well enough to promote to the
main placeholder UI.
BUG=63695
TEST=see bug
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71528
Patch Set 1 #
Total comments: 3
Patch Set 2 : '' #Patch Set 3 : '' #
Messages
Total messages: 14 (0 generated)
Seems to make all those irritating sites usable in "block mode.
Could you add a screenshot? http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.cc File chrome/renderer/blocked_plugin.cc (right): http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.... chrome/renderer/blocked_plugin.cc:95: container->element().setAttribute("height", "0"); Is setting the element to 0x0 what we want to do? I think Darin mentioned keeping it the same size, but making it transparent and ignoring user input.
Yeah, how about using "visibility: hidden" instead so that the plugin still takes up space? Page layout may depend on the size of the plugin. -Darin On Mon, Nov 29, 2010 at 7:57 AM, <bauerb@chromium.org> wrote: > Could you add a screenshot? > > > > http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.cc > File chrome/renderer/blocked_plugin.cc (right): > > > http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.... > chrome/renderer/blocked_plugin.cc:95: > > container->element().setAttribute("height", "0"); > Is setting the element to 0x0 what we want to do? I think Darin > mentioned keeping it the same size, but making it transparent and > ignoring user input. > > > http://codereview.chromium.org/5275007/ >
On Mon, Nov 29, 2010 at 7:57 AM, <bauerb@chromium.org> wrote: > Could you add a screenshot? > Screenshots added. > > > http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.cc > File chrome/renderer/blocked_plugin.cc (right): > > > http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.... > chrome/renderer/blocked_plugin.cc:95: > > container->element().setAttribute("height", "0"); > Is setting the element to 0x0 what we want to do? I think Darin > mentioned keeping it the same size, but making it transparent and > ignoring user input. > > > http://codereview.chromium.org/5275007/ >
On Mon, Nov 29, 2010 at 10:29 AM, Chris Evans <cevans@google.com> wrote: > On Mon, Nov 29, 2010 at 7:57 AM, <bauerb@chromium.org> wrote: > >> Could you add a screenshot? >> > > Screenshots added. > (to the bug... d'oh!) > > >> >> >> http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.cc >> File chrome/renderer/blocked_plugin.cc (right): >> >> >> http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.... >> chrome/renderer/blocked_plugin.cc:95: >> >> container->element().setAttribute("height", "0"); >> Is setting the element to 0x0 what we want to do? I think Darin >> mentioned keeping it the same size, but making it transparent and >> ignoring user input. >> >> >> http://codereview.chromium.org/5275007/ >> > >
On Mon, Nov 29, 2010 at 9:05 AM, Darin Fisher <darin@chromium.org> wrote: > Yeah, how about using "visibility: hidden" instead so that the plugin still > takes up space? Page layout may depend on the size of the plugin. > visibility:hidden will still swallow events and therefore breaks the key case this fix is addressing: http://scary.beasts.org/misc/overlay.html I quite like width,height:0 (just changed to display:none for simplicity) for the following reasons: - Addresses the key intended use case properly (removing problematic overlays, fixing the underlying page). - In the non-overlay case, it's an overall nicer experience to "recover" the space that was occupied by the banner. Patch it in and give it a try :) I think we should try it this way on trunk / dev to see what the feedback is. Cheers Chris > -Darin > > > On Mon, Nov 29, 2010 at 7:57 AM, <bauerb@chromium.org> wrote: > >> Could you add a screenshot? >> >> >> >> http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.cc >> File chrome/renderer/blocked_plugin.cc (right): >> >> >> http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.... >> chrome/renderer/blocked_plugin.cc:95: >> >> container->element().setAttribute("height", "0"); >> Is setting the element to 0x0 what we want to do? I think Darin >> mentioned keeping it the same size, but making it transparent and >> ignoring user input. >> >> >> http://codereview.chromium.org/5275007/ >> > >
FWIW, I've been deliberately running across sites with Flash ads all morning, and I can't fault the layout behaviour. Feel free to pop by my desk for demo / tweaking :) On Mon, Nov 29, 2010 at 11:10 AM, Chris Evans <cevans@google.com> wrote: > On Mon, Nov 29, 2010 at 9:05 AM, Darin Fisher <darin@chromium.org> wrote: > >> Yeah, how about using "visibility: hidden" instead so that the plugin >> still >> takes up space? Page layout may depend on the size of the plugin. >> > > visibility:hidden will still swallow events and therefore breaks the key > case this fix is addressing: http://scary.beasts.org/misc/overlay.html > > I quite like width,height:0 (just changed to display:none for simplicity) > for the following reasons: > > - Addresses the key intended use case properly (removing problematic > overlays, fixing the underlying page). > - In the non-overlay case, it's an overall nicer experience to "recover" > the space that was occupied by the banner. Patch it in and give it a try :) > I think we should try it this way on trunk / dev to see what the feedback > is. > > > Cheers > Chris > > >> -Darin >> >> >> On Mon, Nov 29, 2010 at 7:57 AM, <bauerb@chromium.org> wrote: >> >>> Could you add a screenshot? >>> >>> >>> >>> http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.cc >>> File chrome/renderer/blocked_plugin.cc (right): >>> >>> >>> http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.... >>> chrome/renderer/blocked_plugin.cc:95: >>> >>> container->element().setAttribute("height", "0"); >>> Is setting the element to 0x0 what we want to do? I think Darin >>> mentioned keeping it the same size, but making it transparent and >>> ignoring user input. >>> >>> >>> http://codereview.chromium.org/5275007/ >>> >> >> >
http://codereview.chromium.org/5275007/diff/1/chrome/renderer/resources/outda... File chrome/renderer/resources/outdated_plugin.html (right): http://codereview.chromium.org/5275007/diff/1/chrome/renderer/resources/outda... chrome/renderer/resources/outdated_plugin.html:29: #outer:hover img { Nit: Could you join these two selectors?
On Mon, Nov 29, 2010 at 11:10 AM, Chris Evans <cevans@google.com> wrote: > On Mon, Nov 29, 2010 at 9:05 AM, Darin Fisher <darin@chromium.org> wrote: > >> Yeah, how about using "visibility: hidden" instead so that the plugin >> still >> takes up space? Page layout may depend on the size of the plugin. >> > > visibility:hidden will still swallow events and therefore breaks the key > case this fix is addressing: http://scary.beasts.org/misc/overlay.html > That's easy to fix by making the dummy plugin return false from its handleInputEvent method. > > I quite like width,height:0 (just changed to display:none for simplicity) > for the following reasons: > > - Addresses the key intended use case properly (removing problematic > overlays, fixing the underlying page). > - In the non-overlay case, it's an overall nicer experience to "recover" > the space that was occupied by the banner. Patch it in and give it a try :) > I think we should try it this way on trunk / dev to see what the feedback > is. > I see. I hadn't considered that side-effect. I guess since there is a close button that the user is clicking on, it makes sense to collapse the plugin when they choose to close it. If it messes up the layout on some pages, then the users will probably be OK with that. -Darin > > > Cheers > Chris > > >> -Darin >> >> >> On Mon, Nov 29, 2010 at 7:57 AM, <bauerb@chromium.org> wrote: >> >>> Could you add a screenshot? >>> >>> >>> >>> http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.cc >>> File chrome/renderer/blocked_plugin.cc (right): >>> >>> >>> http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.... >>> chrome/renderer/blocked_plugin.cc:95: >>> >>> container->element().setAttribute("height", "0"); >>> Is setting the element to 0x0 what we want to do? I think Darin >>> mentioned keeping it the same size, but making it transparent and >>> ignoring user input. >>> >>> >>> http://codereview.chromium.org/5275007/ >>> >> >> >
Selectors merged. http://codereview.chromium.org/5275007/diff/1/chrome/renderer/resources/outda... File chrome/renderer/resources/outdated_plugin.html (right): http://codereview.chromium.org/5275007/diff/1/chrome/renderer/resources/outda... chrome/renderer/resources/outdated_plugin.html:29: #outer:hover img { On 2010/11/29 21:02:49, Bernhard Bauer wrote: > Nit: Could you join these two selectors? Done.
On 2010/11/30 03:17:46, Chris Evans wrote: > Selectors merged. > > http://codereview.chromium.org/5275007/diff/1/chrome/renderer/resources/outda... > File chrome/renderer/resources/outdated_plugin.html (right): > > http://codereview.chromium.org/5275007/diff/1/chrome/renderer/resources/outda... > chrome/renderer/resources/outdated_plugin.html:29: #outer:hover img { > On 2010/11/29 21:02:49, Bernhard Bauer wrote: > > Nit: Could you join these two selectors? > > Done. Thanks. If we file a bug for a nicer icon (or leave http://crbug.com/63695 open), LGTM.
On Mon, Nov 29, 2010 at 3:43 PM, Darin Fisher <darin@chromium.org> wrote: > On Mon, Nov 29, 2010 at 11:10 AM, Chris Evans <cevans@google.com> wrote: > >> On Mon, Nov 29, 2010 at 9:05 AM, Darin Fisher <darin@chromium.org> wrote: >> >>> Yeah, how about using "visibility: hidden" instead so that the plugin >>> still >>> takes up space? Page layout may depend on the size of the plugin. >>> >> >> visibility:hidden will still swallow events and therefore breaks the key >> case this fix is addressing: http://scary.beasts.org/misc/overlay.html >> > > That's easy to fix by making the dummy plugin return false from its > handleInputEvent method. > Heya Darin, I wonder if you could help a little here? I'm not going to submit this change for now due to difficulties in event handling. Even when setting the <embed> size to width and height==0, it seems common for the <embed> to have a parent <div> with the same pixel size which then proceeds to swallow events. I updated my example at http://scary.beasts.org/misc/overlay.html to illustrate this. Even without this CL applied, you can use dev tools to manually edit the <embed> height to set it to 0. You'll see that the underlying page cannot be interacted with due to the <div> overlay. Any ideas regarding event propagation, before I apply more of a sledgehammer approach? :) Cheers Chris > > >> >> I quite like width,height:0 (just changed to display:none for simplicity) >> for the following reasons: >> >> - Addresses the key intended use case properly (removing problematic >> overlays, fixing the underlying page). >> - In the non-overlay case, it's an overall nicer experience to "recover" >> the space that was occupied by the banner. Patch it in and give it a try :) >> I think we should try it this way on trunk / dev to see what the feedback >> is. >> > > I see. I hadn't considered that side-effect. I guess since there is a > close button that the user is clicking on, it makes sense to collapse the > plugin when they choose to close it. If it messes up the layout on some > pages, then the users will probably be OK with that. > > -Darin > > > >> >> >> Cheers >> Chris >> >> >>> -Darin >>> >>> >>> On Mon, Nov 29, 2010 at 7:57 AM, <bauerb@chromium.org> wrote: >>> >>>> Could you add a screenshot? >>>> >>>> >>>> >>>> http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.cc >>>> File chrome/renderer/blocked_plugin.cc (right): >>>> >>>> >>>> http://codereview.chromium.org/5275007/diff/1/chrome/renderer/blocked_plugin.... >>>> chrome/renderer/blocked_plugin.cc:95: >>>> >>>> container->element().setAttribute("height", "0"); >>>> Is setting the element to 0x0 what we want to do? I think Darin >>>> mentioned keeping it the same size, but making it transparent and >>>> ignoring user input. >>>> >>>> >>>> http://codereview.chromium.org/5275007/ >>>> >>> >>> >> >
Ok so here's the final piece which adds the "x" to the UI. I've used a nicely-blended, not-in-your-face "x" image, based on previous discussions. It's a reuse of an existing one which is nice. I've uploaded a new screenshot to the bug.
On Fri, Jan 14, 2011 at 23:59, <cevans@chromium.org> wrote: > Ok so here's the final piece which adds the "x" to the UI. I've used a > nicely-blended, not-in-your-face "x" image, based on previous discussions. > It's > a reuse of an existing one which is nice. I've uploaded a new screenshot to > the > bug. LGTM. I think we could add it to the click-to-play UI as well (and get rid of the crosshair cursor while we're at it ;-) > > http://codereview.chromium.org/5275007/ > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
