1117885 - web component for video controls

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Components, defect)

Description

[Russ Nicoletti [:russn]]

Create web component from video app video controls. This component should include the 'videoBar' and 'videoControlBar' elements of the video app.

Comment 1

[Russ Nicoletti [:russn]]

Attachment: video controls web component repo

Hi David, attached is my video controls web component repo. 

The video app changes are here: https://github.com/russnicoletti/gaia/commit/9d66948cf0c8a30a3d818a3c68c9e10103f5ecd2.

Please let me know your feedback. Thanks.

Comment 2

[Russ Nicoletti [:russn]]

Comment on attachment 8546791 [details]
video controls web component repo

>https://github.com/russnicoletti/gaia/commit/475b99b5abf3284d75690632f13b1eeab8d121b6

Comment 3

[Russ Nicoletti [:russn]]

Please ignore comment 2. The latest video app changes are here:

https://github.com/russnicoletti/gaia/commit/475b99b5abf3284d75690632f13b1eeab8d121b6

Comment 4

[David Flanagan [:djf]]

I've started leaving feedback comments on github, but haven't gotten to the component itself yet.

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8546791 [details]
video controls web component repo

Russ: I gave feedback on the changes to the video app, then saw that that did not include the component itself. This link to the component is not a commit or pull request, so github doesn't allow me to add comments to it, which makes it hard to give feedback. It could be that this first batch of feedback is enough for you to continue working. If not, just give me a link to a commit so I can offer github comments and reset the feedback flag and I'll take another look.

I do have two high-level comments on the component itself.

I'm curious about the structure of the code where you have a VideoControls class inside the video controls component. Why did you decide to do it that way, rather than just putting everything directly in the component?

Also, I see that you're using a gaia-component library. That means that you're having to learn web components plus this gaia-component framework at the same time.  If it was me, I think I'd start trying to do a raw web component (maybe just in the apps/video/js/ directory. Then, when that is working well, consider moving it to the components repo and converting to use bower and gaia-component as needed.  On the other hand, if the gaia-component library is well-documented and you understand what it is doing and think it is helpful, I don't want to say you shouldn't use it.  (But you'll have to teach me what it is and what it does so that I can review your work!)

Comment 6

[Russ Nicoletti [:russn]]

Attachment: Link to github PR for video app changes

Comment 7

[Russ Nicoletti [:russn]]

Attachment: Link to github PR for media controls component

Comment 8

[Russ Nicoletti [:russn]]

I've modified the media controls component, removing the dependency on gaia-component and removing the functions from the API of the component -- it handles all relevant events internally.

Comment 9

[Russ Nicoletti [:russn]]

Wilson, David and I were discussing the subject of localization and web components. Is it the responsibility of the consumer of the component to own the locale files and to supply the localized strings to the component, as is done with gaia-header, or is there a way for the component to have its own locales files so that it can own the localizable content? I suppose a compromise would be to have a shared, component-specific locales directory/files -- containing the localizable strings owned by the component -- and for the  apps that used that component to load the component-specific locales files. Can you give us your guidance on this subject?

Comment 10

[Wilson Page [:wilsonpage]]

It isn't advised for component's to have their own locale files for two reasons:

- Localization team's workflow works out of the Gaia repo and this is apparently hard to change
- Localized strings depend on the usage context of the component. It is most likely too opinionated of the component to provide content strings.

Therefore the best solutions for localization of shared components are either:

A. Do nothing. If your component digestests user (light-dom) content, then it is up to the user to localize the content of these nodes.
B. Suck user strings into the shadow-dom. Gaia-header provides this functionality by allowing the user to provides a <span class="l10n-action"> that gets pulled inside the shadow-dom action-button [1].

[1] https://github.com/gaia-components/gaia-header#localization

Comment 11

[David Flanagan [:djf]]

Since there is not a github PR for me to provide feedback directly in github, I've read the files in https://github.com/russnicoletti/gaia-media-controls/tree/media-controls-component and will paste my comments below:

In the example code in index.html and the README file:

- can't you just use autoplay instead of the loadedmetadata event? Or
  have the video not autoplay to keep it simple?

- I don't know why you need a play-button-click event handler to make
  the video play and pause. Why can't the controls do this
  automatically?

- The fact that you need a setTimeout() seems like a "code smell". It
  seems to me that you ought to be able to tell the controls about the
  player at any point, without needing setTimeout().

- if you want the demo to work well on a device and on the web, you
  could use a @media query in the stylesheet and give the container a
  different size for large screens, if you want.

-----

package.json is completely out of date. I don't know what it is for,
though, so I don't know if you need to update it or could just delete
it.

-------

Demo: 

When I actually run the demo at
http://russnicoletti.github.io/media-controls/ on desktop, I get the
problem where I can click and drag on the slider, but then when I
release it, it remains hilighted and keeps following my mouse. I
suspect this means that you are not using a capturing handler for the
mouseup event.

-----

gaia-media-controls.js

