1412210 - Click to play button should hide when error occurred

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect, P3)

Description

[Ray Lin[:ralin]]

Visit https://bug1376004.bmoattachments.org/attachment.cgi?id=8885585, the click to play button should not show up.

It is a regression of Bug 1328060.

Comment 1

[Ray Lin[:ralin]]

Attachment: Bug 1412210 - Respect video error state while adjusting controls to determine the visibility of clickToPlay button.

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

Comment 2

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

Comment on attachment 8922677 [details]
Bug 1412210 - Respect video error state while adjusting controls to determine the visibility of clickToPlay button.

https://reviewboard.mozilla.org/r/193810/#review199654

::: toolkit/content/widgets/videocontrols.xml:1737
(Diff revision 1)
>          const minVideoSideLength = Math.min(videoWidth, videoHeight);
>          const clickToPlayViewRatio = 0.15;
>          const clickToPlayScaledSize = Math.max(
>          this.clickToPlay.minWidth, minVideoSideLength * clickToPlayViewRatio);
>  
> -        if (clickToPlayScaledSize >= videoWidth ||
> +        if (this.hasError() || clickToPlayScaledSize >= videoWidth ||

It doesn't seem right to put this here because setting `this.clickToPlay.hideByAdjustment = true` isn't representative of why we are actually hiding it.

In other words, it doesn't make sense to show the clickToPlay button if the video was made larger in this case.

Comment 3

[Ray Lin[:ralin]]

Comment on attachment 8922677 [details]
Bug 1412210 - Respect video error state while adjusting controls to determine the visibility of clickToPlay button.

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

Comment 4

[Ray Lin[:ralin]]

Comment on attachment 8922677 [details]
Bug 1412210 - Respect video error state while adjusting controls to determine the visibility of clickToPlay button.

https://reviewboard.mozilla.org/r/193810/#review199654

> It doesn't seem right to put this here because setting `this.clickToPlay.hideByAdjustment = true` isn't representative of why we are actually hiding it.
> 
> In other words, it doesn't make sense to show the clickToPlay button if the video was made larger in this case.

Thanks, I agree. adjustControlSize cares only about size changing, so if the clickToPlay button was hidden by other reasons, e.g. error or video played, which menas .hidden is true but .hideByAdjustment is false, we just leave it as-is.

Comment 5

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

Comment on attachment 8922677 [details]
Bug 1412210 - Respect video error state while adjusting controls to determine the visibility of clickToPlay button.

https://reviewboard.mozilla.org/r/193810/#review201532

Thanks, this is better.

Comment 6

[Ray Lin[:ralin]]

Comment on attachment 8922677 [details]
Bug 1412210 - Respect video error state while adjusting controls to determine the visibility of clickToPlay button.

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

Comment 7

[Ray Lin[:ralin]]

Thank you Jared :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8164446177a
Respect video error state while adjusting controls to determine the visibility of clickToPlay button. r=jaws

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f8164446177a