Closed Bug 1659761 Opened 4 years ago Closed 4 years ago

Fullscreen video on desktop Youtube, southpark cc not fullscreen, controls broken

Categories

(Core :: Layout, defect)

Unspecified
All
defect

Tracking

()

VERIFIED FIXED
82 Branch
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox79 --- unaffected
firefox80 --- wontfix
firefox81 --- fixed
firefox82 --- verified

People

(Reporter: liuche, Assigned: botond)

References

(Regression)

Details

(Keywords: regression, webcompat:needs-diagnosis, Whiteboard: [geckoview])

Attachments

(2 files)

Attached image southpark-fullscreen.jpg (deleted) —

Filed from Github issues: https://github.com/mozilla-mobile/fenix/issues/13773 and https://github.com/mozilla-mobile/fenix/issues/13790

Sounds like regression on 80, and doesn't show up on 79 from reports

STR #1 southpark

Steps to repro:
Go to https://southpark.cc.com/full-episodes
Play video in full screen.

Expected:
Video should be play in full screen immersive mode hiding android navigation bar.

Actual behavior
In landscape Full screen mode there's a white bar in bottom and side of video and video is smaller than fullscreen. Video isn't in immersive landscape mode.
The south park cc site video seek bar, subtitle button, Full screen switch button are not responding.

STR #2 Youtube desktop

When you open Youtube in desktop browser view and switch to landscape mode, it offsets the video to the top right corner and white in the remaining spaces. Controls like seek, full-screen, etc. trigger in the normal locations as if there's no offset even of the controls show offset as well. Noticed this on both firefox beta and nightly (android).

Repro-ing on the current Nightly 8/17 w/ GV

I can repro this in desktop using responsive design mode. It think the culprit may be:

<meta name="viewport" content="width=640, user-scalable=no" id="viewport">

It seems like this is probably web compat or layout.

Flags: needinfo?(miket)

The user does say that Fenix 79 doesn't have this problem, which points to it being a regression rather than a webcompat issue though.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #1)

I can repro this in desktop using responsive design mode. It think the culprit may be:

<meta name="viewport" content="width=640, user-scalable=no" id="viewport">

It seems like this is probably web compat or layout.

I wouldn't expect meta viewport to affect fullscreen video, but Karl knows more about viewport than I do. Karl, ever seen anything like this?

Flags: needinfo?(miket) → needinfo?(kdubost)
[EdgePlayer] ERROR [177]  Error in "FULL_SCREEN_CHANGE" callback. FULL_SCREEN_CHANGE@https://southpark.cc.com/sitewide/js/app/main-packaged.js:15130:38
trigger/<@https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1:555720
trigger@https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1:555694
onFullscreenChange@https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1:378779
E/</n.proxy@https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1:37334
nrWrapper@https://southpark.cc.com/full-episodes/s23e10-christmas-snow:12:25380
 TypeError: can't access property "style", document.querySelector(...) is null
    FULL_SCREEN_CHANGE https://southpark.cc.com/sitewide/js/app/main-packaged.js:15130
    trigger https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1
    trigger https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1
    onFullscreenChange https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1
    proxy https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1
    nrWrapper https://southpark.cc.com/full-episodes/s23e10-christmas-snow:12
 
Object { callback: FULL_SCREEN_CHANGE(event)
, context: undefined, ctx: {…} }
edgeplayer.min.js:1:17751
Uncaught TypeError: can't access property "style", document.querySelector(...) is null
    FULL_SCREEN_CHANGE https://southpark.cc.com/sitewide/js/app/main-packaged.js:15130
    trigger https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1
    trigger https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1
    onFullscreenChange https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1
    proxy https://player.mtvnservices.com/edge/player/3.0.1/edgeplayer.min.js:1
    nrWrapper https://southpark.cc.com/full-episodes/s23e10-christmas-snow:12
main-packaged.js:15130:38

​

The player init is

var playerInit = function () {
  VideoPlayer.init(
    $.extend(
      {
        id: self.mgid,
        size: {
          width: parseInt($videoContainer.width(), 10),
          height: parseInt($videoContainer.width() * (9 / 16), 10),
        },
        events: {
          PLAYLIST_END: playlistCompleteCallback,
          FULL_SCREEN_CHANGE: function (event) {
            document.querySelector(".evidon-banner").style.display = event.data
              ? "none"
              : "";
          },
        },
      },
      window.playerConfig.playerVars
    ),
    window.playerConfig.containerID
  );

  var resizePlayerOnViewportSizeChange = function () {
    var playerId = "#player_page_player";
    var $videoPlayer = self.ui.$video.find(playerId);
    var $videoPlayerIframe = $videoPlayer.children("iframe");

    // No Videos Exist
    if ($videoPlayer.length == 0) {
      return;
    }

    // Resize Video
    $videoPlayer.width($videoContainer.width());
    $videoPlayer.height($videoContainer.width() * (9 / 16));

    // Hulu
    if (typeof NewSite != "undefined")
      NewSite.videoPlayerComponent.setSize(
        $videoContainer.width(),
        $videoContainer.width() * (9 / 16)
      );
  };

  if (mobileDetect.isIpad() || mobileDetect.isAndroidTablet()) {
    $window.on("HEADER::orientationChange", function () {
      resizePlayerOnViewportSizeChange();
    });
  } else {
    // Window Resize
    $window.on("WINDOW::resize", resizePlayerOnViewportSizeChange);
  }

  $(document).trigger("SP::PlayerInit");
};