line 4: You've got global variables in here. Earlier, when you had a build
process and a dist/ directory, I assume those were getting wrapped
somehow into a module. But if you're not going to be doing that, then
you need to put all of this into a (function() {}()); wrapper.

On the other hand, I see that line 304 uses an exports object that is
not defined, so maybe you are still doing some sort of build process
to wrap this up?

line 16: it took me a while to realize that setupShadowRoot() is not a
function you were defining but one inherited. 

line 18: the whole toCamelCase() function and ids.forEach()
stuff goes back to Dale Harvey's days on the video app. I've always
thought it was too clever.  If it was me, I'd just do:

var dom = {
  bufferedTime = shadowRoot.getElementById('bufferedTime'),
  durationText = shadowRoot.getElementById('duration-text'),
  etc.
};

Or maybe I'd define a $ utility function as a shortcut for
getElementById or for querySelector and use that.

I don't feel strongly about this, but just wanted to point out that
you don't have to pull that old code into this component if you don't
want to. You could also use this opportunity to rationalize the ids so
that they all use hyphens or all use camel case....

line 38: I've seen in your examples that initialize() is part of the
public API of the component. That makes me wonder what the
gaia-component superclass is doing with the created and template
properties. Are these exposed, too? Nothing you need to fix, but a
design choice I question, if so.

line 38: the method name initialize() implies one-time use. Do you
intend for this component to be re-useable? Could we call initialize()
once to have it control one video and then later call initialize()
again to control a different <video> element?  If so, then may name it
setPlayer() or setMediaElement() or something.  If not, then you
should probably have it throw an error if called more than once.
Finally, I wonder if you could just call the MediaControlsImpl()
constructor here in this initialize() method instead of in the created
callback. That way the constructor could do all the initialization and
you wouldn't need two separate steps in MediaControlsImpl.

line 38: another name for this method could be attach(), I
suppose. And it occurs to me that you probably need a detach() method
that deregisters all of the event handlers.

line 66: Footer elements make sense for the app. For this general
purpose component, I don't think they do make sense. I suspect it would
be better to change the footers to generic divs because footers carry
semantic meaning that might not be accurate in this more general case.

line 67: Ideally the background color is something that the shadow dom
elements can just inherit from the <gaia-media-controls>. If you
remove the background and foreground colors from the template, can you
set them on the container element instead? That would be the best
flexibility for the user of your component.

line 69: How does the absolute positioning work inside of a shadow
dom? Does this mean that the user of <gaia-media-controls> has to
specify position:relative or position:absolute on the component in
order to get it to lay itself out correctly? It would be better, I
think if you could do without position:absolute. This component might
be a good use for a flexbox layout.

line 228: add a semicolon and a comment explaining why this is
hardcoded.

line 297: do you need any aria-label attributes on these buttons for
accessibility? Or do those get added later in localization code? I
forget exactly what Eitan did to make the video app accessible.

line 309: note that you could just pass the shadow root to the
implementation class, and have that class find the elements it cares
about. No need for the outer class to look those elements up if they
are only going to be manipulated by the implementation class.

line 321: the constructor for this one controls element is assigning a
value to a global variable shared by all controls elements, which
seems wrong. Making this distinction based on the size of the slider
seems wrong, too, since a desktop client could be using the component
at small size.  I'm not convinced that any distinction needs to be
made, but if it does, it should probably be done based on screen size
not on component size.

line 326: again you're using a shared global variable to hold state
for one controls element, which means that you can't write an example
that uses two controls elements to control two different videos.  I
don't see why you can't use this.mediaPlayer everywhere instead of a
global. 

line 360,364: for mouse dragging events (but not for touch events) the
mousemove and mouseup events can be generated over elements different
than the element that the mousedown was sent to. This is why there is
the issue of not being able to let go of the slider when testing it
with a mouse, if you release it in the wrong place. So to handle
drags, you have to use a capturing event listener for the mousemove
and mouseup events.  I typically register capturing event handlers for
mousemove and mouseup on the window when I get a mousedown event, and
then remove both handlers when I get the mouseup event.

line 370: the fullscreen button is used for tablets only, I think. It
is fine with me if we leave it out. If it works, I'm fine leaving it
in, but if it causes  you any difficulty, let's just blow it off.

line 411: all of these functions for handling events are bound to the
impl object so they can use 'this'. But they are written like global
functions, which is weird. Please make them methods of
MediaControlsImpl.  Also, do you know that you can just define a
single handleEvent() method of MediaControlsImpl and register the
object itself as the event handler?  If you do that, then all events
go to handleEvent() and you don't need to mess with bind()'ing all the
handler methods.  The you can just use a switch(event.type) in
handleEvent to dispatch each event to the appropriate handler method,
or for simple events you can just handle them right there. This is a
technique I recommend for event-heavy code like this.

line 413: I'd say that handlePlayButton ought to actually do
this.mediaPlayer.play() or this.mediaPlayer.pause() and not emit an
event at all.

lines 420, 422: no need to compare the ids of these two elements when
you can just compare the elements themselves.

