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 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 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

(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 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?

(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 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 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

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

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

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

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.

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

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

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

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.

Works fine on the b2g18 5/31 build.