876426 - Videocontrols don't run adjustControlSize when the element is resized

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In the B2G Youtube app, we have a problem where the <video> element is set to a height of 1px in CSS, and adjustControlSize sets the controls to hidden. Then, the app resizes to a reasonable height, but nothing runs adjustControlSize again so the controls remain hidden when they should be visible.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Fire 'resize' event on video controls element when the element's size is changed

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Call adjustControlSize when the controls are resized for whatever reason

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 754462 [details] [diff] [review]
Part 2: Call adjustControlSize when the controls are resized for whatever reason

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

Throw some more reviewers on the pile

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 754462 [details] [diff] [review]
Part 2: Call adjustControlSize when the controls are resized for whatever reason

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

Throw some more reviewers on the pile

Comment 5

[Justin Dolske [:Dolske]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #0)
> In the B2G Youtube app, we have a problem where the <video> element is set
> to a height of 1px in CSS

Do we know why it's doing this?

Ideally, if it's being dumb the app should be fixed, but if there's some purpose to it (waiting to preload?) we should think about if this is really the right root-cause fix. (Although this fix seems straightforward enough to take in any case -- would have done it this way if bug 227495 had been fixed when implementing!)

Hmm, any perf concerns if someone does a transition/animation that changes size?

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 754462 [details] [diff] [review]
Part 2: Call adjustControlSize when the controls are resized for whatever reason

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

This is adequate to cause the control bar to be shown again, but I think there's still going to be a bug with the click-to-play icon... The code in adjustControls() only knows how to _hide_ it.

Unfortunately that's a bit tricky because it may already have been hidden for other legit reasons, and so the code can't simply set .hidden = true based on size. To fix that we'd need to make this attribute-controlled, via CSS like:

  .clickToPlay[isTooBig] {
      visibility: hidden;
  }

and have this code add/remove the isTooBig attribute on it as needed.

I'm assuming that'll be wanted since bug 876380 talks about other click-to-play issues?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Justin Dolske [:Dolske] from comment #5)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #0)
> > In the B2G Youtube app, we have a problem where the <video> element is set
> > to a height of 1px in CSS
> 
> Do we know why it's doing this?

No. It's obfuscated, but we might be able to figure it out.

> Hmm, any perf concerns if someone does a transition/animation that changes
> size?

adjustControlSize will run on every size change, although most animations/transitions use transforms because they're always a lot faster, and transforms won't affect this.

(In reply to Justin Dolske [:Dolske] from comment #6)
> This is adequate to cause the control bar to be shown again, but I think
> there's still going to be a bug with the click-to-play icon... The code in
> adjustControls() only knows how to _hide_ it.

OK, but that doesn't affect us in this case.

> Unfortunately that's a bit tricky because it may already have been hidden
> for other legit reasons, and so the code can't simply set .hidden = true
> based on size. To fix that we'd need to make this attribute-controlled, via
> CSS like:
> 
>   .clickToPlay[isTooBig] {
>       visibility: hidden;
>   }
> 
> and have this code add/remove the isTooBig attribute on it as needed.

Makes sense.

> I'm assuming that'll be wanted since bug 876380 talks about other
> click-to-play issues?

So far in my testing I haven't seen this problem. I don't think we need to do that immediately.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 754462 [details] [diff] [review]
Part 2: Call adjustControlSize when the controls are resized for whatever reason

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: video controls fail to appear in Youtube app
Testing completed: a small amount of testing of the Youtube app by me and Chris Double
Risk to taking this patch (and alternatives if risky): we could ask Youtube to work around the bug instead. We could do some kind of insane hack to treat height=1 video elements differently and not hide the controls in that case. Otherwise, this is the sanest thing to do.
String or UUID changes made by this patch: none

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 754460 [details] [diff] [review]
Part 1: Fire 'resize' event on video controls element when the element's size is changed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: video controls fail to appear in Youtube app
Testing completed: a small amount of testing of the Youtube app by me and Chris Double
Risk to taking this patch (and alternatives if risky): we could ask Youtube to work around the bug instead. We could do some kind of insane hack to treat height=1 video elements differently and not hide the controls in that case. Otherwise, this is the sanest thing to do.
String or UUID changes made by this patch: none

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9395a7b2e54c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6a72bd7411

Comment 11

[Ed Morley [:emorley]]

Push backed out for crashtest timeouts:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6e02333d572b&jobname=crashtest

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ef539966c3c3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a23a7c47e4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7be40b778117

Comment 12

[Jason Smith [:jsmith]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=876380#c16 in response to comment 11.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The crashtest timeout was caused by the dispatching of this "resize" event.

This was reaching nsGlobalWindow::PreHandleEvent, which set mIsHandlingResizeEvent to true. Normally nsGlobalWindow::PostHandleEvent would set mIsHandlingResizeEvent to false, but in this case since my event doesn't bubble, PostHandleEvent was not called and mIsHandlingResizeEvent is permanently set to true.

That isn't so bad, except nsLocation::Reload has a crazy hack in it:
    // location.reload() was called on a window that is handling a
    // resize event. Sites do this since Netscape 4.x needed it, but
    // we don't, and it's a horrible experience for nothing. In stead
    // of reloading the page, just clear style data and reflow the
    // page since some sites may use this trick to work around gecko
    // reflow bugs, and this should have the same effect.
This crashtest does a cycle of location.reload()s. Since these reloads are basically ignored when mIsHandlingResizeEvent is set, the test hangs.

The solution is to not use the "resize" event name here. I'll use "resizevideocontrols". This fixes the bug and lowers the risk of the patch somewhat.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/603eed8ed8a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5615d916d8

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/ba8ab3f6ab49
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6a665aaf977b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/77740fdf4e84
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ef586800120e

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/603eed8ed8a8
https://hg.mozilla.org/mozilla-central/rev/6a5615d916d8

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Not clear how this got uplifted to v1.0.1 without being tef+ - setting that flag now so we can keep this tracked correctly in case of backout/regressions.

Comment 18

[Jason Smith [:jsmith]]

Works fine on the b2g18 5/31 build.