lines 436,441: I think these are for accessiblity. Please add a
comment explaining what they do.

lines 460,464: the fact that this code has to use call() is a clue
that these functions, and they functions they call should all be
methods of the same MediaControlsImpl object.

line 489: has this bug been fixed? Can we remove this awkward timer
workaround code?

line 561: it may not be necessary to worry about the touchID. If the
user interacts with the slider with two fingers, maybe we can just let
them do that. For my color picker component, it seemed fine to always
just use changeTouches[0].

line 709: I think that ForwardRewindController should probably just be
merged into the MediaControlsImpl class.  Again, it has the weird
pattern of having some methods, and then some related functions that
act like methods but aren't on the protype object and have to use
.call() to invoke each other. Maybe that was originally some kind of
pattern to keep implementation details private, but it doesn't seem
worth it, to me.

Comment 12

[David Flanagan [:djf]]

Comment on attachment 8555647 [details] [review]
Link to github PR for video app changes

Feedback above. Clearing the feedback request flag.

Comment 13

[Russ Nicoletti [:russn]]

Notes regarding how I've addressed the feedback from comment 11:

In the demo code, because the component now handles all events internally, the index.html file contains only markup and style, no script.

I fixed the slider issue in the demo by using a capturing event handler on the window.

There are no more global variables in gaia-media-controls.js; all the formerly global variables now belong to MediaControlsImpl.

I’m no longer using the array of object IDs to retrieve the elements in a loop. I’m now explicitly declaring the variables corresponding to the elements I’m using and explicitly fetching them from the shadow root.

There is no longer an ‘initialize’ API function on the component. I experimented with the component pulling in the video element — where the consumer specifies the video element as a descendent of the component, like:

<gaia-media-component …
  <video src=‘…’</video>
</gaia-media-component>

but I found the css issues that result to be insurmountable. Also, I don’t think the media controls component should own the video element. The alternative approach I took is to have the consumer of the component set an attribute on the component specifying the id of the player which the component uses to retrieve the player element. 

I changed the ‘footer’ elements in the component to ‘div’.

I found that the component does not inherit any style from the host so the component is still specifying the background color of it’s elements. 

The elements of the component now use the flex box layout; no more absolute positioning. As a result, the consumer — that is, the video app — is positioning the component.

Regarding the ‘ended’ event hack, the way I read the bug (783512), it’s unknown as to whether the problem still exists. As such, I have removed the hack from the component. I’m still getting the ‘ended’ event for all the videos I tested with (some from camera, some from the web).

My guess is the tablet is not a high priority platform for ffoxOS and I didn’t want to needlessly complicate the component so I’ve removed the ‘fullscreen’ button from the component.

I’ve merged the ForwardRewindConroller code into the MediaControlsImpl object.

Comment 14

[Russ Nicoletti [:russn]]

Attachment: Second feedback request

Comment 15

[David Flanagan [:djf]]

Comment on attachment 8575612 [details]
Second feedback request

Russ,

This is looking much better, so feedback+.

I still have a bunch of comments, primarily on three things:

1) how to handle the case when a gaia-media-controls element is created before it is attached to a media player element.

2) how your handleEvent() code is structured

3) your incomplete use of flexbox layout.

Since I'm not reviewing a pull request here there is nowhere to put my comment on github, so I'm just pasting them all here:

line 1: This is still not encapsulated inside of a function, so if
used as it stands, it will export multiple objects (including the Impl
class) into the global namespace.  I see the package.json file defines
a build step that uses browserify. But the bower.json file does not
refer to the processed file in dist/. I don't know how the build stuff
is supposed to work, but one way or another, you need to end up with a
file works standalone or with require.js, and only exports the
GaiaMediaControls() constructor without exposing the implementation
class.

line 20: what happens if this mediaPlayerId is not set yet? If the
user is creating a <gaia-media-controls> element with
document.createElement() or new GaiaMediaControls(), then it will get
created without any attributes, and there will be a time when it is
not linked to any player. Have you considered that, and allowed for
player changes?

line 34: if you need to make this an option, let's do it with an
attribute and property pair and let the app do the user agent
sniffing. I think we really don't want to do this in the component
itself. I'd default to fastSeek on: make it work well by default for
FirefoxOS, and add desktop support second.  Or maybe just use fastSeek
everywhere and file a bug to get better desktop support for it. For
now, our primary use case is making this great on mobile. If we need
to wait for desktop to catch up, that is okay.

line 39: this function is assuming that this.mediaPlayer is always
defined. If you're going to allow the media player to be swapped,
you'll probably want to break this function into two separate
functions so that you can add and remove the media player event
handlers separately from the controls handlers.  (And if you're not
going to allow the media player to ever change, you just need to
document that, and probably define an API other than HTML attribute
for setting the media player, because we assume that attributes can
always be changed.)

line 76: do you need to do a preventDefault() on context menu events
to actually prevent a context menu from being displayed?

