Bug 163592

Summary: Shadow hosts aren't automatically containing blocks for positioned descendants
Product: WebKit Reporter: Antoine Quint <graouts>
Component: Layout and RenderingAssignee: Antoine Quint <graouts>
Status: RESOLVED WONTFIX    
Severity: Normal CC: ap, graouts, koivisto, rniwa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 10   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Testcase
none
Patch none

Description Antoine Quint 2016-10-18 01:48:41 PDT
As of http://trac.webkit.org/changeset/207436, turning on the Development > Experimental Features > Modern Media Controls flag will use a different codebase for media controls. While it's limited in terms of feature right now (more patches are going to land throughout the week), it has the basic functionality of showing a play button over a <video> element on macOS and allowing for that button to be clicked to show a whole set of controls laid out in a controls bar at the bottom of the video as it starts playing. Right now, the initial play displays correctly, but once clicked the controls bar doesn't appear. Showing the Web Inspector makes the controls bar render correctly.
Comment 1 Radar WebKit Bug Importer 2016-10-18 01:49:32 PDT
<rdar://problem/28820129>
Comment 2 Antoine Quint 2016-10-18 02:19:30 PDT
Created attachment 291933 [details]
Testcase
Comment 3 Antti Koivisto 2016-10-18 08:06:23 PDT
--- Source/WebCore/Modules/modern-media-controls/controls/media-controls.css	(revision 207457)
+++ Source/WebCore/Modules/modern-media-controls/controls/media-controls.css	(working copy)
@@ -23,7 +23,9 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-.media-controls,
+.media-controls {
+    position: relative;
+}
 .media-controls > .controls-bar {
     position: absolute;

makes them show up
Comment 4 Antoine Quint 2016-10-18 09:34:15 PDT
Created attachment 291950 [details]
Patch
Comment 5 Alexey Proskuryakov 2016-10-18 09:52:21 PDT
Comment on attachment 291950 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        instead, which removes the rendering issues.

What bug tracks fixing the root cause?
Comment 6 Antoine Quint 2016-10-18 10:04:10 PDT
(In reply to comment #5)
> Comment on attachment 291950 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291950&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        instead, which removes the rendering issues.
> 
> What bug tracks fixing the root cause?

I suppose this should be kept as the bug tracking the original issue. I doubt it'll get fixed once we have a workaround though.
Comment 7 Antti Koivisto 2016-10-18 10:30:00 PDT
Root cause appears to be in RenderImage (where RenderMedia inherits it). Since position:absolute makes little sense in UA shadow tree root element I don't think there is any particular desire to fix this.
Comment 8 Antti Koivisto 2016-10-18 10:35:33 PDT
Not that I'm stopping anyone.
Comment 9 Simon Fraser (smfr) 2016-10-18 10:41:51 PDT
(In reply to comment #7)
> Root cause appears to be in RenderImage (where RenderMedia inherits it).
> Since position:absolute makes little sense in UA shadow tree root element I
> don't think there is any particular desire to fix this.

What about RenderImage?
Comment 10 Antti Koivisto 2016-10-18 10:48:45 PDT
> What about RenderImage?

What about it?
Comment 11 Simon Fraser (smfr) 2016-10-18 10:50:27 PDT
You say "Root cause appears to be in RenderImage" but don't explain what it is.
Comment 12 Antti Koivisto 2016-10-18 10:54:30 PDT
The issue is about UA shadow trees of renderers derived from RenderImage using absolute positioning in their shadows.

This is what I said in a mail about this

"The problem appears to be that you are using absolute positioned container as the rendering root of your shadow tree. Absolute positioning inside shadow trees works the same as it does outside so your controls end up being relative to the body, not <video>. Not sure why it doesn’t show up at all, maybe something to do with the wackiness of RenderImage::layoutShadowControls."

and

"I’m not sure I would even call it a workaround. Using absolute positioning in the root of a shadow tree is strange since the host element is not going to be your containing block. It is very easy to get such shadow to render outside the host (for example left:0px top:0px will put it to the top left corner of the document)."
Comment 13 Alexey Proskuryakov 2016-10-18 11:39:15 PDT
Yes, it doesn't seem to make sense indeed. Can an assertion be added to catch trying to do this in UA shadow trees?
Comment 14 Simon Fraser (smfr) 2016-10-18 11:41:36 PDT
Should the spec say that shadow roots always have position: relative (or words about acting as containing block and/or stacking context)?
Comment 15 Antti Koivisto 2016-10-18 12:06:01 PDT
(In reply to comment #14)
> Should the spec say that shadow roots always have position: relative (or
> words about acting as containing block and/or stacking context)?

Shadow roots have no rendering at all. Children of shadow root act like direct children of the host. The host can be also be inline.

I'm not sure there is problem to solve as position:relative should generally give you what you want and position:absolute behavior is still consistent (bugs in special-renderer UA shadow trees notwithstanding).
Comment 16 Ryosuke Niwa 2016-10-18 13:40:01 PDT
(In reply to comment #14)
> Should the spec say that shadow roots always have position: relative (or
> words about acting as containing block and/or stacking context)?

There was a discussion to use display: block as the default styling for shadow roots:
https://github.com/w3c/webcomponents/issues/426
Comment 17 Ryosuke Niwa 2016-10-18 13:41:58 PDT
The current discussion hinges on adding a "lightweight" way to style custom elements: https://github.com/w3c/webcomponents/issues/468
Comment 18 Antti Koivisto 2016-10-18 14:59:47 PDT
> There was a discussion to use display: block as the default styling for
> shadow roots:

Shadow hosts. And this wouldn't be useful here as the host is a replaced inline element.
Comment 19 Ryosuke Niwa 2017-07-25 17:05:39 PDT
The current behavior matches the spec. We need to modify the host's style in order to do this, and nobody wanted attaching a shadow root to have that kind of side-effect in style.