Hiroyuki, does the viewport size change when switching to full screen on fenix?

Flags: needinfo?(kdubost) → needinfo?(hikezoe.birchill)

The viewport size could be changed because we don't allow minimum-scale size on fullscreen state, I tried to drop the restriction in bug 1650686, but it's not yet happened. Anyways, the restriction is only affecting where the minimum-scale size is applied to the document, is the document having an element bigger than the initial containing block size?

That said, even if the viewport size is changed, I don't know how it's affecting the video size at the first glance of the code in comment 4.

Probably the best way to move this issue forward to find out the regression range.

Flags: needinfo?(hikezoe.birchill)

On the RDM, I saw the video is rendered at the bottom on full screen state, so maybe it's not the same issue, but I narrowed it down but mozregression failed with 'taskId' error, probably it's due to bug 1655682.

Anyways the range I got is;
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dd1766e040a2db3cf1497ffe4b221e5082a831ee&tochange=2aa3b889d60386ac20e2a7ab7f315a742a2eea87

The most suspicious one is bug 1650488. That said, the change itself looks pretty reasonable to me. So there must be something badly relying on the async zoom container in full screen stuff.

CCing kats.

I could narrow down the range further; https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3f4573c955ea4e1f9b7ee38e0254ca3d13d095b5&tochange=7e46319259d9d69f1dcb5dbdbca0677f385ab8c6

Bug 1650488 is still there. So I am ~100% sure it's the regressioner of the RDM issue, but I still believe it's just a trigger of unwallpapering a pre-existing issue.

Another interesting thing is I can't reproduce the original issue on a local built Fenix based on m-c: 069bb8bd2356 and based on fenix: 751efb7bfbc2930fcbe5cd4a1b90f9ffca8c701e.

Component: General → Layout
Product: GeckoView → Core
Whiteboard: [geckoview]

I could mange to reproduce the original issue on a local built Fenix on Android emulator. To reproduce the issue, I had to change the device resolution by using "adb shell wm size 1080x1920".

And dropping !document->Fullscreen() check introduced in bug 1650488 fixes the original issue actually. Nothing more I've done so far.

Regressed by: 1650488
Has Regression Range: --- → yes

Set release status flags based on info from the regressing bug 1650488

Kats, given the regressing change Hiro identified here, can you help at all?

Severity: -- → S2
Flags: needinfo?(kats)

I'll try to get to this next week if nobody else gets to it before then. I can't repro using the southpark STR because playing videos there is blocked in Canada. I see wonky behaviour on youtube but I'm not sure if it's the same as what is described in this bug. So it might be hard for me to track this down, but I'll see what I can do.

Assignee: nobody → kats
Flags: needinfo?(kats)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #12)

I can repro with https://www.w3.org/2010/05/video/mediaevents.html

What I see on this is similar to what I was seeing on youtube - when I enter fullscreen, the video is zoomed in rather than zoomed to fit. Which is clearly a bug, but sounds different from what the "actual results" in comment 0 is describing.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #12)

I can repro with https://www.w3.org/2010/05/video/mediaevents.html

What I see on this is similar to what I was seeing on youtube - when I enter fullscreen, the video is zoomed in rather than zoomed to fit. Which is clearly a bug, but sounds different from what the "actual results" in comment 0 is describing.

Yeah, I guess I sorta think they're the same bug?

The change made in bug 1650488 is not to build an async zoom container if we're in fullscreen mode. This has the effect of not applying any APZ resolution to the page contents.

On desktop, this is fine, since you can't zoom while in fullscreen mode. However, on Android, allowing the user to zoom is not the only purpose of the APZ resolution -- it's also used to respect the initial scale computed for mobile viewport sizing purposes (commonly, on desktop sites, to zoom out such that the ICB size (which for desktop sites is usually wider than the screen) fits visually into the screen size). It appears that this remains important in fullscreen mode, too, i.e. the fullscreen page contents are still sized to the ICB size (and with a fixed width in the meta viewport tag, the ICB size does not change based on being on fullscreen mode).

So, I don't think the fix in bug 1650488 is appropriate for mobile. We may just want to restrict it to desktop only.

Thanks! Your reasoning makes sense. I'm assigning the bug to you since you already have a patch up :)

Assignee: kats → botond
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

Since Fenix is doing a big release with 81, could we get an uplift to beta as well?

Flags: needinfo?(botond)

Comment on attachment 9172292 [details]
Bug 1659761 - Restrict the fix of bug 1650488 to desktop only. r=kats

Beta/Release Uplift Approval Request

  • User impact if declined: On some pages on android (particularly desktop-mode sites), fullscreening a video can result in incorrect rendering (e.g. the video being sized larger than the screen such that only part of the video is visible).
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): We are just reverting a change (bug 1650488) that was motivated by desktop zooming, for android. After this change, the android behaviour will return to known-good state prior to bug 1650488.

In terms of test coverage, we have coverage for this patch not regressing bug 1650488 on desktop, but not for the issue being fixed on android.

  • String changes made/needed: none
Flags: needinfo?(botond)
Attachment #9172292 - Flags: approval-mozilla-beta?

Comment on attachment 9172292 [details]
Bug 1659761 - Restrict the fix of bug 1650488 to desktop only. r=kats

Approved for 81.0b4.

Attachment #9172292 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Hi, verified as fixed on the latest Nightly Build 200831 using the following devices:
• Google Pixel 3a (Android 10)
• Huawei Mate 20 Lite (Android 10)
• OnePlus A3 (Android 6.0.1)

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: