Closed Bug 1273468 Opened 8 years ago Closed 8 years ago

Full-screen button is missing in 47 Beta 6

Categories

(Toolkit :: Video/Audio Controls, defect)

47 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 + verified
firefox48 --- verified
firefox49 --- verified

People

(Reporter: sflorean, Assigned: xidorn)

References

Details

(Keywords: regression)

Attachments

(1 file)

Environment: 
Device: Asus Transformer Pad (Android 4.2.1);
Build: Beta 47.0b6;

Steps to reproduce:
1. Go to http://people.mozilla.com/~nhirata/html_tp/elephants-dream.webm;
2. Tap the video to show the video controls.

Expected result:
Full-screen button is displayed near speaker button.

Actual result:
Full-screen button is missing.
Ray, can you investigate this?
Flags: needinfo?(ralin)
Tracked for Fx47 since it's a recent regression.
Do we know the regression range?
(In reply to :Margaret Leibovic from comment #1)
> Ray, can you investigate this?
No problem, I'll see what I can do.


Thank you Tim
It would really help if range provided, since mozregression seems not support fennec beta.
Flags: needinfo?(ralin)
Regression:
On 47 Beta 4 - full-screen button is displayed
On 47 Beta 6 - full-screen button is not displayed

Pushlog between the two betas: https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FENNEC_47_0b4_BUILD1&tochange=FENNEC_47_0b6_BUILD1

Looking at the pushlog I think that Bug 1268750 might be the cause of this issue.
The bug # on the commit message is incorrect. The correct one is bug 1268749.

We would need to figure out where the unprefixed Fullscreen API is used in the front-end code and revert it, on beta channel.
Assignee: nobody → ralin
quick test about prefs: https://reviewboard.mozilla.org/r/49963/diff/3#3

Firefox beta on LG G2, manually toggle "full-screen-api.unprefix.enabled" to "true" and fullscreen button is back.

Steps:
1. Download firefox beta from google play
2. Go to http://people.mozilla.com/~nhirata/html_tp/elephants-dream.webm and found full screen button is missing
3. Open new tab to about:config, and toggle "full-screen-api.unprefix.enabled" to value "true"
4. Switch to video tab and reload the page.... then full screen button is back
---- reconfirm ----
5. Toggle "full-screen-api.unprefix.enabled" to "false"
6. Switch back to video tab and reload. Now full screen button is missing again...
Updates:

video control ui changed from prefix api to unpreifx:
https://hg.mozilla.org/releases/mozilla-beta/diff/55ecf767b62d/toolkit/content/widgets/videocontrols.xml#l1.44

It is clear that we moved to unprefix api on front end. As line 44 in videocontrols.xml, if .fullscreenEnabled is undefined(full-screen-api.unprefix.enabled = false), the full screen button fallback to like as audioOnly. I'm not sure how to deal with this regression, it seems more followup patches are related to converting to unprefixed full screen api[0].

Margaret, is anything I could do to fix it?

[0] https://hg.mozilla.org/mozilla-central/filelog/f3f2fa1d7eed5a8262f6401ef18ff8117a3ce43e/dom/webidl/Document.webidl
Flags: needinfo?(margaret.leibovic)
I'll take this.
Assignee: ralin → bugzilla
Component: Audio/Video → Video/Audio Controls
Flags: needinfo?(margaret.leibovic)
OS: Android → All
Product: Firefox for Android → Toolkit
Hardware: ARM → All
So it seems video document doesn't have chrome permission, and thus unprefixed fullscreen api is not exposed if it is disabled. I'd revert the usage there back to prefixed API, and file another bug for using unprefixed API.
The patch is basically just a revert of the toolkit part in bug 743198 part 8.
Attachment #8754142 - Flags: review?(dolske) → review+
Comment on attachment 8754142 [details]
MozReview Request: Bug 1273468 - Revert video controls to use prefixed Fullscreen API again. r?dolske

https://reviewboard.mozilla.org/r/53766/#review50486

rs=me for basically backing out the changes in bug 743198.

What's the plan for going back to the unprefixed version? Do we expect to land this only on beta of fix things soon, or is this a longer term thing?
Blocks: 1274098
(In reply to Justin Dolske [:Dolske] from comment #13)
> What's the plan for going back to the unprefixed version? Do we expect to
> land this only on beta of fix things soon, or is this a longer term thing?

Filed bug 1274098. It would also need to land on 48 and 49, and probably stay there as long as we still have the separate pref for unprefixed Fullscreen API.
Comment on attachment 8754142 [details]
MozReview Request: Bug 1273468 - Revert video controls to use prefixed Fullscreen API again. r?dolske

Approval Request Comment
[Feature/regressing bug #]: bug 1268749
[User impact if declined]: fullscreen won't work properly for video documents when unprefixed Fullscreen API is disabled (by default for release versions)
[Describe test coverage new/current, TreeHerder]: looks like no coverage currently
[Risks and why]: low risk, it is just a revert of toolkit part in bug 743198, makes video documents to use prefixed Fullscreen API again
[String/UUID change made/needed]: n/a
Attachment #8754142 - Flags: approval-mozilla-beta?
Attachment #8754142 - Flags: approval-mozilla-aurora?
hmmm, wait... I suppose it would break test_videocontrols.html on beta...
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> hmmm, wait... I suppose it would break test_videocontrols.html on beta...

Hmmmm, it won't... test_videocontrols.html is run as a mochitest-chrome, which does have chrome permission and thus always have access to unprefixed Fullscreen API.

I think we should make that test only have content permission so that we can catch regressions which only present in content.
Looks like test_videocontrols.html was moved from mochitest to mochitest-chrome in bug 694696, and the reason was described in bug 694696 comment 18. Not sure what should we do here...
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #16)
> [User impact if declined]: fullscreen won't work properly for video
> documents when unprefixed Fullscreen API is disabled (by default for release
> versions)

Not only video documents. All video controls are affected.
https://hg.mozilla.org/integration/mozilla-inbound/rev/33509142de5fe1f6357006c391de85d7a6f11943
Bug 1273468 - Revert video controls to use prefixed Fullscreen API again. r=dolske
Blocks: 1274104
https://hg.mozilla.org/mozilla-central/rev/33509142de5f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8754142 [details]
MozReview Request: Bug 1273468 - Revert video controls to use prefixed Fullscreen API again. r?dolske

Recent regression, Aurora48+, Beta47+
Attachment #8754142 - Flags: approval-mozilla-beta?
Attachment #8754142 - Flags: approval-mozilla-beta+
Attachment #8754142 - Flags: approval-mozilla-aurora?
Attachment #8754142 - Flags: approval-mozilla-aurora+
Hi Sorina, could you please verify this issue is fixed as expected on a latest Nightly/Aurora/Beta build? Thanks!
Flags: needinfo?(sorina.florean)
To verify the fix, you need to turn off unprefixed fullscreen api via setting full-screen-api.unprefix.enabled to false. With the unprefixed api enabled, this bug would not appear.
I'm hitting conflicts uplifting this to aurora:
$ hg graft -er 33509142de5f
grafting 343977:33509142de5f "Bug 1273468 - Revert video controls to use prefixed Fullscreen API again. r=dolske"
merging toolkit/content/TopLevelVideoDocument.js
merging toolkit/content/tests/widgets/test_videocontrols.html
merging toolkit/content/widgets/videocontrols.xml
warning: conflicts while merging toolkit/content/tests/widgets/test_videocontrols.html! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(bugzilla)
Hi Ritu,

Tested using:
Asus Transformer Pad (Android 4.2.1) 
LG G4 (Android 5.1)
Nexus 5 (Android 6.0.1)

After May 18th, on https://ftp.mozilla.org/pub/mobile/nightly/2016/05/ aren't builds available.
Build: Nightly 49.0a1 (2016-05-18), on phone and tablet: 
-Full-screen-api.unprefix.enabled to false-> full-screen button is missing;
-Full-screen-api.unprefix.enabled to true-> full-screen button is displayed;
 

Build: Aurora 48.0a2 (2016-05-22), on phone: 
-Full-screen-api.unprefix.enabled to false-> full-screen button is missing;
-Full-screen-api.unprefix.enabled to true-> full-screen button is missing;  

Build: Aurora 48.0a2 (2016-05-22), on tablet: 
-Full-screen-api.unprefix.enabled to false-> full-screen button is displayed;
-Full-screen-api.unprefix.enabled to true-> full-screen button is displayed;
Hi, :sorina

Is missing button on Aurora because of bug 1267935?
(In reply to Ray Lin[:ralin] from comment #32)

> Is missing button on Aurora because of bug 1267935?

I think that Bug 1267935 might be the problem, who is only affecting Aurora. Please see that on phone and tablet, the behavior is different.
Flags: needinfo?(sorina.florean)
On 47 Beta 8 full-screen button is displayed.
Tested using Nexus 5 (Android 6.0.1) and Samsung Galaxy R (Android 2.3.4).
Depends on: 1332699
You need to log in before you can comment on or make changes to this bug.