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

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: suraj_patel_95, Assigned: tallOwen)

Tracking

Trunk
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 677635 [details]
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.
(Reporter)

Updated

6 years ago
Component: Untriaged → General
OS: Windows 7 → All
Product: Firefox → Toolkit
Hardware: x86_64 → All
See Also: → bug 803015
Version: 18 Branch → Trunk
(Reporter)

Comment 1

6 years ago
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
(Reporter)

Updated

6 years ago
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]

Comment 4

6 years ago
A fix in the style system might be better than trying to reset all 100+ style properties in pluginProblemContent.css.
(Assignee)

Updated

6 years ago
Assignee: nobody → owen
(Reporter)

Comment 5

6 years ago
(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. :(
(Assignee)

Comment 7

6 years ago
Created attachment 701912 [details] [diff] [review]
Patch v0.1
Attachment #701912 - Flags: review?(jaws)
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+
(Assignee)

Comment 9

6 years ago
Created attachment 702663 [details] [diff] [review]
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.
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?
(Reporter)

Comment 12

6 years ago
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

Comment 15

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Comment 17

6 years ago
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.