Closed Bug 449358 Opened 16 years ago Closed 8 years ago

Add test to confirm that video/audio UI controls function when JavaScript is disabled

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox49 --- fixed

People

(Reporter: BijuMailList, Assigned: jaws)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: testcase, Whiteboard: [gs][fixed by bug 834697])

Attachments

(1 file, 3 obsolete files)

http://www.whatwg.org/specs/web-apps/current-work/#controls

>>>> 
The controls attribute is a boolean attribute. If the attribute is present, or if the media element is without script, then the user agent should expose a user interface to the user. 
<<<<

I dont see controls when JavaScript is disabled for
  <video src=support/video.ogg></video>

Steps:
1. From menu goto Tools > Options > Content
2. uncheck "Enable Javascript"
3. Goto URL http://simon.html5.org/test/html/semantics/video/006.htm

Result:-
User dont see controls

Expected:-
There should be controls on video element.
Note: Same applies to audio element too.
Flags: wanted1.9.1?
Behaviour of BUG CHANGED !!!
From: No controls when JavaScript is disabled  
===> Video/Audio UI Controls not functioning when JavaScript is disabled


This bug is partially fixed now...
(may be after patch related to bug 448909 or bug 449522)
Currently user see non-functioning controls over VIDEO.

Steps:
1. From menu goto Tools > Options > Content
2. uncheck "Enable Javascript"
3. Goto URL http://simon.html5.org/test/html/semantics/video/006.htm
4. User see controls

Result:-
But it is not functioning.
a) Pause button is shown instead of play
b) clicking any controls do nothing
c) play video thru context menu, still controls not functioning
d) controls dont disappear after mouse leave video area

Expected:-
Controls should behave the same way as JS enabled.
Summary: No controls when JavaScript is disabled → Video/Audio UI Controls not functioning when JavaScript is disabled
The script in the XBL hides the controls on init, so if the script isn't being run they'll be visible by default. (Conversely, if the  controls were hidden by default, they'd never become visible if its script couldn't run.)
Attached file testcase (obsolete) —
The links given in comment 0 and comment 2 are broken.  To prevent further links becoming broken, I'm attaching a bugzilla-hosted HTML testcase that uses the <video> element.

The video src is a random ogg attachment I found on bugzilla:
  https://bugzilla.mozilla.org/attachment.cgi?id=318816
which is a short screencast of a user interacting with a firefox window.
Here's one other interesting point (with scripting disabled):
 - If I load the HTML testcase (attachment 347660 [details]) ...
   -- Result: I see frozen screenshot of the video, with controls overlayed.

 - If I load OGG directly (attachment 318816 [details]) ...
   -- Result: I see a **blank page**, with controls overlayed.

It seems odd that these two experiments don't have the same results.
I'm not sure what we can do here :-(.

We can probably override the script-enabled setting for stand-alone video documents, I guess. But I'm not sure how to do that.

Fixing this for regular <video> elements sounds really hard.
Flags: wanted1.9.1? → wanted1.9.1+
> I'm not sure what we can do here :-(.

