User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0 Build ID: 20170414022702 Steps to reproduce: visit http://www.wueste-welle.de/mediathek/viewsendung/id/17053 click "Sendung abspielen" (play show) Actual results: Window with html5 audio player opens, sound runs no controls visible nor available Expected results: Window with html5 audio player opens, sound runs controls visible, available, and ready for handling This worked flawlessly in the past, and does still with chromium Downloaded older firefox version (52.02) to doublecheck, but didn't succed to start it. 53.0 opened even when starting older binary.

propably interesting even if no form or dropdown available(??): User agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0 OS: Linux 4.10.13-1-ARCH Installation from mozilla.org not from arch-Package.

I can reproduce the problem on Windows10. Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0a0ad749ef253283e2288b5bcac8ea7a06b16eed&tochange=baff63f434f73b9b70aff0fbbcad42ed7c777f71 Regressed by: aff63f434f7 Ray Lin — Bug 1339269 - Enhance controls adjustment against delay layout reflow. r=jaws

The <audio> inside popup window set 30px to its maximum height, and the new media controls' min-required height is 40px which is 12px higher than the original media controls. It's expected that we hide the entire media controls if the given size is smaller than required size, and that's what we've been behaving for long[0][1]. I'm worried how many website would be like this case, and wondering if we need to care about whether controls can fit in or not. [0] https://hg.mozilla.org/mozilla-central/file/b20c524ca0c4/toolkit/content/widgets/videocontrols.xml#l1524 [1] https://dxr.mozilla.org/mozilla-central/rev/33b92d9c40562dab3d7b602368c75619f1d793f7/toolkit/content/widgets/videocontrols.xml#1745-1748

This screenshot shows that the max-height: 30px is also too small for chrome, you can see the top border is cut off. The difference is that chrome still show up partial controls even if it could not fit in, but our media hide entirely.

Hi Jared, Do you have idea about this case? Perhaps, media controls can show partially if no enough space? Thanks.

Edge hides their control when the width is lower than 29px, and Chrome seems to show their controls all the way down to max-width:1px. Can we apply CSS transform:scale(.69); when the height is lower than 40px? That will reduce the height of the controls to 28.98px which should round up to 29px. You should also set transform-origin:bottom; to make sure that it still is at the bottom of the video and doesn't move up.

Ray, can you try implementing Jared's fix from comment 7?

Sorry Nathan, I'm afraid that I don't have cycle to full-time work on this until 57, but I'll spend some time test it locally.

Taking this bug as controls adjustment is still problematic in certain cases: Bug 1367875, Bug 1367846, Bug 1367868

remove all blocks as I filed another metabug for all layout adjustment issues.

Hi Gijs, Could you help me to review this patch if you got some times? Thank you.

Comment on attachment 8873027 [details] Bug 1362146 - Rewrite media controls adjustment that always show audio controls instead of hiding entirely when the given height is smaller than 40px and avoid recursively calling reflow handling. https://reviewboard.mozilla.org/r/144512/#review148746 I'm not convinced this is entirely right. I suspect Jared might be a better reviewer. I noticed an issue, besides what is listed below: if I change the testcase Alice helpfully provided to use <video> instead of <audio>, we don't show the same set of controls. Is that intentional? I don't think it's a consequence of this patch, but I'm a bit surprised nonetheless. data:text/html,<video src="https://upload.wikimedia.org/wikipedia/commons/b/bb/Test_ogg_mp3_48kbps.wav" type="audio/mp3" id="player2" controls="controls" autoplay="autoplay" style="max-width: 300px;max-height: 30px;"></video> ::: toolkit/content/widgets/videocontrols.xml:1774 (Diff revision 1) > this.clickToPlay.style.height = `${clickToPlayScaledSize}px`; > } > + > + > + // If the given height is smaller than controlBarMinHeight we hide control bar entirely > + // for video. However, We don't want to hide control bar for audio as it doesn't cover Nit: capitalization - "However, we" - 'we' should be lowercase. Also, here and the previous sentence please put 'the' in front of 'control bar'. ::: toolkit/content/widgets/videocontrols.xml:1783 (Diff revision 1) > + } else { > + this.controlsContainer.style.transformOrigin = ""; > + this.controlsContainer.style.transform = ""; > + } This will only remove the transform if this.isAudioOnly is still true. I think the `givenHeight` assignment should be outside the if block, and then the if block should look like this: ```js if (this.isAudioOnly && givenHeight < this.controlBarMinheight) { // set transform } else { // reset transform } ``` Though I will also note that this is introducing a synchronous style flush, because we've just changed the click to play overlay and/or control bar spacer state. It seems like it'd be better if we didn't need to do that, but I'm unsure if we can avoid it. Could we use the video metadata height, or is that not always available here? Could we read the height earlier, to avoid the 'read, set, read' sequence ? Or could we read the height lazily, with domwindowutils?

