1362146 - html5 audio player controls not present

Status: RESOLVED DUPLICATE of bug 1367846

(Toolkit :: Video/Audio Controls, defect)

Description

[Friedrich Strohmaier]

Attachment: screenshot htm5 audio player firefox 53.0

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.

Comment 1

[Friedrich Strohmaier]

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.

Comment 2

[Alice0775 White]

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

Comment 3

[Ray Lin[:ralin]]

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

Comment 4

[Ray Lin[:ralin]]

Attachment: audio-size-comparison.png

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.

Comment 5

[Ray Lin[:ralin]]

Attachment: test page.html

Comment 6

[Ray Lin[:ralin]]

Hi Jared,

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

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 8

[Nathan Froyd [:froydnj]]

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

Comment 9

[Ray Lin[:ralin]]

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.

Comment 10

[Ray Lin[:ralin]]

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

Comment 11

[Ray Lin[:ralin]]

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

Comment 12

[Ray Lin[:ralin]]

Attachment: 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.

Review commit: https://reviewboard.mozilla.org/r/144512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144512/

Comment 13

[Ray Lin[:ralin]]

Hi Gijs,

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

Comment 14

[:Gijs (he/him)]

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?

Comment 15

[Ray Lin[:ralin]]

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

[Ray Lin[:ralin]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144512/diff/1-2/

Comment 17

[Ray Lin[:ralin]]

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.

Comment 18

[Ray Lin[:ralin]]

Attachment: audio-only-video layout comparison.png

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

Comment 19

[Loic]

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

Comment 20

[Ray Lin[:ralin]]

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.

Comment 21

[Gerry Chang [:gchang]]

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