1381851 - Stylo: Flash click to play doesn't display on video elements when Stylo is enabled

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Peter Dolanjski [:pdol]]

Attachment: Expected-UI.png

v56 Nightly

Steps to Reproduce:
1. Go to about:config
2. Change layout.css.servo.enabled to true
3. Visit http://www.ctvnews.ca/video?playlistId=1.3507592

Expected Results:
- Flash Click to Play UI displays within video frame on page

Actual Results:
- No Flash Click to Play UI displays within video frame on page
- You can still click in the grey box (or in the URL bar) to give permissions for Flash
- Setting layout.css.servo.enabled to false fixes the issue

Comment 1

[Peter Dolanjski [:pdol]]

Attachment: Actual-UI.png

Comment 2

[Benjamin Smedberg]

The code that powers this is here: https://dxr.mozilla.org/mozilla-central/rev/e0b0865639cebc1b5afa0268a4b073fcdde0e69c/browser/modules/PluginContent.jsm#307

The goal is to only show the in-content UI if that is a clickable element. We check the following points to ensure this:

    let points = [[left, top],
                   [left, bottom],
                   [right, top],
                   [right, bottom],
                   [centerX, centerY]];

And we use nsIDOMWindowUtils.elementFromPoint to determine whether that hits the plugin element or something covering it.

I don't know yet whether it's the .getBoundingClientRect call or the .elementFromPoint call which is returning different results.

This feature is covered by browser tests: browser_pluginnotification.js browser_CTP_zoom.js and browser_CTP_iframe.js at least. Jet, do you know if we're running all the browser tests with stylo enabled anywhere? And can you hook this bug up to the stylo tracking system appropriately?

Comment 3