Some general comments on your handleEvent() function:

 - consider using switch(e.type), at least to handle the easy cases

 - the most complicated stuff is the drag handling, and it is right in
   the middle. Consider moving that to the end of the function

 - if handleEvent() just dispatches to a bunch of othe
   handleFooEvent() functions then it isn't serving much purpose other
   than allowing you to avoid lots of .bind() calls.  If
   handleFooEvent() is just a few lines of code, then I'd recommend
   just moving those lines of code directly into handleEvent(). It
   then becomes the nerve center for your component: a single function
   that shows how everything fits together.

lines 147 and 151: I think you should just call this.seekVideo()
here. It doesn't make sense to me to have startFastSeeking() handle
this case since there will never be a corresponding call to
stopFastSeeking() in this case.  And note that these are examples of
things that could easily go directly into the handleEvent() function.

line 170: no need for the else return here.

lines 176, 181: if you use startFastSeeking only for long presses and
not clicks, then you probably don't need the isLongPressing flag
anymore.  (I'm not certain of that, but it seems likely to me)

line 206: update the reference to the forwardRewindController

line 224: another example of an unnecessary handle method. I'd remove
this method and also the playerEnded method and just put the
playerEnded() body directly into the handleEvent() method.

346: moveMediaPlayerPosition() seems like an awkward method
name. Maybe rename that method to seek() or something? Hard to keep it
distinct from seekVideo(), though. Not a big deal.

364: this was hard for me to understand. I think the condition would
be better expressed the other way:

   if (!this.isPausedWhileDragging && currentTime !== duration)

374: consider renaming this to formatTime() or something, since it is
also used for the current playback position not just the duration.

394: you can simplify this by only handling the long press case
here. Then you don't need the isLongPressing flag. It doesn't seem to
me like there is any need to define seekOnInterval as a separate
thing. You could just define it as a nested function in the call to
setInterval(). 

425: it would be cleaner if this stopFastSeeking() test case was
inside of the nested function in startFastSeeking()

423: what if you renamed seekVideo() to seekBy() and just passed the
interval: 10 or -10 or whatever. Then rename moveMediaPlayerPosition()
to seekTo().  seekBy could be the one with error checking and seekTo
just do what it does currently. Those might be cleaner, more logical
names.

466: so this is where you assume that media-player-id is set when the
element is first created.

I suspect the thing to do here is to not create the
MediaControlsImpl object until you know which video element will be
controlled.

If you're going to implement an API that allows the video element to
be changed, then the easiest thing to do when it changes might be to
just create a new MediaControlsImpl object. To do that, though, you'll
need to implement a destroy() or release() method on MediaControlsImpl
that unregisters all the event handlers so that it can be garbage
collected when you replace it with a new one.

So now there's just the issue of what API to use to indicate which
video element to control.  If we take the HTML <label> element as an
model, then using a for attribute (instead of media-player-id) is a
reasonable thing to do. The <label> also allows the nested children,
which would be nice here, too, but probably buggy in gecko. One
downside to 'for' as an attribute is it is a JS keyword, so we end up
with 'htmlFor' as the JS standard property for the HTML for attribute,
which is gross.

A simpler approach is to go back to using a method. Instead of calling
it initialize(), though, I'd call it attachTo(). So:

    mediaControlsElement.attachTo(playerElement);

In the simplest case, you'd only allow this method to be called once,
and you'd never let it change.  If you want to be more flexible, you
could implement a detachFrom() method so that the app could use a
single controls object with more than one video or audio elements.

Whatever you decide about the API, I'd guess that it is easiest to
not create the MediaControlsImpl until you know which player element
is being controlled.  (And destroy it and create a new one if the
player element changes.) Also, I think I'd modify the
MediaControlsImpl() constructor so that you pass the player element
directly to it.)

line 497: I had to look up flex-flow. I think flex-direction would be
the simpler thing to do here and below since you're not setting any
wrapping properties.

line 498: I would have expected align-items: stretch so that both rows
contained in this column are automatically forced to be as wide as the
container.

505: for this row, setting justify-content:center is fine, but
probably unnecessary, if you use any flex settings on the items in the
row, then those items will grow to fill the row and the justify value
doesn't make any difference. But you might want to set
align-items here to specify the vertical alignment of the elements in
this row. Maybe you're just using whatever the default is, and that
seems reasonable.

509: I was surprised to see this z-index property. Why is it needed
here? In my experience z-index properties always need a comment to say
what element you're want to be on top of.  In this case, if you're
just making sure that the controls are on top of the video, I think
this is a property that the app should set, not something the
component should handle.

