Bug 88105 – "display" property value of computed style for audio element without controls is "inline" (not "none")

• Component: Media
• Status: RESOLVED FIXED

Copied from http://code.google.com/p/chromium/issues/detail?id=130717


What steps will reproduce the problem?
1. Open attached AudioStyleIssue.html in Web browser.
2. Click "Show information" button.
3. You will see 2 alert boxes with information. First contains information about <audio> element with controls, second contains info about <audio> element without controls.

What is the expected result?

[first alert]
Audio element:
id = audioWithControls
has controls = true
display (CSS value) = inline
visibility (CSS value) = visible

[second alert]
Audio element:
id = audioWithoutControls
has controls = false
display (CSS value) = none
visibility (CSS value) = visible

What happens instead?

[first alert]
Audio element:
id = audioWithControls
has controls = true
display (CSS value) = inline
visibility (CSS value) = visible

[second alert]
Audio element:
id = audioWithoutControls
has controls = false
display (CSS value) = inline
visibility (CSS value) = visible


IE and FF: display property of computed style for audio element without controls is "none".

Chrome: display property of computed style for audio element without controls is "inline".

So, this behaviour is different in major browsers.

Test on different browsers/platforms.
Mac 10.7, gc: 19.0.1084.52 and 21.0.1158.0 canary - able to reproduce the issue 
Mac 10.7, safari - able to reproduce the issue 
Win7, 19.0.1084.53 - able to reproduce the issue 
Win7, FF - unable to reproduce the issue

[Silvia Pfeiffer]

Created attachment 161455 [details]
Patch

[Eric Carlson]

Comment on attachment 161455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161455&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:4118
> +    if (!isVideo()) {
> +        if (controls())
> +            removeInlineStyleProperty(CSSPropertyDisplay);
> +        else
> +            setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone);
> +    }

This will change user-applied 'display' style. I don't know if there is any other way to do this, but is it OK?

[Silvia Pfeiffer]

(In reply to comment #3)
> (From update of attachment 161455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161455&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4118
> > +    if (!isVideo()) {
> > +        if (controls())
> > +            removeInlineStyleProperty(CSSPropertyDisplay);
> > +        else
> > +            setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone);
> > +    }
> 
> This will change user-applied 'display' style. I don't know if there is any other way to do this, but is it OK?

Can we add someone else to review this who might know a better way?

[Dimitri Glazkov (Google)]

Comment on attachment 161455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161455&action=review

>>> Source/WebCore/html/HTMLMediaElement.cpp:4118
>>> +    }
>> 
>> This will change user-applied 'display' style. I don't know if there is any other way to do this, but is it OK?
> 
> Can we add someone else to review this who might know a better way?

We already do this in HTMLMediaElement::rendererIsNeeded. What's not happening is triggering the re-evaluation of rendererIsNeeded condition when controls() value changes. This in turn means that we need to re-attach the HTMLAudioElement whenever we call configureMediaControls.

[Silvia Pfeiffer]