[Jet Villegas (inactive)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #2)
> This feature is covered by browser tests: browser_pluginnotification.js
> browser_CTP_zoom.js and browser_CTP_iframe.js at least. Jet, do you know if
> we're running all the browser tests with stylo enabled anywhere? And can you
> hook this bug up to the stylo tracking system appropriately?

Yes, and browser_CTP_iframe.js is listed [1] as a known Stylo failure in bug 1320841.

[1] https://bug1320841.bmoattachments.org/attachment.cgi?id=8865289

Comment 4

[Bobby Holley (:bholley)]

IIRC the click-to-play stuff is done with an XBL binding, so it might be a good thing for TYLin to look at.

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

The click-to-play feature uses internal-only pseudo-classes like :-moz-handler-clicktoplay [1] in XBL style sheets [2], but we load all the chrome XBL style sheets under `SheetParsingMode::eAuthorSheetFeatures.`

We'll need to use `SheetParsingMode::eSafeAgentSheetFeatures` when loading chrome XBL sheets to allow servo to parse them under agent level.

[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/servo/components/style/gecko/non_ts_pseudo_class_list.rs#80
[2] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/pluginproblem/content/pluginProblemContent.css#24
[3] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/dom/xbl/nsXBLResourceLoader.cpp#143

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: style: Rename PSEUDO_CLASS_INTERNAL to PSEUDO_CLASS_ENABLED_IN_UA_SHEETS

The click-to-play feature uses internal only pseudo-classes like
:-moz-handler-clicktoplay in XBL style sheets, so we need to use
eSafeAgentSheetFeatures to allow servo to parse them under agent level.

Review commit: https://reviewboard.mozilla.org/r/160892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160892/

Comment 8

[Cameron McCormack (:heycam)]

Per discussion just now: Gecko currently distinguishes between features allowed in UA sheets and features allowed in chrome:// URL sheets, and I think it is the fact that these pseudo-classes like :-moz-handler-clicktoplay are allowed in chrome:// URLs that they work, and not because Gecko parses the sheet with UA features enabled.  Ideally, we wouldn't allow all chrome:// URL sheets loaded from XBL bindings to get access to UA sheet features.

So, if it's not too difficult, I think we should teach Servo how to enable/disable pseudo-classes based on whether we're parsing for a chrome:// URL sheet.  In ServoStyleSheet::ParseSheet, we have the aSheetURI argument we can look at (and call dom::IsChromeURI on, just like nsCSSParser does) and pass that through to Servo_StyleSheet_FromUTF8Bytes.  Eventually that flag should get passed down and stored on the SelectorParser struct (in selector_parser.rs), and in non_ts_pseudo_class_list.rs we can have a new flag like PSEUDO_CLASS_CHROME_SHEET on all the relevant pseudo-classes.

After that it might be worth filing a followup bug to do the same for properties that Gecko says are allowed in chrome:// URL sheets.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8889849 [details]
style: Rename PSEUDO_CLASS_INTERNAL to PSEUDO_CLASS_ENABLED_IN_UA_SHEETS

https://reviewboard.mozilla.org/r/160892/#review166652

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: style: Introduce Chrome UI privilege for parsers

The motivation is that Chrome XBL stylesheet can be parsed under author
level, but we allow some event-state pseudo classes like
:-moz-handled-clicktoplay to be used.

Also make the privilege of pseudo classes in non_ts_pseudo_class_list.rs and
nsCSSPseudoClassList.h in sync (except :fullscreen).

Review commit: https://reviewboard.mozilla.org/r/162476/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162476/

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: style: Update comments for push_applicable_declarations_as_xbl_only_stylist()

Review commit: https://reviewboard.mozilla.org/r/162478/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162478/

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1381851 - Add mIsChrome to URLExtraData for querying on servo side.

Review commit: https://reviewboard.mozilla.org/r/162480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162480/

Comment 13

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1381851 - Update comments for nsXBLPrototypeResources::ComputeServoStyleSet().

Review commit: https://reviewboard.mozilla.org/r/162482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162482/

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8889849 [details]
style: Rename PSEUDO_CLASS_INTERNAL to PSEUDO_CLASS_ENABLED_IN_UA_SHEETS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160892/diff/1-2/

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f43fc0b2f8b7c3bd65fd35b73d70bd3b43b571d

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8889849 [details]
style: Rename PSEUDO_CLASS_INTERNAL to PSEUDO_CLASS_ENABLED_IN_UA_SHEETS

https://reviewboard.mozilla.org/r/160892/#review168190

::: commit-message-dcfb5:3
(Diff revision 2)
> +Some of the pseudo classes in nsCSSPseudoClassList are enabled only in ua
> +sheets but not in chrome. Rename the "internal" flag to "enabled in ua
> +sheets" to make the semantics clearer.

Nit: "UA" rather than "ua", since it's an acronym.  (Well, an initialism if you're being pedantic.)

::: servo/components/style/gecko/selector_parser.rs:127
(Diff revision 2)
>  
>  
>  impl NonTSPseudoClass {
>      /// A pseudo-class is internal if it can only be used inside
>      /// user agent style sheets.
>      pub fn is_internal(&self) -> bool {

I think we should rename this function too.  (And then update the comment in the bitflags definition above.)

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8891415 [details]
style: Introduce Chrome UI privilege for parsers

https://reviewboard.mozilla.org/r/162476/#review168194

::: commit-message-a63f4:1
(Diff revision 1)
> +style: Introduce Chrome UI level for parsers

I think this comment doesn't accurately describe the patch (since it's not really a new cascade level you're introducing).  Maybe "mode"?

::: commit-message-a63f4:3
(Diff revision 1)
> +style: Introduce Chrome UI level for parsers
> +
> +The motivation is that Chrome XBL stylesheet can be parsed under author

"stylesheets"

::: commit-message-a63f4:7
(Diff revision 1)
> +Also make the privilege of pseudo classes in non_ts_pseudo_class_list.rs and
> +nsCSSPseudoClassList.h in sync (except :fullscreen).

"make the privilege of ... be in sync" or "synchronize the privilege of ...".

::: servo/components/layout_thread/lib.rs:1726
(Diff revision 1)
>              ServoUrl::parse(&format!("chrome://resources/{:?}", filename)).unwrap(),
>              None,
>              None,
>              Origin::UserAgent,
> +            false,

It's a bit funny that here Servo uses a chrome:// URL but doesn't enable the chrome sheet privileges.

Here is any idea:

What if we use the url_data argument to Stylesheet::from_bytes and {Stylesheet,StylesheetContents}::from_str to determine whether to enable chrome-privileged features?  For stylo, we could store a bool on the UrlExtraData object that C++ allocates, since we would need to check nsIURI from there.  For Servo, we could look directly at the ServoURL.

This way we could have similar behavior between Servo and Stylo, and it would also avoid the need to pass a boolean down some functions.  (Even though Servo doesn't have any pseudo-classes defined where they're enabled only when chrome privileges are enabled, it would be nice to minimize the differences between Servo and Stylo.)

For any struct where we do have to store the boolean (because we can't reach up to the Stylesheet object to check it), can we use an enum instead of a boolean, to make things a little clearer?  Maybe SheetPrivelege::Normal and SheetPrivelege::Chrome, or maybe some nicer name.

WDYT?

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8891416 [details]
style: Update comments for push_applicable_declarations_as_xbl_only_stylist()

https://reviewboard.mozilla.org/r/162478/#review168196

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8891418 [details]
Bug 1381851 - Update comments for nsXBLPrototypeResources::ComputeServoStyleSet().

https://reviewboard.mozilla.org/r/162482/#review168198

Comment 20

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8889849 [details]
style: Rename PSEUDO_CLASS_INTERNAL to PSEUDO_CLASS_ENABLED_IN_UA_SHEETS

https://reviewboard.mozilla.org/r/160892/#review168190

> I think we should rename this function too.  (And then update the comment in the bitflags definition above.)

I'll fix this in the next patch.

Comment 21

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8891415 [details]
style: Introduce Chrome UI privilege for parsers

https://reviewboard.mozilla.org/r/162476/#review168194

> It's a bit funny that here Servo uses a chrome:// URL but doesn't enable the chrome sheet privileges.
> 
> Here is any idea:
> 
> What if we use the url_data argument to Stylesheet::from_bytes and {Stylesheet,StylesheetContents}::from_str to determine whether to enable chrome-privileged features?  For stylo, we could store a bool on the UrlExtraData object that C++ allocates, since we would need to check nsIURI from there.  For Servo, we could look directly at the ServoURL.
> 
> This way we could have similar behavior between Servo and Stylo, and it would also avoid the need to pass a boolean down some functions.  (Even though Servo doesn't have any pseudo-classes defined where they're enabled only when chrome privileges are enabled, it would be nice to minimize the differences between Servo and Stylo.)
> 
> For any struct where we do have to store the boolean (because we can't reach up to the Stylesheet object to check it), can we use an enum instead of a boolean, to make things a little clearer?  Maybe SheetPrivelege::Normal and SheetPrivelege::Chrome, or maybe some nicer name.
> 
> WDYT?

Your idea is better! I'll write a new patch based on this.

Comment 22

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8889849 [details]
style: Rename PSEUDO_CLASS_INTERNAL to PSEUDO_CLASS_ENABLED_IN_UA_SHEETS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160892/diff/2-3/

Comment 23

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8891415 [details]
style: Introduce Chrome UI privilege for parsers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162476/diff/1-2/

Comment 24

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8891416 [details]
style: Update comments for push_applicable_declarations_as_xbl_only_stylist()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162478/diff/1-2/

Comment 25

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8891417 [details]
Bug 1381851 - Add mIsChrome to URLExtraData for querying on servo side.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162480/diff/1-2/

Comment 26

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8891418 [details]
Bug 1381851 - Update comments for nsXBLPrototypeResources::ComputeServoStyleSet().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162482/diff/1-2/

Comment 27

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c40ed02b96bba9cac445a13f3d3db27f70c95339

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8891415 [details]
style: Introduce Chrome UI privilege for parsers

https://reviewboard.mozilla.org/r/162476/#review169612

::: servo/components/style/selector_parser.rs:55
(Diff revision 2)
>  pub struct SelectorParser<'a> {
>      /// The origin of the stylesheet we're parsing.
>      pub stylesheet_origin: Origin,
>      /// The namespace set of the stylesheet.
>      pub namespaces: &'a Namespaces,
> +    /// The extra URL data of the stylesheet.

Maybe comment here that we use this to look up whether we are parsing a chrome:// URL style sheet.

::: servo/components/style/selector_parser.rs:84
(Diff revision 2)
> +        if let Some(url_data) = self.url_data {
> +            url_data.is_chrome()
> +        } else {
> +            false
> +        }

A bit shorter (and maybe more Rust-y):

  self.url_data.map_or(false, |d| d.is_chrome())

Comment 29

[Cameron McCormack (:heycam)]

Comment on attachment 8891417 [details]
Bug 1381851 - Add mIsChrome to URLExtraData for querying on servo side.

https://reviewboard.mozilla.org/r/162480/#review169614

::: layout/style/URLExtraData.h:30
(Diff revision 2)
>                 already_AddRefed<nsIURI> aReferrer,
>                 already_AddRefed<nsIPrincipal> aPrincipal)
>      : mBaseURI(Move(aBaseURI))
>      , mReferrer(Move(aReferrer))
>      , mPrincipal(Move(aPrincipal))
> +    , mIsChrome(mReferrer ? dom::IsChromeURI(mReferrer) : false)

Since it's a bit non-obvious, can you add a comment here pointing out that the referrer URI is always the same as the sheet URI (which is actually what we want to check for being chrome://)?

::: layout/style/URLExtraData.h:59
(Diff revision 2)
>    ~URLExtraData();
>  
>    nsCOMPtr<nsIURI> mBaseURI;
>    nsCOMPtr<nsIURI> mReferrer;
>    nsCOMPtr<nsIPrincipal> mPrincipal;
> +  const bool mIsChrome;

And comment here that mIsChrome is talking about whether the URI of the style sheet that this UrlExtraData is for is chrome://.

Comment 30

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8891417 [details]
Bug 1381851 - Add mIsChrome to URLExtraData for querying on servo side.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162480/diff/2-3/

Comment 31

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8891418 [details]
Bug 1381851 - Update comments for nsXBLPrototypeResources::ComputeServoStyleSet().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162482/diff/2-3/

Comment 32

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Servo PR #17958

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/323e0b914fd6
Add mIsChrome to URLExtraData for querying on servo side. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e1c23455946b
Update comments for nsXBLPrototypeResources::ComputeServoStyleSet(). r=heycam

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/323e0b914fd6
https://hg.mozilla.org/mozilla-central/rev/e1c23455946b

Comment 36

[Chris Peterson [:cpeterson]]

Ting-Yu, should we uplift this Flash fix to Beta 56? Some websites could be broken if Flash click-to-play doesn't work.

Comment 37

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: servo: Merge #17958 - Pseudo classes chrome privilege (bug 1381851) (from aethanyc:pseudo-classes-chrome-privilege-bug1381851); r=heycam

This is reviewed in https://bugzilla.mozilla.org/show_bug.cgi?id=1381851

Source-Repo: https://github.com/servo/servo
Source-Revision: c18cac1f342ecf7f5986ea3fec857fdaca649d9f

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : d6120073c92edf3ae9827dc6d1699b25854fa50f

MozReview-Commit-ID: 323FeB6en7S

Comment 38

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Chris Peterson [:cpeterson] from comment #36)
> Ting-Yu, should we uplift this Flash fix to Beta 56? Some websites could be
> broken if Flash click-to-play doesn't work.

Yes, let's uplift this to fix the UI artifact of the click-to-play feature.

Comment 39

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8891417 [details]
Bug 1381851 - Add mIsChrome to URLExtraData for querying on servo side.

Approval Request Comment
[Feature/Bug causing the regression]: Unimplement chrome stylesheet privilege for stylo.
[User impact if declined]: No visual Flash click-to-play feature is broken. See comment 0.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes, with "layout.css.servo.enabled" pref is "true"
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0.
[List of other uplifts needed for the feature/fix]: The patch "servo: Merge #17958 - Pseudo classes chrome privilege (bug 1381851)" attached in omment 37
[Is the change risky?]: Low. 
[Why is the change risky/not risky?]: It has effect only when  "layout.css.servo.enabled" pref is true
[String changes made/needed]: None

Comment 40

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8891417 [details]
Bug 1381851 - Add mIsChrome to URLExtraData for querying on servo side.

Approval Request Comment
[Feature/Bug causing the regression]: stylo doesn't implement chrome stylesheet privilege.
[User impact if declined]: Flash click-to-play feature is broken. See comment 0.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes, when "layout.css.servo.enabled" pref is "true".
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0.
[List of other uplifts needed for the feature/fix]: The patch "servo: Merge #17958 - Pseudo classes chrome privilege (bug 1381851)" attached in comment 37.
[Is the change risky?]: Low. 
[Why is the change risky/not risky?]: It only has effect when "layout.css.servo.enabled" pref is true. It should not affect gecko.
[String changes made/needed]: None

Comment 41

[Chris Peterson [:cpeterson]]

Blocking bug 1381147 because we want to fix this crash before starting the Stylo experiment on Beta 56.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8891417 [details]
Bug 1381851 - Add mIsChrome to URLExtraData for querying on servo side.

Crash fix, we need it for stylo testing, let's uplift for beta 2.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b5abd699f37b
https://hg.mozilla.org/releases/mozilla-beta/rev/4f459069cd2c

Comment 44

[Bogdan Maris, Desktop Test Engineering]

I managed to reproduce this issue using Mac OS X 10.11 on Nightly from 2017-07-18.

I retested everything using the same platform on beta 56.0b9. The issues is not reproducing anymore, but I have to mention that the video starts to play automatically without asking for permission. I even checked in about:addons - Plugins and the Flash is set to 'Ask to Activate'. Is that expected behaviour?

Comment 45

[Bobby Holley (:bholley)]

We shouldn't be testing anything stylo-related on beta 56. Can you test on a recent 57 nightly?

Comment 46

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #44)
> I managed to reproduce this issue using Mac OS X 10.11 on Nightly from
> 2017-07-18.
> 
> I retested everything using the same platform on beta 56.0b9. The issues is
> not reproducing anymore, but I have to mention that the video starts to play
> automatically without asking for permission. I even checked in about:addons
> - Plugins and the Flash is set to 'Ask to Activate'. Is that expected
> behaviour?

This is unexpeced behavior. This bug fixed the rendering of the click-to-play feature, and doesn't have any behavior changes. If you still see the bug on latest nightly, please file a new bug.

Comment 47

[Bogdan Maris, Desktop Test Engineering]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #45)
> We shouldn't be testing anything stylo-related on beta 56. Can you test on a
> recent 57 nightly?

I retested again on latest Nightly 57 using Mac OS x10.11, the issue here is not reproducing anymore.

Also can we test this fix somehow in Firefox 56 beta build since the fixes arrived there as well?

As for the autolay issue, I found out that's somewhat intermittent, sometimes it plays straight away, sometimes it does not (clean profiles every time). I'll investigate further and log a bug if necessary.

Comment 48

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #47)
> I retested again on latest Nightly 57 using Mac OS x10.11, the issue here is
> not reproducing anymore.

Thanks for the testing!

> Also can we test this fix somehow in Firefox 56 beta build since the fixes
> arrived there as well?

I've uplifted my patches to beta as in comment 43, so Firefox 56 beta should have them.

Comment 49

[Bogdan Maris, Desktop Test Engineering]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #45)
> We shouldn't be testing anything stylo-related on beta 56. Can you test on a
> recent 57 nightly?

Since we should not test in 56 because of stylo I'll go ahead and close this bug as verified and remove qe-verify.