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

VERIFIED FIXED in Firefox 56

Status

()

P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: pdol, Assigned: TYLin)

Tracking

(Blocks: 2 bugs)

56 Branch
mozilla57
Unspecified
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 unaffected, firefox56 fixed, firefox57 verified)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8887494 [details]
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
(Reporter)

Comment 1

a year ago
Created attachment 8887496 [details]
Actual-UI.png

Comment 2

a year ago
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?
Flags: needinfo?(bugs)
(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
Blocks: 1320841
Component: Plug-ins → CSS Parsing and Computation
Flags: needinfo?(bugs)
Summary: Flash click to play doesn't display on video elements when Stylo is enabled → Stylo: Flash click to play doesn't display on video elements when Stylo is enabled
Blocks: 1375906
IIRC the click-to-play stuff is done with an XBL binding, so it might be a good thing for TYLin to look at.
(Assignee)

Updated

a year ago
Flags: needinfo?(tlin)
Priority: -- → P2
Assignee: nobody → tlin
Duplicate of this bug: 1383633
(Assignee)

Comment 6

a year ago
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
Status: NEW → ASSIGNED
Flags: needinfo?(tlin)
Comment hidden (mozreview-request)
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

a year ago
mozreview-review
Comment on attachment 8889849 [details]
style: Rename PSEUDO_CLASS_INTERNAL to PSEUDO_CLASS_ENABLED_IN_UA_SHEETS

https://reviewboard.mozilla.org/r/160892/#review166652
Attachment #8889849 - Flags: review?(cam)
Priority: P2 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

a year ago
mozreview-review
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.)
Attachment #8889849 - Flags: review?(cam) → review+

Comment 17

a year ago
mozreview-review
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

a year ago
mozreview-review
Comment on attachment 8891416 [details]
style: Update comments for push_applicable_declarations_as_xbl_only_stylist()

https://reviewboard.mozilla.org/r/162478/#review168196
Attachment #8891416 - Flags: review?(cam) → review+

Comment 19

a year ago
mozreview-review
Comment on attachment 8891418 [details]
Bug 1381851 - Update comments for nsXBLPrototypeResources::ComputeServoStyleSet().

https://reviewboard.mozilla.org/r/162482/#review168198
Attachment #8891418 - Flags: review?(cam) → review+
(Assignee)

Comment 20

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 21

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Priority: P1 → --
Priority: -- → P1

Comment 28

a year ago
mozreview-review
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())
Attachment #8891415 - Flags: review?(cam) → review+

Comment 29

a year ago
mozreview-review
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://.
Attachment #8891417 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8889849 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8891415 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8891416 - Attachment is obsolete: true
(Assignee)

Comment 32

a year ago
Created attachment 8893271 [details] [review]
Servo PR #17958

Comment 33

a year ago
Pushed by ecoal95@gmail.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/323e0b914fd6
https://hg.mozilla.org/mozilla-central/rev/e1c23455946b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Updated

a year ago
Duplicate of this bug: 1364303
Ting-Yu, should we uplift this Flash fix to Beta 56? Some websites could be broken if Flash click-to-play doesn't work.
status-firefox55: --- → unaffected
status-firefox56: --- → affected
Flags: needinfo?(tlin)
(Assignee)

Comment 37

a year ago
Created attachment 8894394 [details] [diff] [review]
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
(Assignee)

Comment 38

a year ago
(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.
Flags: needinfo?(tlin)
(Assignee)

Updated

a year ago
Attachment #8894394 - Flags: approval-mozilla-beta?
Comment hidden (typo)
(Assignee)

Comment 40

a year ago
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
Blocking bug 1381147 because we want to fix this crash before starting the Stylo experiment on Beta 56.
Blocks: 1381147
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.
Attachment #8891417 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8894394 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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?
Flags: needinfo?(tlin)
We shouldn't be testing anything stylo-related on beta 56. Can you test on a recent 57 nightly?
(Assignee)

Comment 46

a year ago
(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.
Flags: needinfo?(tlin)
(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.
status-firefox57: fixed → verified
Flags: needinfo?(tlin)
(Assignee)

Comment 48

a year ago
(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.
Flags: needinfo?(tlin)
(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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.