510: with flex box layout, I don't think you need width specifications
like this. I assume that the preferred way to do this is to use
align-items:stretch in the parent column.  (Or override a different align-item
setting in the parent by setting align-self:stretch here) (I used
http://demo.agektmr.com/flexbox/ to play around with this)

line 513: Let's not land it with this special case in it. Maybe this
is outdated anyway. The container does not have a width set in the
normal case, only for this desktop case. Seems strange.

525: I think flex-grow: 0 is the default, so you don't need to set
it. flex-shrink has a defualt of 1, which you probably shouldn't
override since it will give slightly better behavior when there isn't
enough room to display everything.

538: I think you could use set flex or flex-basis here instead of
width. That's probably the preferred way to do it.  Also, it is
probably worth including * {box-sizing:border-box} in the
stylesheet. Then the padding values become part of width and the width
would be 6.8rem instead of 3.8.

542: I thought you only needed to set the order if you wanted to
display elements in a different order then they appear in the layout.
I think you can probably remove all of your order: attributes

549: If you make this element flexible, then you don't need to use a
calc() on the width property. calc() has always struck me as a hack,
and shouldn't be needed in flex boxes.  Use something like:

  flex-grow: 1;
  flex-basis: 10rem;

Instead of 10rem, use whatever number seems like the minimum viable
width for this element. By setting flex-grow on it, the element will
stretch to take up all available room.

550: I don't think you should need to set the height of this
element. Instead, I'd set the height of the row (probably with
flex-basis, not height) and then set the height of the items in the
row with align-items: stretch. Or maybe use align-items:center so that
the text is centered and override it here with align-self:stretch

557 and 562: these two blocks of styles refer to exactly the same
elements. I'd combine them both using this selector:

      #slider-wraper > div.progress

570: I don't think you really need the z-index attributes here. Just
change the order of the HTML so that the background element comes
first and elapsed-time comes after it and they should layer correctly.

573: we've never used the buffered-time element, and since there is no
JS to every display it, I'd say take it out of the CSS and HTML. And
note that if you do that, then it may not longer make sense to set
width and height in the .progress class since there will be only two
elements.

581: I think this is the margin-top that you were telling me about. I
suspect the issue here is that both bars have top:50% when to get them
perfectly centered on the same spot, you'd need to use
calc(50%-0.05rem) for the background and calc(50%-0.15rem) for the
elapsed time.

609: can't these :after properties go directly in the playhead? This
kind of CSS always confuses me, so maybe I'm missing something.

634: use flex-direction

635: since the children are flexible, this has no effect. Maybe use
align-items: stretch or align-items:center, though?

636: wasn't the opacity of the background above 0.85? Is it
intentionally different here?

637: Probably better to specify the height with flex-basis here,
though I'm not sure it makes any difference.

638: you shouldn't need to specify the width if you used
align-items:stretch in the column container.

639: how does setting font-size here interact with the icon font used
to display the buttons? Is it normal to have to do this? I'm actually
surpized that this doesn't make the icons tiny.  Can you remove it?

643: add a comment saying why you're overriding the direction here.

644: I think z-index should probably be set by the app, not here.

651,652: I think just flex:1 will work here.

656-658: these look leftover from when we were using images instead of
the icon font. I'd guess you can just remove them. Or I really don't
understand how icons fonts work.

661-674: you shouldn't need to specify order, and it is wrong to
specify the width, since you make these three items equally flexible,
they will be equally spread out.

Line 707: the HTML uses a lot of id attributes. I'm guessing that this
works correctly in shadow DOM, but I'm not completely sure. What do
other components do? Do they use ids too, or do they use classes
instead? I'd just follow Wilson's lead here. If necessary, change
things like id="video-control-bar" to
class="gaia-media-controls-buttons" shadowRoot.querySelector('.foo')
instead of shadowRoot.getElementById('foo').

Comment 16

[Wilson Page [:wilsonpage]]

It looks like you should be using <gaia-slider> [1] internally if possible for OS consistency. It hasn't been used in production yet, so may well need some refinement. 

[1] https://github.com/gaia-components/gaia-slider

Comment 17

[Russ Nicoletti [:russn]]

Below are my responses to the feedback comments in comment 15. I have updated the 'media-controls-component' branch with the changes described below.

“505: for this row, setting justify-content:center is fine, but
probably unnecessary, if you use any flex settings on the items in the
row, then those items will grow to fill the row and the justify value
doesn't make any difference. But you might want to set
align-items here to specify the vertical alignment of the elements in
this row. Maybe you're just using whatever the default is, and that
seems reasonable.”

I’ve added “align-items: center”. I removed ‘justify-content’ and at the same time removed the explicit setting of width on ‘slider-wrapper’ instead letting it grow/shrink via ‘flex-grow/flex-shrink’

“509: I was surprised to see this z-index property. Why is it needed
here? In my experience z-index properties always need a comment to say
what element you're want to be on top of.  In this case, if you're
just making sure that the controls are on top of the video, I think
this is a property that the app should set, not something the
component should handle.”

That was leftover from the original video.css. I have removed it. In fact, just yesterday I noticed when dragging the play head, part of the blue background indicating the play head was being dragged appeared behind the video control bar (where the rewind/play/forward buttons are). The z-index property was causing that behavior and is now corrected :)

