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 upImplement HTMLMediaElement muted and defaultMuted attributes #22347
Conversation
highfive
commented
Dec 1, 2018
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon. |
highfive
commented
Dec 1, 2018
|
Heads up! This PR modifies the following files:
|
highfive
commented
Dec 1, 2018
|
Does not build! Will fix soon |
|
Thanks for working on this! I realize that you might already fixed this, but I added a couple of comments of things that may be causing the build issues that you are seeing. |
| @@ -1263,6 +1266,11 @@ impl HTMLMediaElementMethods for HTMLMediaElement { | |||
| // https://html.spec.whatwg.org/multipage/#dom-media-autoplay | |||
| make_bool_setter!(SetAutoplay, "autoplay"); | |||
|
|
|||
| // https://html.spec.whatwg.org/multipage/#dom-media-defaultmuted | |||
| make_bool_getter!(DefaultMuted, "muted"); | |||
This comment has been minimized.
This comment has been minimized.
ferjm
Dec 4, 2018
•
Member
The make_bool_getter! and make_bool_setter! macros implement the getter and setter of HTML attributes. (You can see its implementation here). In this case, defaultMuted is not a HTML attribute of the <video> or <audio> elements, but a property of the HTMLMediaElementJS object, so we don't need to use these macros here. We have to implement the getter and setter manually, just like you did with muted.
This comment has been minimized.
This comment has been minimized.
ferjm
Dec 4, 2018
•
Member
Actually, I was wrong about this :\ muted is indeed an attribute of the html tag. Add servo/html5ever#358 is what needed to be done. Sorry about the temporary confussion.
|
@stevesweetney how are things going with this PR? Do you need any further help to progress? |
|
Hey @ferjm ,
to the Cargo.toml at the root of this repo. |
I think this should do the trick if you use double |
|
Yup that worked. Just needed to run After updating there are some compatibility issues and changes in the api going from from markup5ever 0.7.1 to markup5ever 0.7.5 I'm not sure where to go from here |
|
@stevesweetney What were the API changes that you encountered? |
|
This what appears when using the html5ever patch above and attempting to build:
|
|
I'm wondering if I should remove the patch take @ferjm suggestion to use the implement the getter and setter manually for the muted attribute just for now and leave a todo to eventually use the make_bool_getter! and make_bool_setter! macros? |
|
I think that's worth a try to allow you to make progress. |
|
I don't know if this helps, but #22563 is updating markup5ever to 0.7.5. |
|
@ferjm You're right that PR did help |
|
Good start! We still need to make a few changes before merging. I added some comments inline. Make sure that you run all the tests from Thanks! |
| } | ||
|
|
||
| // https://html.spec.whatwg.org/multipage/#dom-media-muted | ||
| fn SetMuted(&self, value: bool) { |
This comment has been minimized.
This comment has been minimized.
|
|
||
| // https://html.spec.whatwg.org/multipage/#dom-media-muted | ||
| fn SetMuted(&self, value: bool) { | ||
| self.muted.set(value); |
This comment has been minimized.
This comment has been minimized.
ferjm
Jan 14, 2019
Member
Apart from setting the value of the mute property, we need to set the same value in the media backend. To do that, we need to expose a new method in the servo-media Player trait (i.e. set_mute) which implementation should use the GStreamer set_mute method.
| window | ||
| .task_manager() | ||
| .media_element_task_source() | ||
| .queue_simple_event(self.upcast(), atom!("volumechange"), &window); |
This comment has been minimized.
This comment has been minimized.
ferjm
Jan 14, 2019
Member
We need to implement the following piece from the spec here:
Then, if the media element is not allowed to play, the user agent must run the internal pause steps for the media element
This comment has been minimized.
This comment has been minimized.
ferjm
Jan 14, 2019
Member
This is also missing from the SetVolume method. It would be really nice if you could also add the check there.
|
@stevesweetney how are things going here? Let me know if you need any help with the remaining work, please. |
|
@ferjm Hey, I'm still continuing work on this. Could you clarify this part for me
How do I check to see if the media element is allowed to play? |
|
Hmm... the spec does not define any specific constraints why an element shouldn't be allowed to play and it leaves that decision to the UA. Let's always allow playback for now, but let's create a placeholder function for that: This is out of the scope of this issues, but it would be great if you could also address this comment as well. |
|
We need to update the expectations for
|
|
@stevesweetney: |
1 similar comment
|
@stevesweetney: |
|
@bors-servo try=wpt |
Implement HTMLMediaElement muted and defaultMuted attributes <!-- Please describe your changes on the following line: --> PR for issue #22291 --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #22291 (github issue number if applicable). - [X] There are tests for these changes OR <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/22347) <!-- Reviewable:end -->
|
|
|
I filed #22935 as a follow-up. @stevesweetney Thanks for your work here! @bors-servo r+ |
|
|
Implement HTMLMediaElement muted and defaultMuted attributes <!-- Please describe your changes on the following line: --> PR for issue #22291 --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #22291 (github issue number if applicable). - [X] There are tests for these changes OR <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/22347) <!-- Reviewable:end -->
|
|
|
@bors-servo retry |
Implement HTMLMediaElement muted and defaultMuted attributes <!-- Please describe your changes on the following line: --> PR for issue #22291 --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #22291 (github issue number if applicable). - [X] There are tests for these changes OR <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/22347) <!-- Reviewable:end -->
|
|
stevesweetney commentedDec 1, 2018
•
edited
PR for issue #22291
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is