807880 - [click to play] plugin placeholder can be styled by inherited CSS

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Suraj Patel]

Attachment: Testcase of inherited CSS

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0
Build ID: 20121101042011

Steps to reproduce:

Turned on the plugin.click_to_play about:config preference. Applied (inheritable) CSS to a HTML element that contains a plugin object whilst the plugin had not been activated via a click.


Actual results:

The CSS preferences were inherited by the plugin placeholder.


Expected results:

The CSS should not have been inherited by the plugin placeholder. This can be achieved by merely applying the default CSS values to the plugin placeholder itself for all the inheritable CSS properties.

Comment 1

[Suraj Patel]

Requires a very similar patch to the one here: https://bugzilla.mozilla.org/show_bug.cgi?id=803015

Just with these CSS properties too:

border-radius
white-space
text-indent
word-spacing
letter-spacing
cursor
line-height

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Suraj, would you like to put together a patch to fix this? If you join #introduction on Mozilla's IRC server (irc.mozilla.org) and send a message to me at "jaws" I can walk you through getting a build environment set up.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

I talked with Suraj and he would like to open the bug up to other people to work on.

Comment 4

[Jesse Ruderman]

A fix in the style system might be better than trying to reset all 100+ style properties in pluginProblemContent.css.

Comment 5

[Suraj Patel]

(In reply to Jesse Ruderman from comment #4)
> A fix in the style system might be better than trying to reset all 100+
> style properties in pluginProblemContent.css.

As far as I know, only the CSS properties displayed above currently affect the plugin container's CSS, because they are inheritable. Of course, a more permanent solution would be better to make it more future-proof.

Comment 6

[Justin Dolske [:Dolske]]

(In reply to Jesse Ruderman from comment #4)
> A fix in the style system might be better than trying to reset all 100+
> style properties in pluginProblemContent.css.

This has bitten videocontrols on a number of occasions as well. :(

Comment 7

[Owen Coutts [:tallOwen]]

Attachment: Patch v0.1

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 701912 [details] [diff] [review]
Patch v0.1

Review of attachment 701912 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, just change these and the one above them to 'initial' and resubmit the patch for review.

::: toolkit/mozapps/plugins/content/pluginProblemContent.css
@@ +51,5 @@
> +  cursor: auto;
> +  white-space: normal;
> +  word-spacing: normal;
> +  letter-spacing: normal;
> +  line-height: normal;

This is fine, but I think using the CSS value of 'initial' is probably better. It is easier to see what is intended that way and there should be no second-guessing about if the correct value was used to emulate the initial value.

Comment 9

[Owen Coutts [:tallOwen]]

Attachment: Patch v0.2

Updated to use `initial` value. This doesn't cover the border radius case which I think would have to be done in a more complicated manner since a I think a border radius change should still effect the embed elements size/shape.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 702663 [details] [diff] [review]
Patch v0.2

Review of attachment 702663 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Do you need me to land this for you?

Comment 11

[John Schoenick [:johns]]

Should this cover opacity as well?

Comment 12

[Suraj Patel]

I would personally say yes, since the opacity could be used maliciously to mislead users into unintentionally/accidentally activating the plugin.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Why don't we look at what should be inherited, and block everything else? My vote would be for just width/height.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

I don't think this should cover opacity. There are useful times where a plugin should be hidden until the page is ready to show it. Click-to-play is great in that it severely limits drive-by attacks, but we also don't want to break users experiences on websites or make the feature too annoying to use.

I pushed the attached patch to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e00fb1ffd19

Comment 15

[Jesse Ruderman]

Opacity is multiplied, not inherited, so this approach won't let you clear opacity anyway.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/5e00fb1ffd19

Comment 17

[Jesse Ruderman]

I filed bug 842329 for a more general fix. I wasn't sure whether to put it in Style System or XBL.