“510: with flex box layout, I don't think you need width specifications
like this. I assume that the preferred way to do this is to use
align-items:stretch in the parent column.  (Or override a different align-item
setting in the parent by setting align-self:stretch here) (I used
http://demo.agektmr.com/flexbox/ to play around with this)”

Yes, ‘align-items: stretch’ makes the ‘width’ attribute unnecessary; I’ve removed it.

"525: I think flex-grow: 0 is the default, so you don't need to set
it. flex-shrink has a defualt of 1, which you probably shouldn't
override since it will give slightly better behavior when there isn't
enough room to display everything."

I’ve removed ‘flex-shrink: 1’ because as you mentioned that is the default.

"538: I think you could use set flex or flex-basis here instead of
width. That's probably the preferred way to do it."

Done

"Also, it is probably worth including * {box-sizing:border-box} in the
stylesheet. Then the padding values become part of width and the width
would be 6.8rem instead of 3.8."

What’s the advantage of doing this? It seems like “6 of one, half a dozen of the other”…

"542: I thought you only needed to set the order if you wanted to
display elements in a different order then they appear in the layout.
I think you can probably remove all of your order: attributes"

Yes, I removed the ‘order’ attributes.

"flex-basis: 10rem;

Instead of 10rem, use whatever number seems like the minimum viable
width for this element. By setting flex-grow on it, the element will
stretch to take up all available room.”

I found that using ‘flex-basis’ doesn’t enforce a minimum width. When the element is flexible, it doesn’t seem to have an effect at all. Instead, I am using ‘min-width’ on the ‘media-controls-container’ element to enforce a minimum width of both of the rows.

"550: I don't think you should need to set the height of this
element. Instead, I'd set the height of the row (probably with
flex-basis, not height) and then set the height of the items in the
row with align-items: stretch. Or maybe use align-items:center so that
the text is centered and override it here with align-self:stretch"

I experimented with using ‘flex-basis/align-items’ instead of ‘height’; what I found when using ‘flex-basis/align-items: center’ is that ‘elapsed-text’  and duration-text’ get centered vertically but the time-slider elements do not. And furthermore, it’s not possible to center elements vertically using a percentage of the height (e.g., ‘top: 50%’) when ‘height’ is not specified. I had to use integers to specify the ‘top’ position, which I found unintuitive because the values that resulted in proper centering didn’t make sense to me (1.0 for ‘time-background’, 1.2 for ‘elapsed-time’). I like using ‘height’ because the method of centering the items vertically is more intuitive (can use ’top:50% - n). In the end, I’m using a hybrid approach: ‘align-items: center’ (to center ‘elapsed’ and ‘duration’ text) and ‘height’ so that I can intuitively center the slider wrapper elements.

"557 and 562: these two blocks of styles refer to exactly the same
elements. I'd combine them both using this selector:

      #slider-wraper > div.progress"

I combined the rules from the two blocks into the ‘.progress’ selector and removed ‘#slider-wrapper div’

"570: I don't think you really need the z-index attributes here. Just
change the order of the HTML so that the background element comes
first and elapsed-time comes after it and they should layer correctly."

I’m finding the z-index is necessary to ensure the elapsed time overlays the time background and the play head overlays the elapsed time.

"573: we've never used the buffered-time element, and since there is no
JS to every display it, I'd say take it out of the CSS and HTML. And
note that if you do that, then it may not longer make sense to set
width and height in the .progress class since there will be only two
elements."

I’ve made these changes.

"581: I think this is the margin-top that you were telling me about. I
suspect the issue here is that both bars have top:50% when to get them
perfectly centered on the same spot, you'd need to use
calc(50%-0.05rem) for the background and calc(50%-0.15rem) for the
elapsed time."

I’m using calc(50% - 0.1rem)’ and ‘calc(50% - 0.3rem)’ for ‘elapsed-time’ and ‘time-background’ respectively. I’ve added comments in the css explaining these values.

"609: can't these :after properties go directly in the playhead? This
kind of CSS always confuses me, so maybe I'm missing something."

I experimented with taking the attributes from ‘play-head:after’ and putting them in ‘play-head’. I found that ‘play-head:after’ and ‘play-head.active:before’ are critical to the affect of having the ‘active’ (large  blue) play head appearing behind the normal, smaller, white play head. When combining ‘play-head:after’ attributes into ‘play-head’, the large, blue ‘active’ play head covered the normal, white play head. I thought I could fix that by specifying a z-index of the “active” play head to be less than the normal play head but that didn’t have any affect. It seems using “after” and “before” has  an effect on the order of display such that “before” is drawn before the “after” (which seems intuitive — but I couldn’t find any documentation saying so) which gives us the proper layering. I have added comments to this affect to the css.

"636: wasn't the opacity of the background above 0.85? Is it
intentionally different here?"

It was that way in the video app. But I’m finding the opacity of the video control bar is not changed by this attribute. It is always fully opaque. I’ve removed the attribute.

"638: you shouldn't need to specify the width if you used
align-items:stretch in the column container."

You are correct. I’m using ‘align-items: stretch’ in the column container and I’ve removed the ‘width’ attribute from both rows.

"639: how does setting font-size here interact with the icon font used
to display the buttons? Is it normal to have to do this? I'm actually
surpized that this doesn't make the icons tiny.  Can you remove it?"

