1367868 - audio element collapses to 0 size if given bogus audio URL, with "too much recursion videocontrols.xml:1185:13"

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:
 1. Load attached testcase.

EXPECTED RESULTS:
 Visible audio controls (nonzero size)

ACTUAL RESULTS:
 Audio element collapses to 0 size.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] (reduced availability - travel & post-PTO backlog) from comment #0)
> ACTUAL RESULTS:
>  Audio element collapses to 0 size.

Actually, in current Nightly:
 - The audio controls show up at first.
 - Then they fade out.
 - Then the element collapses to 0 size (as shown by its border collapsing to a single point).


Additionally, after the bogus mp3 load fails, I see the following JS Error in my browser console:
* too much recursion    videocontrols.xml:1185:13
https://dxr.mozilla.org/mozilla-central/rev/f81bcc23d37d7bec48f08b19a9327e93c54d37b5/toolkit/content/widgets/videocontrols.xml#1185

* too much recursion    videocontrols.xml:1485:9
https://dxr.mozilla.org/mozilla-central/rev/f81bcc23d37d7bec48f08b19a9327e93c54d37b5/toolkit/content/widgets/videocontrols.xml#1485

Comment 2

[Ray Lin[:ralin]]

It looks like those problem including "too much recursion" are derived from the patch in bug 1339269. I'll address this issue in bug 1362146, and add all these test cases afterwards. Thanks.

Comment 4

[Ray Lin[:ralin]]

Hi Daniel,


> Actually, in current Nightly:
>  - The audio controls show up at first.
>  - Then they fade out.
>  - Then the element collapses to 0 size (as shown by its border collapsing to a single point).

I just found that in 52, we do collapses controls as well since this case fulfill the condition[0]: "media has never been played && got error". 
> Additionally, after the bogus mp3 load fails, I see the following JS Error in my browser console:
> * too much recursion    videocontrols.xml:1185:13
So, I think we just need to break the infinite loop caused by reflow handler here. 

I'll file another bug to discuss whether we should hide controls when encounter error especially for <audio> as there's no extra information like in <video> telling users what going on and why controls dismisses immediately.


[0]: https://hg.mozilla.org/mozilla-central/file/62c5218b7325/toolkit/content/widgets/videocontrols.xml#l629

Comment 5

[Jean-Yves Avenard [:jya]]

With audio, the issue is that you can't even tell an audio element was there to start with.

Comment 6

[Ray Lin[:ralin]]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> With audio, the issue is that you can't even tell an audio element was there
> to start with.

Yeah, because the audio controls hide automatically as an error occurred prior to it's able to be started, and that's what we've been doing for a long time. The janky caused by "too much recursion" seems more urgent to me, so I'd prefer to split off the visibility issue to another bug for more specific discussion.

Comment 7

[Alexandre Folle de Menezes]

Well, I opened a bug about the visibility (#1368053) that was marked as a duplicate of this one...

Comment 8

[Ray Lin[:ralin]]

Reopened it. My fault didn't think it through at the first time :P

Comment 9

[Ray Lin[:ralin]]

Attachment: Bug 1367868 - Don't hide media controls when error occurred for audio even if it hasn't been played yet.

Unlike video element, audio has no status overlay to inform users if an
error occurred. Instead of hiding entire media controls, we should keep
it visible in order not confuse users, and see if we can come up with a
better approach such as making the buttons disabled afterwards.

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

Comment 10

[Ray Lin[:ralin]]

> Additionally, after the bogus mp3 load fails, I see the following JS Error
> in my browser console:
> * too much recursion    videocontrols.xml:1185:13
> https://dxr.mozilla.org/mozilla-central/rev/
> f81bcc23d37d7bec48f08b19a9327e93c54d37b5/toolkit/content/widgets/
> videocontrols.xml#1185
> 
> * too much recursion    videocontrols.xml:1485:9
> https://dxr.mozilla.org/mozilla-central/rev/
> f81bcc23d37d7bec48f08b19a9327e93c54d37b5/toolkit/content/widgets/
> videocontrols.xml#1485

Bug 1373537 has addressed this issue. We no longer rely on calculation of clientWidth before adjustment, so that adjustment won't trigger another synchronous reflow and cause the loop.

Comment 11

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

Comment on attachment 8883182 [details]
Bug 1367868 - Don't hide media controls when error occurred for audio even if it hasn't been played yet.

https://reviewboard.mozilla.org/r/154110/#review159626

Comment 12

[Ray Lin[:ralin]]

Thanks for the review :D

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1ed3af55efb
Don't hide media controls when error occurred for audio even if it hasn't been played yet. r=jaws

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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