Closed Bug 1412210 Opened 2 years ago Closed 2 years ago

Click to play button should hide when error occurred

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ralin, Assigned: ralin)

References

Details

Attachments

(1 file)

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 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.
Attachment #8922677 - Flags: review?(jaws) → review-
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 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.
Attachment #8922677 - Flags: review?(jaws) → review+
Thank you Jared :)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f8164446177a
Respect video error state while adjusting controls to determine the visibility of clickToPlay button. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f8164446177a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1415596
You need to log in before you can comment on or make changes to this bug.