As with a lot of the css, this was from the video app. I’ve removed it; there was no noticeable affect.

"651,652: I think just flex:1 will work here."

I’m using ‘flex-grow: 1’, since as you pointed out earlier ‘flex-shrink’ defaults to ‘1’.

"656-658: these look leftover from when we were using images instead of
the icon font. I'd guess you can just remove them. Or I really don't
understand how icons fonts work."

I’ve removed these lines; they appear to have no purpose.

"661-674: you shouldn't need to specify order, and it is wrong to
specify the width, since you make these three items equally flexible,
they will be equally spread out."

Good catch, I’ve removed these lines.

"Line 707: the HTML uses a lot of id attributes. I'm guessing that this
works correctly in shadow DOM, but I'm not completely sure. What do
other components do? Do they use ids too, or do they use classes
instead? I'd just follow Wilson's lead here. If necessary, change
things like id="video-control-bar" to
class="gaia-media-controls-buttons" shadowRoot.querySelector('.foo')
instead of shadowRoot.getElementById('foo')."

Wilson is using classes so I have changed from ids to classes.

Comment 18

[Russ Nicoletti [:russn]]

There were some issues from comment 15 that I didn't address in comment 17:

"line 1: This is still not encapsulated inside of a function, so if
used as it stands, it will export multiple objects (including the Impl
class) into the global namespace.  I see the package.json file defines
a build step that uses browserify. But the bower.json file does not
refer to the processed file in dist/. I don't know how the build stuff
is supposed to work, but one way or another, you need to end up with a
file works standalone or with require.js, and only exports the
GaiaMediaControls() constructor without exposing the implementation
class."

I'm using 'require' to pull in the implementation object, which now exists in its own file.

Regarding the "build" process, my understanding is the 'browserify' step is only used to produce a standalone js file that is consumed by the app that uses the component. Otherwise, the tests and the demo use the js file registered with bower.

"line 20: what happens if this mediaPlayerId is not set yet?"
"line 39: this function is assuming that this.mediaPlayer is always
defined."

I've added an 'attachTo' API function that takes the player as an argument. And the implementation object's constructor now takes the player as an argument. There's also a 'detachFrom' API function if the app wants to swap media elements.

"line 34: if you need to make this an option, let's do it with an
attribute and property pair and let the app do the user agent
sniffing."

I don't have a good explanation for this but I'm seeing that 'fastSeek' is working fine in the browser. I've removed the 'seekTo' function and am directly using 'fastSeek' instead.

"line 76: do you need to do a preventDefault() on context menu events
to actually prevent a context menu from being displayed?"

I'm no longer using the 'contextmenu' event to support the "longpress" functionality. Instead I'm detecting when a touch or mouse press is continuously held.

"handleEvent" comments:

I'm now switching on 'e.type' and implementing the functionality within the function; no longer calling functions to do the work (unless the functions are invoked in more than one place). I've moved the 'slider' handling code to the end of the function as you suggested.

"line 497: I had to look up flex-flow. I think flex-direction would be
the simpler thing to do here and below since you're not setting any
wrapping properties.
line 498: I would have expected align-items: stretch so that both rows
contained in this column are automatically forced to be as wide as the
container."

I've made these changes.

Comment 19

[Russ Nicoletti [:russn]]

Attachment: Link to github PR: https://github.com/gaia-components/gaia-media-controls/pull/1

David, as I have commented on the PR, the README will need further updates to address: 1) It mentions installing the component's dependencies via bower but I have not yet registered the component with bower. The workaround is to install the dependencies one at a time ('bower install gaia-component; bower install gaia-icons'). 2) The demo url will need to be changed when it is pushed to its proper github location (it is currently pointing to my github 'pages' location).

Comment 20

[Russ Nicoletti [:russn]]

I'm creating a web component for media controls (the goal is to use it in the gallery and camera apps -- I'm assuming the video app won't be part of v3). The starting point for the component was the markup/code/css from the video app that is related to video controls. A demo is here [1].

Wilson mentioned there is a gaia-slider web component that I should consider using in the media controls component. I have a demo of the media controls component using the gaia-slider component here [2]. Can you have a look at both demos and provide your feedback? I would like to use the gaia-slider component in the media controls component because it reduces a lot of unnecessary code/markup/css, and, I'm starting to like the look-and-feel of it better than the one based on the video app controls. 

[1] http://russnicoletti.github.io/media-controls/index.html
[2] http://russnicoletti.github.io/media-controls-gaia-slider/index.html

You will probably notice the slider is not vertically centered in [2]. Please ignore that as it is only a POC.

Thanks in advance for your feedback.

Comment 21

[Russ Nicoletti [:russn]]

Wilson, as you can see from [2] in comment 20, I have integrated gaia-slider into the media-controls component. I found that gaia-slider required some changes to be used in the media controls component. Specifically, so that the media controls can:

1) Set length of the input field (input.max) to the duration of the media 
2) Set the 'step' to 1 so that moving the thumb by 1 corresponds to one second of the media
3) Set the input.value so that the slider moves as the media is played