(In reply to comment #5)

I am confused by what you are suggesting. Could you help me understand?


> We already do this in HTMLMediaElement::rendererIsNeeded.

I can't seem to understand what you mean by "this". Are you saying that audio elements without @controls already test whether a renderer is required, and have display:none without a @controls attribute? My tests don't confirm this. All audio elements on a page, with or without @controls, current are display:inline.


> What's not happening is triggering the re-evaluation of rendererIsNeeded condition when controls() value changes.

My tests show that we have the wrong display value even without JavaScript. A simple
<audio></audio> has the wrong display value.

When we create just an object like this:
a = new Audio();
then this object has a display=""
no matter whether it has or has no @controls attribute.


> This in turn means that we need to re-attach the HTMLAudioElement whenever we call configureMediaControls.

Are you suggesting we destroy the shadow subtree for audio elements that have no @controls attribute and with that set to display:"" ? Then recreate the shadow subtree when the HTMLAudioElement get a @controls attribute again?

[Dimitri Glazkov (Google)]

(In reply to comment #6)
> (In reply to comment #5)
> 
> I am confused by what you are suggesting. Could you help me understand?

What I did here is suggest a solution to a behavior bug without looking at the actual problem that you're trying to solve :)

So first, I'll demonstrate the behavior bug, and then give you the answer to the actual problem at hand :P

Here's a simple test case: http://jsfiddle.net/xdGtZ/

The key thing to watch for is the red background color of the <div> that encloses the <audio>.

When we start, there's no 'controls' attribute on the <audio>. As you can see, there's no red in the results pane, telling us that the height of the <div> that encloses the <audio> is 0. This happens because the <audio> itself did not generate a RenderObject (which would in turn inform the intrinsic height of <div>. The HTMLMediaElement::rendererIsNeeded check made sure of this.

Now, click the "Add Controls" button. Immediately, the <audio> inflates in height (and width), and you see the red. The RenderObject for <audio> now exists.

Now, click the "Remove Controls" button. Technically, we should now go back to no RenderObject. However, we can still clearly see the Red, which means the <audio> is still being rendered.

That's a bug. Anytime the "controls" attribute is added or removed, we must ensure that the styles are recalculated and correct decisions about rendering the box or not are made.

Further, this still does not solve the reported problem. Since we're not actually changing the styles (we are manipulating directly on RenderObjects), the computed display value stays the same.

Luckily, we just happened to have the right technology in our back pocket. It's called CSS -- specifically the UA stylesheet and the :not pseudoclass. Just add these lines to mediaControls.css:

audio:not([controls]) {
    display: none;
}

And voila! Everything works just as prescribed. As a bonus, you can now remove all that cruft in HTMLMediaElement::rendererIsNeeded. It's no longer necessary. Hurray!

[Eric Carlson]

(In reply to comment #7)
> Luckily, we just happened to have the right technology in our back pocket. It's called CSS -- specifically the UA stylesheet and the :not pseudoclass. Just add these lines to mediaControls.css:
> 
> audio:not([controls]) {
>     display: none;
> }
> 
> And voila! Everything works just as prescribed. As a bonus, you can now remove all that cruft in HTMLMediaElement::rendererIsNeeded. It's no longer necessary. Hurray!

Bravo!

[Silvia Pfeiffer]

Oh, such an elegant solution - why didn't I see it! Thanks. :-)

[Ahmad Saleem]

Updated results from test case present within Chrome bug and changed to JSfiddle:

Link - https://jsfiddle.net/c8btuq0L/show
___

*** Safari 15.6 on macOS 12.5:

[First Alert]
Audio element:
id = audioWithControls
has controls = true
display (CSS value) = inline
visibility (CSS value) = visible

[Second Alert]
Audio element:
id = audioWithoutControls
has controls = false
display (CSS value) = inline <<<<<<<<<<----------------- BUG
visibility (CSS value) = visible

*** Firefox Nightly 105:

[First Alert]
Audio element:
id = audioWithControls
has controls = true
display (CSS value) = inline
visibility (CSS value) = visible

[Second Alert]
Audio element:
id = audioWithoutControls
has controls = false
display (CSS value) = none
visibility (CSS value) = visible

*** Chrome Canary 106:

[First Alert]
Audio element:
id = audioWithControls
has controls = true
display (CSS value) = inline
visibility (CSS value) = visible

[Second Alert]
Audio element:
id = audioWithoutControls
has controls = false
display (CSS value) = none
visibility (CSS value) = visible

__________

Plus the bug also exist in the test case from Comment 07 - where adding control make 'control' with "Red background" but removing will keep "Red" background.

________

I just wanted to share updated results to confirm that this bug still exist in Safari 15.6 on macOS 12.5. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/97694222>

[EWS]

Committed 255528@main (13c796fb7f1d): <https://commits.webkit.org/255528@main>

Reviewed commits have been landed. Closing PR #5304 and removing active labels.