Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upMedia controls #23208
Media controls #23208
Conversation
highfive
commented
Apr 16, 2019
|
Heads up! This PR modifies the following files:
|
highfive
commented
Apr 16, 2019
|
|
|
Do you have an example available that I can use to diagnose the layout problems with the media controls? |
|
In general, try absolute positioning if you want something that is guaranteed to work right. Trying to lay out with inlines is annoying and error-prone. |
You should be able to see the media controls in action with this test running Servo with the Thanks in advance for your help! |
|
|
|
OK. This needs a rebase. |
ferjm
force-pushed the
ferjm:media.ui
branch
2 times, most recently
from
367c155
to
73dccc2
|
|
||
| // Servo only API to get an instance of the controls of a specific | ||
| // media element matching the given id. | ||
| fn ServoGetMediaControls(&self, id: DOMString) -> Fallible<DomRoot<ShadowRoot>> { |
This comment has been minimized.
This comment has been minimized.
Manishearth
Apr 30, 2019
Member
Is there a danger of this getting overwritten by client code? We may need something like XOWs to do this right?
(Not that we have to do that in this PR but we should be careful when shipping)
This comment has been minimized.
This comment has been minimized.
ferjm
May 13, 2019
Author
Member
I guess client code code could overwrite this. But I cannot think of a way to exploit that in a dangerous way. In any case, I agree that we may need something like XOWs to do this right.
|
|
The best way forward might be to trace through building the flow tree for the two cases (part of the shadow DOM and not) and finding where the behaviour differs? |
|
Video is replaced content, so you should not be trying to hack layout to render anything underneath Instead you should use the shadow DOM to replace |
|
Thanks for looking into this @pcwalton
Yes, I understand that. The temporary hack is only there to allow me to see the controls while coding them.
Indeed. FWIW in Firefox the DOM looks like this for a
The I don't understand Gecko's layout at all (even less than Servo's layout), but it may seem that Gecko is treating
Sorry, I am not sure I understand what you propose here. Would this replacement happen in the actual DOM tree? The sole point of internally wrapping the controls in a closed shadow tree is to encapsulate and hide them from content. Also, I believe wrapping the <div id="author-video-container">
<video></video>
</div>const video = document.getElementById("author-video-container").firstChild;
// Here video would be `<div id=video-container>` instead of `<video>` as the author would expect. |
|
So the Gecko setup here is that But I'm not sure how that's quite relevant to this. Servo doesn't have the concept of "leaf" / "not-leaf", and the checks @ferjm is adding to the flow constructor are just special-casing |
FWIW something like works fine, the content of the While something like: Does not work. The So I suspect the problem is not Shadow DOM, but the layout of media elements. |
|
I think you may be talking past each other a bit... So what @ferjm is doing looks right (as in, correct modulo the pseudo-element bit, albeit hacky) to me, since children() does the right thing wrt Shadow DOM. The issue with the video controls may be that |
|
That being said I may be wrong too, does this patch make the controls have the right layout tree if |
|
I am afraid there is no change in the layout tree if A test like <html lang="en">
<head>
<meta name="viewport" content="width=device-width, initial-scale=1">
<title>Video test</title>
<style>
video {
display: block;
}
</style>
</head>
<body>
<video src="https://addpipe.com/sample_vid/short.mp4" poster="https://addpipe.com/sample_vid/poster.png"></video>
<script>
setTimeout(() => {
document.querySelector("video").setAttribute("controls", "");
});
</script>
</body>
</html>Produces a DOM tree like: And a flow tree like: |
|
Seems to me that this issue is clearly shadow DOM specific, because if you render the controls without using shadow DOM (by pasting them directly into the HTML) then they are laid out properly below the video. So it has something to do with the implementation of shadow DOM in Servo, which I know nothing about. |
|
One random hypothesis: Shadow DOM could be failing to invalidate parts of the DOM properly. |
Are you pasting them as children of the |
|
The flow/fragment distinction means that Servo layout just doesn't have the concept of a replaced content box that can contain other elements (that are rendered). The spec says that replaced content can't contain visible elements, and Servo layout is designed around this. The only way it could possibly work is to treat video as a "background" of a Generic fragment, like for CSS image backgrounds. But this would be a horrible hack. |
|
@bors-servo r=emilio,jdm |
|
|
Media controls <strike>This is still highly WIP. It depends on #22743 and so it is based on top of it. The basic controls functionality is there, but the layout is highly broken. There is a hack to at least make the controls render on top of the video, but it is not correctly positioned. The controls' div container ends up as sibbling of the media element in the flow tree while IIUC it should end up as a child.</strike> - [X] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [X] These changes fix #22721 and fix #22720 There is at least an extra dependency to improve the functionality and visual aspect: #22728. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23208) <!-- Reviewable:end -->
|
|
|
@bors-servo retry |
Media controls <strike>This is still highly WIP. It depends on #22743 and so it is based on top of it. The basic controls functionality is there, but the layout is highly broken. There is a hack to at least make the controls render on top of the video, but it is not correctly positioned. The controls' div container ends up as sibbling of the media element in the flow tree while IIUC it should end up as a child.</strike> - [X] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [X] These changes fix #22721 and fix #22720 There is at least an extra dependency to improve the functionality and visual aspect: #22728. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23208) <!-- Reviewable:end -->
|
|
ferjm commentedApr 16, 2019
•
edited by atouchet
This is still highly WIP. It depends on #22743 and so it is based on top of it.The basic controls functionality is there, but the layout is highly broken. There is a hack to at least make the controls render on top of the video, but it is not correctly positioned. The controls' div container ends up as sibbling of the media element in the flow tree while IIUC it should end up as a child../mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThere is at least an extra dependency to improve the functionality and visual aspect: #22728.
This change is