Currently I have added API functions to set these values but I plan on changing them to attributes of the component; that is more consistent with the 'input' tag that the component is wrapping.

Lastly, in order for the media controls to act on the slider being moved by the user -- to seek the video accordingly, I have updated the slider component to emit an event with input.value as the payload.

Can you comment on these changes? Do they seem appropriate to you?

Unfortunately, the slider is not working as well on my flame as it is in the browser. I've incorporated the media controls component that's using gaia-slider into the video app. I'm finding I'm not able to drag the thumb when it is running on the flame. From my investigation, it seems the 'input' element is not firing an 'input' event when the thumb is pressed, as it does when running in the browser. Do you have any insight into this?

Comment 22

[Russ Nicoletti [:russn]]

Hi Amy, please comment 20 regarding using the gaia-slider web component in the media-controls web component (under construction).

Comment 23

[Russ Nicoletti [:russn]]

s/please/please see

Comment 24

[Russ Nicoletti [:russn]]

Wilson, following up on comment 21, I'm finding that firing an event in the 'input' event handler (onChange) is a bad idea. Doing so causes the 'active' css does to not take affect and causes the movement of the thumb to be erratic (e.g., the movement stops if the pointer moves too far off the thumb). I also tried having the client of the component specify a callback function to be called by the onChange handler but that has the same negative side-affects. It's not clear to me how else to notify the client the slider is being moved.

Comment 25

[Russ Nicoletti [:russn]]

Amy, currently the gaia-slider isn't fully integrated into the media controls component. Specifically, dragging the slider does not move the position of the video. And dragging the slider while the video is playing results in some erratic behavior. To experience the look and feel of dragging the gaia-slider, best to do it while the video is paused.

Comment 26

[Amy Lee [:amylee] UX]

(In reply to Russ Nicoletti [:russn] from comment #22)
> Hi Amy, please comment 20 regarding using the gaia-slider web component in
> the media-controls web component (under construction).

Hi, 

I'll let Hung comment who has worked on web components. I think the slider works in this scenario.

Comment 27

[Hung Nguyen]

Hi Russ 

I'm fine with using the gaia-slider web component in this scenario. 

Thanks for throwing together the demo, it was good to see it in action. 

Cheers!

Comment 28

[Wilson Page [:wilsonpage]]

(In reply to Russ Nicoletti [:russn] from comment #24)
> Wilson, following up on comment 21, I'm finding that firing an event in the
> 'input' event handler (onChange) is a bad idea. Doing so causes the 'active'
> css does to not take affect and causes the movement of the thumb to be
> erratic (e.g., the movement stops if the pointer moves too far off the
> thumb). I also tried having the client of the component specify a callback
> function to be called by the onChange handler but that has the same negative
> side-affects. It's not clear to me how else to notify the client the slider
> is being moved.

Could you fire it async using `setTimeout()`?

Comment 29

[Wilson Page [:wilsonpage]]

(In reply to Russ Nicoletti [:russn] from comment #21)
> Wilson, as you can see from [2] in comment 20, I have integrated gaia-slider
> into the media-controls component. I found that gaia-slider required some
> changes to be used in the media controls component. Specifically, so that
> the media controls can:
> 
> 1) Set length of the input field (input.max) to the duration of the media 
> 2) Set the 'step' to 1 so that moving the thumb by 1 corresponds to one
> second of the media
> 3) Set the input.value so that the slider moves as the media is played

I noticed in the demo that when the thumb is being dragged you're still updating the position to match the current video time, meaning the thumb jumps around. This should probably be ignored when dragging.

> Currently I have added API functions to set these values but I plan on
> changing them to attributes of the component; that is more consistent with
> the 'input' tag that the component is wrapping.

Have a look at the magic `attrs` property feature. It helps to keep attributes and properties in sync. See gaia-header as example.

> Lastly, in order for the media controls to act on the slider being moved by
> the user -- to seek the video accordingly, I have updated the slider
> component to emit an event with input.value as the payload.

Sounds good.

> Can you comment on these changes? Do they seem appropriate to you?
> 
> Unfortunately, the slider is not working as well on my flame as it is in the
> browser. I've incorporated the media controls component that's using
> gaia-slider into the video app. I'm finding I'm not able to drag the thumb
> when it is running on the flame. From my investigation, it seems the 'input'
> element is not firing an 'input' event when the thumb is pressed, as it does
> when running in the browser. Do you have any insight into this?

Sounds like the 'input' event could be a shadow-dom bug. Could you could produce a reduced test case and file a bug against DOM?

Comment 30

[David Flanagan [:djf]]

Comment on attachment 8589154 [details] [review]
Link to github PR: https://github.com/gaia-components/gaia-media-controls/pull/1

I think I found a couple of minor event-handling related bugs in your code.  And I'm not happy with the test-enabling code in the component itself. Can't you put that stuff in the tests themselves?

Various other suggestions and minor comments on github.

Comment 31

[BMO Automation]

Firefox OS is not being worked on