Closed Bug 807880 Opened 9 years ago Closed 8 years ago

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

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: suraj_patel_95, Assigned: tallOwen)

References

Details

(Whiteboard: [good first bug][mentor=jaws][lang=css])

Attachments

(2 files, 1 obsolete file)

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.
Component: Untriaged → General
OS: Windows 7 → All
Product: Firefox → Toolkit
Hardware: x86_64 → All
See Also: → 803015
Version: 18 Branch → Trunk
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
Attachment #677635 - Attachment mime type: text/plain → text/html
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I talked with Suraj and he would like to open the bug up to other people to work on.
Whiteboard: [good first bug][mentor=jaws][lang=css]
A fix in the style system might be better than trying to reset all 100+ style properties in pluginProblemContent.css.
Assignee: nobody → owen
(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.
(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. :(
Attached patch Patch v0.1 (obsolete) — Splinter Review
Attachment #701912 - Flags: review?(jaws)
Attachment #701912 - Attachment is patch: true
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.
Attachment #701912 - Flags: review?(jaws) → feedback+
Attached patch Patch v0.2Splinter Review
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.
Attachment #701912 - Attachment is obsolete: true
Attachment #702663 - Flags: review?(jaws)
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?
Attachment #702663 - Flags: review?(jaws) → review+
Should this cover opacity as well?
I would personally say yes, since the opacity could be used maliciously to mislead users into unintentionally/accidentally activating the plugin.
Why don't we look at what should be inherited, and block everything else? My vote would be for just width/height.
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
Status: NEW → ASSIGNED
Opacity is multiplied, not inherited, so this approach won't let you clear opacity anyway.
https://hg.mozilla.org/mozilla-central/rev/5e00fb1ffd19
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
I filed bug 842329 for a more general fix. I wasn't sure whether to put it in Style System or XBL.
You need to log in before you can comment on or make changes to this bug.