(In reply to :Gijs from comment #14) > I noticed an issue, besides what is listed below: if I change the testcase > Alice helpfully provided to use <video> instead of <audio>, we don't show > the same set of controls. Is that intentional? I don't think it's a > consequence of this patch, but I'm a bit surprised nonetheless. > no, the consequence is definitely wrong...thanks you and Alice for providing this case. > data:text/html,<video > src="https://upload.wikimedia.org/wikipedia/commons/b/bb/Test_ogg_mp3_48kbps. > wav" type="audio/mp3" id="player2" controls="controls" autoplay="autoplay" > style="max-width: 300px;max-height: 30px;"></video> > > ::: toolkit/content/widgets/videocontrols.xml:1774 > (Diff revision 1) > > this.clickToPlay.style.height = `${clickToPlayScaledSize}px`; > > } > > + > > + > > + // If the given height is smaller than controlBarMinHeight we hide control bar entirely > > + // for video. However, We don't want to hide control bar for audio as it doesn't cover > > Nit: capitalization - "However, we" - 'we' should be lowercase. > > Also, here and the previous sentence please put 'the' in front of 'control > bar'. > I'll pay more attention to typo next time, thanks. > ::: toolkit/content/widgets/videocontrols.xml:1783 > (Diff revision 1) > > + } else { > > + this.controlsContainer.style.transformOrigin = ""; > > + this.controlsContainer.style.transform = ""; > > + } > > This will only remove the transform if this.isAudioOnly is still true. > > I think the `givenHeight` assignment should be outside the if block, and > then the if block should look like this: > > ```js > if (this.isAudioOnly && givenHeight < this.controlBarMinheight) { > // set transform > } else { > // reset transform > } > ``` > much clearer, thanks > Though I will also note that this is introducing a synchronous style flush, > because we've just changed the click to play overlay and/or control bar > spacer state. It seems like it'd be better if we didn't need to do that, but > I'm unsure if we can avoid it. Could we use the video metadata height, or is > that not always available here? Could we read the height earlier, to avoid > the 'read, set, read' sequence ? Or could we read the height lazily, with > domwindowutils? yeah, we can simplify the sequence to 'read set'. We don't need read again as those changes doesn't affect the given height if it's a audio only controls. > I'm not convinced this is entirely right. I suspect Jared might be a better > reviewer. Thanks though, your review really helps me to rethink some of the issues caused by style flush at wrong timing :-) I'll keep update the patch and wait for Jared to come back.

Comment on attachment 8873027 [details] Bug 1362146 - Rewrite media controls adjustment that always show audio controls instead of hiding entirely when the given height is smaller than 40px and avoid recursively calling reflow handling. removing review flag as mozreview doesn't update accordingly.

data:text/html,<video src="https://upload.wikimedia.org/wikipedia/commons/b/bb/Test_ogg_mp3_48kbps.wav" type="audio/mp3" id="player2" controls="controls" autoplay="autoplay" style="max-width: 300px;max-height: 30px;"></video> (In the screenshot, I added the red border around in order to see the real dimensions of the element) Sorry I was wrong about the consequence. It seems that, for the mainstream browsers, they don't restore to audio controls layout after knowing it's an audio instead of video. And the final size: 60x30 is computed with the default size(300 x 150 if no size given, i.e. audio[0]), which latter being resized to 60x30 per the max-height: 30px and its ratio: 2:1. In 52, we don't collapse the controls if no enough space is given, but the rest of UI outside red border is actually unclickable. Therefore, maybe the shrunk layouts is more sensible for this case. [0] https://hg.mozilla.org/mozilla-central/file/103dc4eddacf/layout/generic/nsVideoFrame.cpp#l604

(In reply to Ray Lin[:ralin] from comment #3) > The <audio> inside popup window set 30px to its maximum height, and the new > media controls' min-required height is 40px which is 12px higher than the > original media controls. > > It's expected that we hide the entire media controls if the given size is > smaller than required size, and that's what we've been behaving for > long[0][1]. I'm worried how many website would be like this case, and > wondering if we need to care about whether controls can fit in or not. Same issue reported on the French community board: https://forums.mozfr.org/viewtopic.php?f=8&t=133567 With this URL: http://www.harmonielim.apass.fr/index.php?option=com_content&view=article&id=36&Itemid=172 The style for <audio> in this webpage is: <audio controls style="max-height: 30px;"> </audio>

max-height's issue was verified as fixed in Bug 1367846. I've tested this test case with latest m-c build, and the media controls is now visible, so marking this as duplicate. Thanks.

Per bug 1367846, mark 54 won't fix and 55/56 fixed.