Bug 88105

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

Created attachment 161455 [details] Patch

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?

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

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.

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

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

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

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

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!

<rdar://problem/97694222>

Committed 255528@main (13c796fb7f1d): <https://commits.webkit.org/255528@main> Reviewed commits have been landed. Closing PR #5304 and removing active labels.