Well... take this into account when making the decision to use the brokenness that is XBL for anything in content?  :(

I suppose we could try just fixing bug 236839...  It might be doable if we add some extra args to nsIScriptContext to override the script-enabled checks it does and then carefully call them from XBL as needed.  We'd still have issues with XBL-attached event handlers, though, without a bit more work.  :(
For overriding for standalone documents, you could add some hackery to security manager, I guess...  The thing is, is that safe?  Someone could inject nodes into that document via DOM manipulation, say.
(In reply to comment #8)
> For overriding for standalone documents, you could add some hackery to security
> manager, I guess...

Would that fix this when scripts are blocked via NoScript, too?  (That's how I originally ran into this bug, and I expect it's how most users would end up running into it.)
I think it should, yes.
(In reply to comment #8)
> For overriding for standalone documents, you could add some hackery to security
> manager, I guess...  The thing is, is that safe?  Someone could inject nodes
> into that document via DOM manipulation, say.

They can only do that with script, but it does sound like they might be able to survive JS being disabled that way.
Also, one site can use script + privileges to inject script that would then run as a different site (for which you have script disabled).  Not a common attack scenario, but still...
(In reply to comment #7)
> Well... take this into account when making the decision to use the brokenness
> that is XBL for anything in content?  :(

We could reimplement it all in C++ I guess. But that sucks pretty bad. I'm not sure if Justin's going to want to keep working on it there.

> I suppose we could try just fixing bug 236839...  It might be doable if we add
> some extra args to nsIScriptContext to override the script-enabled checks it
> does and then carefully call them from XBL as needed.  We'd still have issues
> with XBL-attached event handlers, though, without a bit more work.  :(

That sounds like a lot of work, especially to make secure, but probably something we want XBL2 to be able to handle.

I wonder if we can get away with leaving this unfixed in this release. Maybe we can hack things a little bit so that if script is disabled, controls don't show up.
(In reply to comment #12)
> Also, one site can use script + privileges to inject script that would then run
> as a different site (for which you have script disabled).  Not a common attack
> scenario, but still...

If the site has privileges, can't it just enable script wherever and however it wants? Like anti-Noscript
Yeah, I don't know that we have any good options here.  That's why we need xbl2...

Hacking things up to not show the controls should be easy, I'd think.

Privileges needed for content injection are lower than UniversalXPConnect at the moment.  But yes, the whole thing is maybe just me being paranoid.
(In reply to comment #13)

> We could reimplement it all in C++ I guess. But that sucks pretty bad. I'm not
> sure if Justin's going to want to keep working on it there.

My preference would indeed be to keep it as JS/XUL, since that's what the rest of the front end is, and it makes it more approachable to other front-end folks.
Perhaps some kind of hybrid approach might be possible... EG, a bit of C++ to hook up the events to a JS component instead of code in the binding.

> That sounds like a lot of work, especially to make secure, but probably
> something we want XBL2 to be able to handle.

Would it at all help to do something as a special-case hack, unique to "chrome://global/content/bindings/videocontrols.xml"? Evil, but maybe a limited-scope bandaid could help mitigate risk.

> I wonder if we can get away with leaving this unfixed in this release. Maybe we
> can hack things a little bit so that if script is disabled, controls don't show
> up.

Seems like we probably could get away with leaving this broken for 1 release... Especially if fixing now would require a large and/or risky change. The context menu is there, which is enough to play/pause/mute, so it's not totally broken.

Having the controls default to being hidden should be easy to change.
> Would it at all help to do something as a special-case hack

If we could do that it'd be just as easy to do it for all chrome:// (so fix bug 236839).
(In reply to comment #16)
> Perhaps some kind of hybrid approach might be possible... EG, a bit of C++ to
> hook up the events to a JS component instead of code in the binding.

Maybe but then you'd be dealing with chrome wrappers and stuff. I'm not sure how feasible that is.

> Having the controls default to being hidden should be easy to change.

Let's do that at least.
(In reply to comment #18)

> > Having the controls default to being hidden should be easy to change.
> 
> Let's do that at least.

My patch in bug 469211 will do that.
Blocks: TrackAVUI
Flags: blocking1.9.2?
Can't block on this, there is no reasonable way to fix it. Save us XBL2!
Flags: blocking1.9.2? → blocking1.9.2-
Blocks: 515082
This bug seems to be dependent on bug 236839
Depends on: 236839
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Version: Trunk → unspecified
Depends on: 550557
Currently, audio elements is not displaying _at all_ when JavaScript is disabled. This is like as if, say, hyperlinks would not work when JavaScript is disabled. Just nonsensical.

So, current support for audio/video elements in Firefox is more like a temporary _workaround_ instead of a real native implementation. Very sad.

In Opera and Chrome, audio element DOES work regardless of whether JS is enabled or disabled, and that is really correct implementation.

Hopefully to see this fixed in Firefox as soon as possible. Thanks.
PDF.js is similarly disabled by NoScript even though it, like video/audio, is code hosted by the browser…
Depends on: XBL-scopes
With videos, there are absolutely no controls if Javascript is disabled as of version 18.0.1. I don't think this was the case with version 16, the last version I used before 18.0.1.

This is silly. It really ought to be fixed.
(In reply to onpon4 from comment #33)
> With videos, there are absolutely no controls if Javascript is disabled as
> of version 18.0.1. I don't think this was the case with version 16, the last
> version I used before 18.0.1.

I don't think our behavior has changed on this recently.  I see the same behavior (no controls) in the attached testcase, in both trunk and in Firefox 9.0 (which I happen to have handy), when scripts are disabled.

(It does look like this is different from what was reported in Comment 0, thoguh -- there, we had visible-but-non-functional controls.  So, I think the behavior has changed, but it was a long time ago.)

Incidentally, I've noticed that the context menu controls do work, however (Right-click video --> Play/Pause), in trunk as well as in an old build (ver 9.0)
(In reply to Daniel Holbert [:dholbert] from comment #34)
> I don't think our behavior has changed on this recently.  I see the same
> behavior (no controls) in the attached testcase, in both trunk and in
> Firefox 9.0 (which I happen to have handy), when scripts are disabled.

Something in PDFjs changed recently to get around this limitation, as now I can view pdfs without enabling the domain in noscript. Wonder if that can be ported over.
Brendan, do you know anything about comment #35?
Flags: needinfo?(bdahl)
(In reply to John Drinkwater (:beta) from comment #35)
> Something in PDFjs changed recently to get around this limitation, as now I
> can view pdfs without enabling the domain in noscript. Wonder if that can be
> ported over.

It cannot. PDF.js doesn't have to run in the context of a Web page. Video controls do.

This bug has existed since we first implemented video controls. The good news is that the work in bug 821850 will make it possible to fix this bug. We need to wait for that to land first.
No longer depends on: XBL-scopes
Depends on: 834697
I just tested this out with Tools->Options->Content->Enable Javascript unchecked, restarted the browser, and visited http://mirrorblender.top-ix.org/peach/bigbuckbunny_movies/big_buck_bunny_1080p_stereo.ogg.

The controls now work.

Bug 834697 fixed this.
Assignee: nobody → bobbyholley+bmo
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(bdahl)
Resolution: --- → FIXED
Whiteboard: [gs] → [gs][fixed by bug 834697]
Target Milestone: --- → mozilla21
Can we please add a test for this?  E.g. there's a good chance bug 835643 or the like would re-break this....
Flags: in-testsuite?
Attached patch Testcase (obsolete) — Splinter Review
This is a basic testcase that disables JS, opens a new window, and uses the overlay play button to pause the video. I confirmed locally that if I set dom.xbl_scopes=false then the test fails (it hangs).

https://tbpl.mozilla.org/?tree=Try&rev=16805b025240
Attachment #714565 - Flags: review?(dolske)
Should video controls appear if an add-on like NoScript blocks JS?
Yes -- nothing should ever prevent video controls from displaying and being functional/responsive to user manipulation.
Comment on attachment 714565 [details] [diff] [review]
Testcase

The test is failing on try, so more work needs to be done here.
Attachment #714565 - Flags: review?(dolske)
See Also: → 834697
[removing bug 834697 from "see also". As that field's hover-text indicates, it isn't just for linking to related bugs -- rather, it's specifically for *reports of this same bug in other bug-trackers* -- e.g. if a Firefox bug turns out to be a Gnome bug, or something like that, then that would be an appropriate place to link to the Gnome bug page.  Related mozilla bugs should be tracked as dependencies or duplicates, not via "see also". In this case, bug 834697 is already in this bug's "depends on" field, so we're good.]
See Also: 834697
(In reply to Loic from comment #42)
> Should video controls appear if an add-on like NoScript blocks JS?
That appears to be the case in the latest nightly, with NoScript enabled and the domain untrusted, controls still appear and are usable.

The URL supplied in comment #1 hasn’t aged with spec/useragents - it is missing controls be default - so probably no longer a useful testcase to check.
(In reply to Daniel Holbert [:dholbert] from comment #45)
> [removing bug 834697 from "see also". As that field's hover-text indicates,
> it isn't just for linking to related bugs -- rather, it's specifically for
> *reports of this same bug in other bug-trackers*

The tooltip might say that, but I'm pretty sure that's not how I've seen it exclusively used.  However, I'm too lazy to search them out right now to conclusively rebut the point.  :-)
Reopening and assigning to jaws per comment 44.
Assignee: bobbyholley+bmo → jAwS
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Jared Wein from comment #38)
> Bug 834697 fixed this.
(Unless you whitelisted XBL on the site.)
Unassigning as I haven't been working on this and I don't want to hold people up.
Assignee: jaws → nobody
Since Electrolysis is about using different processes for UI and web content, maybe that will solve that bug (=cutting JavaScript for web content may not affect the UI ?)
The UI that runs in the parent process is the browser chrome (url bar, back/forward button, etc).  The video controls would still be in the content process.

That said, as far as I can tell this bug as filed is no longer a problem because the controls run in the separate XBL scope where script is not disabled.  The only reason this bug is still open is to get an automated test checked in.
Summary: Video/Audio UI Controls not functioning when JavaScript is disabled → Add test to confirm that video/audio UI controls function when JavaScript is disabled
Attached patch Test (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe19145fe813
Attachment #347660 - Attachment is obsolete: true
Attachment #714565 - Attachment is obsolete: true
Comment on attachment 8751816 [details] [diff] [review]
Test

This test is written in the same style as the pre-existing test_videocontrols_*.html files, and I basically rebased the previous attachment and changed from using the "play" event to the "timeupdate" event. I would prefer not to rewrite it using the latest conventions, but I am open ears.
Attachment #8751816 - Flags: review?(gijskruitbosch+bugs)
Assignee: nobody → jaws
Status: REOPENED → ASSIGNED
Comment on attachment 8751816 [details] [diff] [review]
Test

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

::: toolkit/content/tests/widgets/test_videocontrols_jsdisabled.html
@@ +11,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +function runTest(event) {
> +  ok(true, "----- test #" + testnum + " -----");

Nit: info()

@@ +43,5 @@
> +
> +  testnum++;
> +}
> +
> +SpecialPowers.setBoolPref("javascript.enabled", false);

I'm fairly suspicious that this does not in fact turn off JS. Either way I think we should be using pushPrefEnv to avoid this leaking to other tests if the test fails.

Can you add a <script> thing in the data URI that sets an expando on the window, and then check that the expando is *not* present when the load has happened ?

@@ +56,5 @@
> +  video.addEventListener("pause", runTest, false);
> +}
> +
> +var a = document.createElement("a");
> +a.href = "seek_with_sound.ogg";

Nit:

var videoURL = new URL("seek_with_sound.ogg", document.documentURI).href;

and then include that in the link.
Attachment #8751816 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #56)
> @@ +43,5 @@
> > +
> > +  testnum++;
> > +}
> > +
> > +SpecialPowers.setBoolPref("javascript.enabled", false);
> 
> I'm fairly suspicious that this does not in fact turn off JS. Either way I
> think we should be using pushPrefEnv to avoid this leaking to other tests if
> the test fails.
> 
> Can you add a <script> thing in the data URI that sets an expando on the
> window, and then check that the expando is *not* present when the load has
> happened ?

To clarify slightly: I think it might work right now (I'm not sure) but when behaviour was changed so that it no longer turned JS off that was already running (ie pages that were open before you changed the pref), people complained. From what I recall, it isn't particularly "supported" as a thing to do, and I would hate for the test to basically not be testing anything.
Attachment #8751816 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #57)
> (In reply to :Gijs Kruitbosch from comment #56)
> > @@ +43,5 @@
> > > +
> > > +  testnum++;
> > > +}
> > > +
> > > +SpecialPowers.setBoolPref("javascript.enabled", false);
> > 
> > I'm fairly suspicious that this does not in fact turn off JS. Either way I
> > think we should be using pushPrefEnv to avoid this leaking to other tests if
> > the test fails.
> > 
> > Can you add a <script> thing in the data URI that sets an expando on the
> > window, and then check that the expando is *not* present when the load has
> > happened ?
> 
> To clarify slightly: I think it might work right now (I'm not sure) but when
> behaviour was changed so that it no longer turned JS off that was already
> running (ie pages that were open before you changed the pref), people
> complained. From what I recall, it isn't particularly "supported" as a thing
> to do, and I would hate for the test to basically not be testing anything.

Right, I agree. A misleading test is worse than no test at all. I needed to rely on the pushPrefEnv callback to wait long enough for JS to be disabled in the new tab. I confirmed that the test passed and failed with/without the pushPrefEnv call, respectively.
https://hg.mozilla.org/mozilla-central/rev/51d7f9e94442
Status: ASSIGNED → RESOLVED
Closed: 11 years ago8 years ago
Resolution: --- → FIXED
Depends on: 1272646
You need to log in before you can comment on or make changes to this bug.