Closed Bug 1444489 Opened 6 years ago Closed 6 years ago

Remove the touchControls binding and instead use the videocontrols binding on mobile

Categories

(Toolkit :: UI Widgets, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

(Blocks 3 open bugs)

Details

Attachments

(10 files)

59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
536.46 KB, image/png
Details
486.84 KB, image/png
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
The touchControls binding 

https://dxr.mozilla.org/mozilla-central/rev/55d91695f4bb951c224005155baef054a786c1f7/toolkit/content/widgets/videocontrols.xml#1909

is the only binding that relies on other xbl bindings to function. As mentioned in  bug 1444193 comment 9, the approach bug 1431255 is going for will not enable us to use custom elements in the future in-content.

We will need to think through the approaches to deal with it.
I wonder if there is some way to "import" the custom elements locally in the in-content widget in JS? Granted it will not follow the usual path, but it would be good to avoid rewrite touchControls.

If the local "import" also handles stylesheets we can invalid bug 1444193. I would like to land the assertion anyway though.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #1)
> I wonder if there is some way to "import" the custom elements locally in the
> in-content widget in JS?

Like rip out <label>s from the markup, and create them dynamically with |new FirefoxLabel(...)| instead. Would it work?
I'd also like to get a sense of what it would take to modify / re-use the HTML markup we currently have for videoControls instead of using XUL widgets (XBL or CE) in touchControls.
Another alternative: try to load videoControls on Android and see if it works. It might be closer than trying to convert few, if not all, elements to HTML in touchControls.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> Another alternative: try to load videoControls on Android and see if it
> works. It might be closer than trying to convert few, if not all, elements
> to HTML in touchControls.

Turned out there is already a bug filed (bug 1310907).
See Also: → 1310907
I have a WIP patch that makes videoControls work on Fennec.

The differences I found so far are:

- The mobile control bar is a little bit taller to make the buttons bigger.
- There isn't a volume slider. There is one in the markup, I assume it shows up in <audio> only (need test)
- There isn't a closed caption button but WebVTT works on mobile.
- There is a Casting button on mobile control — missing on desktop.
- The full screen button looks different (I am not sure if we care).

Overall the design looks pretty similar and the interaction is almost identical — I am pretty sure the effort to tweak desktop controls to what mobile needs will be much smaller than trying to do anything to touchControls.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #7)
> I have a WIP patch that makes videoControls work on Fennec.
>
> Overall the design looks pretty similar and the interaction is almost
> identical — I am pretty sure the effort to tweak desktop controls to what
> mobile needs will be much smaller than trying to do anything to
> touchControls.

That's great progress - using the videoControls instead of touchControls would be a win for at least a few reasons:
1) it would delete an in-content binding
2) it would let us to remove the "load xul.css in content" condition entirely
3) it would mean we don't have to worry about supporting XUL Custom Elements inside of native anonymous content
Blocks: 726240
4: By removing `touchControls` and using `videoControls` exclusively, we end up using responsive design, rather than serving two different layouts, one for desktop and one for mobile, which removes code duplication and will probably make things easier to manage in the long term.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
The new touch control is simply the desktop control with dimension enlarged by around 1.3 times to match the size of the original touch control.
Attachment #8958803 - Attachment description: old-touchcontrols.png → Current video control on Nightly
Attachment #8958801 - Attachment description: new-touchcontrols.png → Resized desktop video controls on mobile
@shorlander and I reviewed your attachments this AM and they look good to us. Thanks!
Summary: Find a way to deal with the touchControls binding → Remove the touchControls binding and instead use the videocontrols binding on mobile
See Also: 1310907
Attachment #8958100 - Flags: review?(dholbert)
Blocks: 1444193
See Also: 1444193
Comment on attachment 8958100 [details]
Bug 1444489 - Part V, Remove XUL videocontrols reflow code on Android

https://reviewboard.mozilla.org/r/227054/#review233728

r=me

::: commit-message-d66df:1
(Diff revision 4)
> +Bug 1444489 - Part IV, Remove xul videocontrols reflow code

Two requests for the commit message here (either in first line or in subsequent extra-info lines, up to you):

 (1) Please mention "Android"  (since this patch is Android-specific -- the deleted code is all #ifdef ANDROID)

 (2) Please make it clear why this code is deletable (I'm guessing it's to reflow frames for elements that no longer exist at this point in the patch stack -- is that right?)
Attachment #8958100 - Flags: review?(dholbert) → review+
Comment on attachment 8957737 [details]
Bug 1444489 - Part III, Workaround bug 718107

https://reviewboard.mozilla.org/r/226696/#review233788


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/widgets/videocontrols.xml:1117
(Diff revision 9)
> -        this.updateOrientationState(this.isVideoInFullScreen());
> +        // XXX This should be the place where we lock/unlock orientation
> +        // according to the fullscreen state. Sadly because of bug 718107 as
> +        // soon as this function exits the attached binding gets destructored.
> +        // we therefore omit the orientation state update here and have the
> +        // constructor to set the orientation lock instead.
> +        //this.updateOrientationState(this.isVideoInFullScreen());

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]
Comment on attachment 8957749 [details]
Bug 1444489 - Part IV, Implement Casting UI on videoControls

https://reviewboard.mozilla.org/r/226710/#review233792


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/widgets/videocontrols.xml:1118
(Diff revision 9)
>          // XXX This should be the place where we lock/unlock orientation
>          // according to the fullscreen state. Sadly because of bug 718107 as
>          // soon as this function exits the attached binding gets destructored.
>          // we therefore omit the orientation state update here and have the
>          // constructor to set the orientation lock instead.
>          //this.updateOrientationState(this.isVideoInFullScreen());

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]
Comment on attachment 8958800 [details]
Bug 1444489 - Part VII, nit: Improve naming consistency in videocontrols.xml

https://reviewboard.mozilla.org/r/227710/#review233794


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/widgets/videocontrols.xml:1118
(Diff revision 3)
>          // XXX This should be the place where we lock/unlock orientation
>          // according to the fullscreen state. Sadly because of bug 718107 as
>          // soon as this function exits the attached binding gets destructored.
>          // we therefore omit the orientation state update here and have the
>          // constructor to set the orientation lock instead.
> -        //this.updateOrientationState(this.isVideoInFullScreen());
> +        //this.updateOrientationState(this.isVideoInFullScreen);

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]
Attachment #8958099 - Flags: review?(gijskruitbosch+bugs)
Attachment #8957737 - Flags: review?(gijskruitbosch+bugs)
Attachment #8957749 - Flags: review?(gijskruitbosch+bugs)
Attachment #8957764 - Flags: review?(gijskruitbosch+bugs)
Attachment #8958800 - Flags: review?(gijskruitbosch+bugs)
Attachment #8959394 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8958099 [details]
Bug 1444489 - Part I, Convert noControls binding to HTML content

https://reviewboard.mozilla.org/r/227052/#review234162

::: browser/base/content/test/static/browser_parsable_css.js:106
(Diff revision 5)
> +  {propName: "--clickToPlay-width",
> +   isFromDevTools: false},
> +  // Bug 1441857
> +  {propName: "--clickToPlay-height",
> +   isFromDevTools: false},

But these are actually not referenced?

::: toolkit/content/widgets/videocontrols.xml
(Diff revision 5)
>  </binding>
>  
>  <binding id="noControls">
>  
>    <resources>
> -    <stylesheet src="chrome://global/content/bindings/videocontrols.css"/>

Do we no longer need https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.css#24-31 ? Why not?

::: toolkit/themes/mobile/jar.mn:46
(Diff revision 5)
>     skin/classic/global/media/imagedoc-lightnoise.png       (global/media/imagedoc-lightnoise.png)
>     skin/classic/global/media/imagedoc-darknoise.png        (global/media/imagedoc-darknoise.png)
>  
> +* skin/classic/global/media/videocontrols.css              (../shared/media/videocontrols.css)
> +  skin/classic/global/media/pauseButton.svg                (../shared/media/pauseButton.svg)
> +  skin/classic/global/media/playButton.svg                 (../shared/media/playButton.svg)

These changes seem like they don't belong in this changeset? There are no controls in the binding that this is altering...

::: toolkit/themes/shared/media/videocontrols.css:22
(Diff revision 5)
> -  --clickToPlay-width: 48px;
> -  --clickToPlay-height: var(--clickToPlay-width);
> +  --clickToPlay-width: var(--clickToPlay-size);
> +  --clickToPlay-height: var(--clickToPlay-size);

Why keep the width/height variants? They're unused now, right?
Attachment #8958099 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8957737 [details]
Bug 1444489 - Part III, Workaround bug 718107

https://reviewboard.mozilla.org/r/226696/#review234178

I think this is broken, per the below, and I think the geckoview changes need review from a mobile peer in addition to the video controls review. It's not clear to me whether geckoview uses the mobile theming, for instance, and breaking media control for geckoview would be bad.

I haven't reviewed this extensively because the brokenness looks serious enough and I'm really busy, so it doesn't seem useful to dive into this in more depth if we're going to need a change of plan for the binding removal anyway.

::: toolkit/content/widgets/videocontrols.xml
(Diff revision 10)
> -<binding id="suppressChangeEvent"
> -         extends="chrome://global/content/bindings/scale.xml#scale">

You've not removed the css in the bindings for this, and this is still used by the desktop controls (which are becoming the touch controls here) at https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#163 and https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#177 . So I don't think this is right.
Attachment #8957737 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8957749 [details]
Bug 1444489 - Part IV, Implement Casting UI on videoControls

https://reviewboard.mozilla.org/r/226710/#review234186

Casting support doesn't exist on desktop, so the button should be hidden there, and I don't see any code in this patch that does that (probably based on AppConstants or ifdef'd or whatever).

::: toolkit/themes/shared/media/castingButton-active.svg:1
(Diff revision 10)
> +<svg width="66" height="54" xmlns="http://www.w3.org/2000/svg">
> +  <path d="M0 45v9h9c0-4.98-4.02-9-9-9zm0-12v6c8.28 0 15 6.72 15 15h6c0-11.61-9.39-21-21-21zm54-21H12v4.89C23.88 20.73 33.27 30.12 37.11 42H54V12zM0 21v6c14.91 0 27 12.09 27 27h6c0-18.24-14.79-33-33-33zM60 0H6C2.7 0 0 2.7 0 6v9h6V6h54v42H39v6h21c3.3 0 6-2.7 6-6V6c0-3.3-2.7-6-6-6z" fill="context-fill" fill-rule="evenodd"/>

Where did these SVG files come from? If they're copied/moved from elsewhere, can you make sure they're annotated as such through hg?

Either way they need a license header, and why does the height/width not match up with the size we used them at?

::: toolkit/themes/shared/media/videocontrols.css:168
(Diff revision 10)
>    display: none;
>  }
>  
> +.castingButton {
> +  background-image: url(chrome://global/skin/media/castingButton-ready.svg);
> +  background-size: 18px 14.727px;

Why do we need this?
Attachment #8957749 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8957764 [details]
Bug 1444489 - Part VI, Enlarge the size of controls on mobile

https://reviewboard.mozilla.org/r/226750/#review234192

::: toolkit/themes/shared/media/videocontrols.css:29
(Diff revision 9)
>    list-style-image: none !important;
>    font: normal normal normal 100%/normal sans-serif !important;
>    text-decoration: none !important;
>  }
>  
> -.controlContainer {
> +.controlsContainer {

This should be in an earlier cset.

::: toolkit/themes/shared/media/videocontrols.css:31
(Diff revision 9)
>    text-decoration: none !important;
>  }
>  
> -.controlContainer {
> +.controlsContainer {
> +%ifdef DESKTOP_VIDEOCONTROLS
>    --clickToPlay-size: 48px;

The "whole point" of variables is that we could just set the variables differently based on a class or attribute that we set on one of the nodes, and change the variables that way, causing all the elements that use them to be restyled.

In other words, can we implement this change without all the ifdefs? We can add font size variables where sizes aren't yet in variables if that's necessary.

::: toolkit/themes/shared/media/videocontrols.css:127
(Diff revision 9)
>    box-sizing: border-box;
>    justify-content: center;
>    align-items: center;
>    overflow: hidden;
> +%ifdef DESKTOP_VIDEOCONTROLS
>    height: 40px;

Looks like this should just be an existing variable.

::: toolkit/themes/shared/media/videocontrols.css:154
(Diff revision 9)
>    background-origin: content-box;
>    background-clip: content-box;
>    -moz-context-properties: fill;
>    fill: #ffffff;
> +%ifdef TOUCH_VIDEOCONTROLS
> +  background-size: 24px 24px;

Why do we not need a bg size for desktop? Are the SVGs going to look blurry or jagged on hi-res mobile devices?
Attachment #8957764 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8958800 [details]
Bug 1444489 - Part VII, nit: Improve naming consistency in videocontrols.xml

https://reviewboard.mozilla.org/r/227710/#review234198
Attachment #8958800 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8959394 [details]
Bug 1444489 - Part VIII, Transition the visibility property instead of using transitionend event

https://reviewboard.mozilla.org/r/228212/#review234202

::: toolkit/themes/shared/media/videocontrols.css:513
(Diff revision 1)
>  }
>  
>  .clickToPlay[fadeout] {
>    transform: scale(3);
>    opacity: 0;
> +  visibility: hidden;

Visibility will just change mid-transition now, won't it? Thus changing the visibility halfway through the transition, cutting off the opacity transition. So I don't think this is right...
Attachment #8959394 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8957737 [details]
Bug 1444489 - Part III, Workaround bug 718107

https://reviewboard.mozilla.org/r/226696/#review234178

> You've not removed the css in the bindings for this, and this is still used by the desktop controls (which are becoming the touch controls here) at https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#163 and https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#177 . So I don't think this is right.

CSS is removed in toolkit/content/widgets/videocontrols.css from the entire file.

The binding is not being used on HTML <input type=range> because the selector is in xul namespace.
Comment on attachment 8957749 [details]
Bug 1444489 - Part IV, Implement Casting UI on videoControls

https://reviewboard.mozilla.org/r/226710/#review234186

It will always hidden on runtime because the control will not receive media-videoCasting event. Do you want to it to be hidden on build time? I can do that in jar.mn & videocontrols.css.

Noted that we do not have access to AppConstants in the content process and ESLint doesn't like #ifdef in JavaScript.

> Where did these SVG files come from? If they're copied/moved from elsewhere, can you make sure they're annotated as such through hg?
> 
> Either way they need a license header, and why does the height/width not match up with the size we used them at?

Did you read the commit message. They are SVGO'd version of the original casting button SVGs from the previous commit. How do you want to annotate in HG for a feature removed from one commit and added back from another? Would you like me to merge the two commits together?

As of the license header: the original files didn't come with them. Should I just add them and assume they are licensed as such?

> Why do we need this?

Because SVGO does not scale the image to the size we want and I am too lazy to manually scale it in Inkscape. Should I do that so that we could not use a CSS feature here?
Comment on attachment 8957764 [details]
Bug 1444489 - Part VI, Enlarge the size of controls on mobile

https://reviewboard.mozilla.org/r/226750/#review234192

> The "whole point" of variables is that we could just set the variables differently based on a class or attribute that we set on one of the nodes, and change the variables that way, causing all the elements that use them to be restyled.
> 
> In other words, can we implement this change without all the ifdefs? We can add font size variables where sizes aren't yet in variables if that's necessary.

I personally think it will be very hard to track the origin of the numbers if we have multiple variables references one and another (and calc(), thank God there is no floor() and ceil() in CSS). That's why I implemented this in #ifdef.

If you still have problem with it I will do what you said, i.e. have one and only one #ifdef block to set a few variables and calc() our way to produce other values including values in these access-by-JS-only variables.

> Why do we not need a bg size for desktop? Are the SVGs going to look blurry or jagged on hi-res mobile devices?

The native size of the SVGs are 18x18, so we don't need the bg size there.

I don't see any blurry or jagged on mobile, in fact I will be very suprised if they do. I am told on comment 44 that shorlander has reviewed the size change. What you do suggest I should do more with this?
Comment on attachment 8959394 [details]
Bug 1444489 - Part VIII, Transition the visibility property instead of using transitionend event

https://reviewboard.mozilla.org/r/228212/#review234202

> Visibility will just change mid-transition now, won't it? Thus changing the visibility halfway through the transition, cutting off the opacity transition. So I don't think this is right...

No. See https://www.w3.org/TR/css-transitions-1/#animtype-visibility . The element is visibile during the entire transition, regardless of transition to hidden or transition to visible.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #69)
> > Where did these SVG files come from? If they're copied/moved from elsewhere, can you make sure they're annotated as such through hg?
> > 
> > Either way they need a license header, and why does the height/width not match up with the size we used them at?
> 
> Did you read the commit message. They are SVGO'd version of the original
> casting button SVGs from the previous commit. How do you want to annotate in
> HG for a feature removed from one commit and added back from another? Would
> you like me to merge the two commits together?

If I understand correctly, it would make sense to update the SVG in place in one commit instead of removing them and re-adding the optimized version in separate commits. This would make the diff easier to follow and also allow us to keep history on the file.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #70)
> Comment on attachment 8957764 [details]
> Bug 1444489 - Part V, Enlarge the size of controls on mobile
> 
> https://reviewboard.mozilla.org/r/226750/#review234192
> 
> > The "whole point" of variables is that we could just set the variables differently based on a class or attribute that we set on one of the nodes, and change the variables that way, causing all the elements that use them to be restyled.
> > 
> > In other words, can we implement this change without all the ifdefs? We can add font size variables where sizes aren't yet in variables if that's necessary.
> 
> I personally think it will be very hard to track the origin of the numbers
> if we have multiple variables references one and another (and calc(), thank
> God there is no floor() and ceil() in CSS). That's why I implemented this in
> #ifdef.
> 
> If you still have problem with it I will do what you said, i.e. have one and
> only one #ifdef block to set a few variables and calc() our way to produce
> other values including values in these access-by-JS-only variables.

I don't think we need any ifdefs, that was sort of my point. The way that I'd think we'd want to do this is to do something like:

.controlsBar {
 --foopy: 15px;
}

.controlsBar[touch] {
  --foopy: 20px;
}

and then set the `touch` attribute in the binding code. Then we can just use `--foopy` everywhere, and it'll do the right thing both in the touch and the non-touch cases.

Would that work?
Flags: needinfo?(timdream)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #69)
> Noted that we do not have access to AppConstants in the content process and
> ESLint doesn't like #ifdef in JavaScript.

We have access to AppConstants in the content process, but not in unprivileged code.

https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#1361

uses navigator.platform to determine whether we're on Mac. I assume we can do the same for Android.

> Did you read the commit message.

I missed it, sorry.

> They are SVGO'd version of the original
> casting button SVGs from the previous commit. How do you want to annotate in
> HG for a feature removed from one commit and added back from another? Would
> you like me to merge the two commits together?

I think they should be moved in one commit, yes.

> As of the license header: the original files didn't come with them. Should I
> just add them and assume they are licensed as such?

You can find out where they come from with hg annotate/blame, but unless they came from outside moco/mofo (e.g. from some external icon set?) they should be licensed with the regular MPL, yes.

> > Why do we need this?
> 
> Because SVGO does not scale the image to the size we want and I am too lazy
> to manually scale it in Inkscape. Should I do that so that we could not use
> a CSS feature here?

I think it would be best to ensure that the image size is correct without the additional CSS, yes.
Comment on attachment 8959394 [details]
Bug 1444489 - Part VIII, Transition the visibility property instead of using transitionend event

https://reviewboard.mozilla.org/r/228212/#review234346

OK, so based on the docs you linked to, I guess this works. Note the issue below, though - and I'm assuming this actually fixes the issue on try? (The trypush mozreview is showing me is orange on android mochitest-plain for the modified test, but doesn't seem to include this cset, unless I'm missing something, and I don't see any trypushes linked in the bug.)

::: toolkit/content/tests/widgets/test_videocontrols_error.html:35
(Diff revision 2)
>      await new Promise(resolve => {
>        video.src = "seek_with_sound.ogg";
>        video.addEventListener("loadedmetadata", () => SimpleTest.executeSoon(resolve));
>      });
>  
> -    // Wait for the fade out transition to complete in case the throbber
> +    await SimpleTest.promiseWaitForCondition(() => statusOverlay.hasAttribute("fadeout") || statusOverlay.hidden,

Is something else still setting .hidden or should we just remove the checks for `.hidden` entirely (here and below) ?
Attachment #8959394 - Flags: review?(gijskruitbosch+bugs) → review+
FWIW, what with the swapping bits between csets, I expect I will come back to these requests after the next set up of updates to the patches, if that's OK.
(In reply to :Gijs from comment #80)
> .controlsBar {
>  --foopy: 15px;
> }
> 
> .controlsBar[touch] {
>   --foopy: 20px;
> }
> 
> and then set the `touch` attribute in the binding code. Then we can just use
> `--foopy` everywhere, and it'll do the right thing both in the touch and the
> non-touch cases.

A better option would probably be:

```
.controls-bar {
  --foopy: 15px;
}

/* Q: Does the :root in XBL <content> point to the root of the shadow XBL content or the root of the light DOM? */
/* If the latter, then this would need to be replaced with `:host[touch]` or something */
:root[touch] .controls-bar {
  --foopy: 15px;
}
```
(In reply to ExE Boss from comment #84)
> A better option would probably be:
> 
> ```
> .controls-bar {
>   --foopy: 15px;
> }
> 
> /* Q: Does the :root in XBL <content> point to the root of the shadow XBL
> content or the root of the light DOM? */
> /* If the latter, then this would need to be replaced with `:host[touch]` or
> something */
> :root[touch] .controls-bar {
>   --foopy: 15px;
> }
> ```

Why do you think this is better, given it adds a descendant selector?

(FWIW, I'm not aware of there being a good way to select for the root of the XBL content anyway. :root almost certainly points to the document root rather than the XBL root.)
Flags: needinfo?(e7358d9c)
(In reply to :Gijs from comment #85)
> (In reply to ExE Boss from comment #84)
> > A better option would probably be:
> > 
> > ```
> > .controls-bar {
> >   --foopy: 15px;
> > }
> > 
> > /* Q: Does the :root in XBL <content> point to the root of the shadow XBL
> > content or the root of the light DOM? */
> > /* If the latter, then this would need to be replaced with `:host[touch]` or
> > something */
> > :root[touch] .controls-bar {
> >   --foopy: 15px;
> > }
> > ```
> 
> Why do you think this is better, given it adds a descendant selector?
> 
> (FWIW, I'm not aware of there being a good way to select for the root of the
> XBL content anyway. :root almost certainly points to the document root
> rather than the XBL root.)

Beause the `touch` attribute (or whatever we will decide to call it) only needs to be set at the root instead of being set for every element in the tree.
Flags: needinfo?(e7358d9c)
(In reply to :Gijs from comment #81)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #69)
> > Noted that we do not have access to AppConstants in the content process and
> > ESLint doesn't like #ifdef in JavaScript.
> 
> We have access to AppConstants in the content process, but not in
> unprivileged code.
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> videocontrols.xml#1361
> 
> uses navigator.platform to determine whether we're on Mac. I assume we can
> do the same for Android.

We can't, sorry. navigator.platform is Linux on Fennec.

I do see |navigator.userAgent.includes("Android")| being used in test scripts. Do you see that as a lesser evil to #ifdef :-P ?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #87)
> We can't, sorry. navigator.platform is Linux on Fennec.
> 
> I do see |navigator.userAgent.includes("Android")| being used in test
> scripts. Do you see that as a lesser evil to #ifdef :-P ?

You could use navigator.appVersion, I think, but yeah, I think that'd be preferred over ifdef. Eventually we'll get rid of preprocessing. Alternatively, you could set the attribute from C++ when creating the <videocontrols> element in layout land.

(In reply to ExE Boss from comment #86)
> Beause the `touch` attribute (or whatever we will decide to call it) only
> needs to be set at the root instead of being set for every element in the
> tree.

But we wouldn't need to do that - the variables get defined on the topmost element where they're needed (probably the controlsContainer or whatever it's called) only, and because of how CSS works you can then use those properties in the subtree without any further work. We should only need to define variables on 1 element, so only that element will need an attribute. Worst case, that element can be the <videocontrols> element itself.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8959394 [details]
Bug 1444489 - Part VIII, Transition the visibility property instead of using transitionend event

https://reviewboard.mozilla.org/r/228212/#review234346

I did try pushes from the command line. This is the push of the latest commits: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17fdd84f25840790769e2c1a924357debb5b89a2

Yes this commit fixes the test on Android; for some reason transitionend is not reliable timing-wise for the new combinations.

> Is something else still setting .hidden or should we just remove the checks for `.hidden` entirely (here and below) ?

The element starts with hidden=true, and it will never get unset if the platform loads the video fast enough which prevents the initial throbber to show.
Comment on attachment 8957764 [details]
Bug 1444489 - Part VI, Enlarge the size of controls on mobile

https://reviewboard.mozilla.org/r/226750/#review234454


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/widgets/videocontrols.xml:1693
(Diff revision 11)
>          if (this.positionDurationBox) {
>            this.durationSpan = this.positionDurationBox.getElementsByTagName("span")[0];
>          }
>  
> +        this.videocontrols.isTouchControls = true;
> +          //navigator.appVersion.includes("Android");

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]
Comment on attachment 8958800 [details]
Bug 1444489 - Part VII, nit: Improve naming consistency in videocontrols.xml

https://reviewboard.mozilla.org/r/227710/#review234456


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/widgets/videocontrols.xml:1693
(Diff revision 6)
>          if (this.positionDurationBox) {
>            this.durationSpan = this.positionDurationBox.getElementsByTagName("span")[0];
>          }
>  
>          this.videocontrols.isTouchControls = true;
>            //navigator.appVersion.includes("Android");

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]
Comment on attachment 8959394 [details]
Bug 1444489 - Part VIII, Transition the visibility property instead of using transitionend event

https://reviewboard.mozilla.org/r/228212/#review234458


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/widgets/videocontrols.xml:1677
(Diff revision 3)
>          if (this.positionDurationBox) {
>            this.durationSpan = this.positionDurationBox.getElementsByTagName("span")[0];
>          }
>  
>          this.videocontrols.isTouchControls = true;
>            //navigator.appVersion.includes("Android");

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]
Comment on attachment 8959394 [details]
Bug 1444489 - Part VIII, Transition the visibility property instead of using transitionend event

https://reviewboard.mozilla.org/r/228212/#review234346

Sorry, I've made a mistake. The correct try push should be

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19712ce334419345dbd2a1f550dd9256302f128d
Comment on attachment 8959394 [details]
Bug 1444489 - Part VIII, Transition the visibility property instead of using transitionend event

https://reviewboard.mozilla.org/r/228212/#review234346

and with a test fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42e444d27888b33a020c4d9e76de22f006731319
Patch updated -- I forget to remove the needinfo!
Flags: needinfo?(timdream)
Comment on attachment 8958099 [details]
Bug 1444489 - Part I, Convert noControls binding to HTML content

https://reviewboard.mozilla.org/r/227052/#review235054

::: toolkit/themes/mobile/jar.mn:44
(Diff revision 7)
>     skin/classic/global/media/TopLevelImageDocument.css     (global/media/TopLevelImageDocument.css)
>     skin/classic/global/media/TopLevelVideoDocument.css     (global/media/TopLevelVideoDocument.css)
>     skin/classic/global/media/imagedoc-lightnoise.png       (global/media/imagedoc-lightnoise.png)
>     skin/classic/global/media/imagedoc-darknoise.png        (global/media/imagedoc-darknoise.png)
>  
> +* skin/classic/global/media/videocontrols-html.css         (../shared/media/videocontrols.css)

You rename this to just plain `videocontrols.css` in the next cset. Seems to me we should just add it without the `-html` suffix in the first place in this commit.
Attachment #8958099 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8957749 [details]
Bug 1444489 - Part IV, Implement Casting UI on videoControls

https://reviewboard.mozilla.org/r/226710/#review235060

r=me with the label put back (might need to remove deletion from the previous cset if that's where it happened?) and the `isTrusted` check restored.

::: toolkit/content/widgets/videocontrols.xml:69
(Diff revision 13)
>                    unmutelabel="&muteButton.unmuteLabel;"
>                    tabindex="-1"/>
>            <div anonid="volumeStack" class="volumeStack progressContainer" role="none">
>              <input type="range" anonid="volumeControl" class="volumeControl" min="0" max="100" step="1" tabindex="-1"/>
>            </div>
> +          <button anonid="castingButton" class="castingButton"/>

This used to have a text label (I expect for a11y) - it probably should still have one? I'm guessing we'll want to add it as an `aria-label` attribute, judging by what we do for the other buttons.

::: toolkit/content/widgets/videocontrols.xml:1783
(Diff revision 13)
> +          addListener(this.video, "media-videoCasting",
> +            (evt) => this.updateCasting(evt.detail));
> +        }

It seems you removed the `evt.isTrusted` check compared to the previous implementation (cf. https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/toolkit/content/widgets/videocontrols.xml#2116-2121 ). Please add it back.
Attachment #8957749 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8957737 [details]
Bug 1444489 - Part III, Workaround bug 718107

https://reviewboard.mozilla.org/r/226696/#review235062

This is basically ready except that I don't understand the fullscreen-related changes. Can you explain why they're part of this cset? From bug 718107 it seems the issue applies equally to mobile and desktop, so apart from moving things around (which isn't what's happening here, as far as I can tell - I can't find equivalent code in the pre-existing binding) I don't really understand why those changes are here.

::: toolkit/content/tests/widgets/test_videocontrols_error.html:38
(Diff revision 13)
>      });
>  
> -    ok(statusOverlay.hidden, "statusOverlay shoud not present without error");
> +    // Wait for the fade out transition to complete in case the throbber
> +    // shows up on slower platforms.
> +    await SimpleTest.promiseWaitForCondition(() => statusOverlay.hidden,
> +      "statusOverlay shoud not present without error");

Nit: Can we fix the spelling while we're here? :-) (s/shoud/should/)

::: toolkit/content/widgets/videocontrols.xml:1108
(Diff revision 13)
> -        this.updateOrientationState(this.isVideoInFullScreen());
> +        // XXX This should be the place where we lock/unlock orientation
> +        // according to the fullscreen state. Sadly because of bug 718107 as
> +        // soon as this function exits the attached binding gets destructored.
> +        // we therefore omit the orientation state update here and have the
> +        // constructor to set the orientation lock instead.
> +        //
> +        // this.updateOrientationState(this.isVideoInFullScreen());

I'm a little bit confused by the changes related to fullscreen. Does this deterministically happen now? Are we actually fixing an orthogonal bug (and in that case, can we do that in a separate cset, please)? Why can we omit the `updateOrientationState` call but need to keep the timeout and button state update?

::: toolkit/content/widgets/videocontrols.xml:1823
(Diff revision 13)
>        terminateEventListeners() {
>          for (var event of this.videoEvents) {
>            try {
>              this.Utils.video.removeEventListener(event, this);
>            } catch (ex) {}
>          }
>        },

As far as I can tell the TouchUtils never have a defined `videoEvents` property. By contrast, some of the event listeners look like they wouldn't be removed (the casting one and the moved `mouseup` handler, for example).

If I'm not missing something, can you file a follow-up for this issue please? (It predates your changes.)
Attachment #8957737 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8957764 [details]
Bug 1444489 - Part VI, Enlarge the size of controls on mobile

https://reviewboard.mozilla.org/r/226750/#review235068

r=me with some minor comments below.

::: toolkit/content/widgets/videocontrols.xml:1692
(Diff revision 13)
> +        this.videocontrols.isTouchControls =
> +          navigator.appVersion.includes("Android");

Can you file a follow-up to also use the 'touch control' mode on Windows 8+ if we detect there's no mouse (ie tablets etc.) ?

::: toolkit/content/widgets/videocontrols.xml:1695
(Diff revision 13)
>          }
>  
> +        this.videocontrols.isTouchControls =
> +          navigator.appVersion.includes("Android");
> +        if (this.videocontrols.isTouchControls) {
> +          this.controlsContainer.classList.add("touch");

I'm tempted to just add the class to the control bar as well to simplify some of the selectors, but I'm not super convinced either way. Up to you.

::: toolkit/themes/shared/media/videocontrols.css:153
(Diff revision 13)
> +.touch .playButton,
> +.touch .muteButton,
> +.touch .castingButton,
> +.touch .closedCaptionButton,
> +.touch .fullscreenButton {

Nit: `.touch .controlBar > button`

would cover all of these, right?
Attachment #8957764 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #114)
> ::: toolkit/content/widgets/videocontrols.xml:1823
> (Diff revision 13)
> >        terminateEventListeners() {
> >          for (var event of this.videoEvents) {
> >            try {
> >              this.Utils.video.removeEventListener(event, this);
> >            } catch (ex) {}
> >          }
> >        },
> 
> As far as I can tell the TouchUtils never have a defined `videoEvents`
> property. By contrast, some of the event listeners look like they wouldn't
> be removed (the casting one and the moved `mouseup` handler, for example).
> 
> If I'm not missing something, can you file a follow-up for this issue
> please? (It predates your changes.)

Err, the mouseup is obviously not a video event - but the casting event looks like it is (at least the listener is added to the 'video' element).
Comment on attachment 8958099 [details]
Bug 1444489 - Part I, Convert noControls binding to HTML content

https://reviewboard.mozilla.org/r/227052/#review235054

> You rename this to just plain `videocontrols.css` in the next cset. Seems to me we should just add it without the `-html` suffix in the first place in this commit.

Actually, no. Because on this cset I have not remove this line:

https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/mobile/android/themes/geckoview/jar.mn#35

So the style sheet here must not occupy the same URL.
Comment on attachment 8957737 [details]
Bug 1444489 - Part III, Workaround bug 718107

https://reviewboard.mozilla.org/r/226696/#review235062

> I'm a little bit confused by the changes related to fullscreen. Does this deterministically happen now? Are we actually fixing an orthogonal bug (and in that case, can we do that in a separate cset, please)? Why can we omit the `updateOrientationState` call but need to keep the timeout and button state update?

It deterministically happens on my phone and on Try. No, bug 718107 does not happen on touchControls.

Without this workaround, videoControls along cannot set the orientation correctly (only once) and test_videocontrols_orientation.html will deterministically fail.

Your question on the timeout and the button state does make a point. I assume the button state update is moot —— the constructor will update it too —— Given that we don't cancel the timeout on the destructor, I assume `_hideControlsFn` will still run (and can't be cancelled?)
Comment on attachment 8957764 [details]
Bug 1444489 - Part VI, Enlarge the size of controls on mobile

https://reviewboard.mozilla.org/r/226750/#review235068

> Nit: `.touch .controlBar > button`
> 
> would cover all of these, right?

I ended up adding a class named `button` on all the <button>s so the specificity won't change.
Comment on attachment 8957737 [details]
Bug 1444489 - Part III, Workaround bug 718107

https://reviewboard.mozilla.org/r/226696/#review235062

> It deterministically happens on my phone and on Try. No, bug 718107 does not happen on touchControls.
> 
> Without this workaround, videoControls along cannot set the orientation correctly (only once) and test_videocontrols_orientation.html will deterministically fail.
> 
> Your question on the timeout and the button state does make a point. I assume the button state update is moot —— the constructor will update it too —— Given that we don't cancel the timeout on the destructor, I assume `_hideControlsFn` will still run (and can't be cancelled?)

I ended up decided to comment out the entire onFullscreenChange function. See comment in the cset for reasons.

The "mozfullscreenchange" was a result of incorrectly rebase here, I think.

https://searchfox.org/mozilla-central/diff/5a89cbb1ca73446d680b7ee7f4dc7f9aafd4d55d/toolkit/content/widgets/videocontrols.xml#1562

So I remove it.
Attachment #8960883 - Flags: review?(gijskruitbosch+bugs)
Attachment #8957737 - Flags: review?(gijskruitbosch+bugs)
Attachment #8957737 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8960883 [details]
Bug 1444489 - Part II, Replace touchControls with videoControls and remove touchControls

https://reviewboard.mozilla.org/r/229640/#review235408

::: toolkit/content/widgets/videocontrols.xml:1339
(Diff revision 1)
>          return textTrack.kind == "subtitles" ||
>                 textTrack.kind == "captions";
>        },
>  
>        get isClosedCaptionAvailable() {
> -        return this.overlayableTextTracks.length && !this.videocontrols.isTouchControls;
> +        return this.overlayableTextTracks.length;

I'm assuming this was just because some of the necessary work here hadn't been implemented in the touch control binding, not because of limitations in where we ship VTT support? That is, have you tested text track support on Android? Do we have any tests that we could now enable on android, maybe, to ensure that things continue working?
Attachment #8960883 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8957737 [details]
Bug 1444489 - Part III, Workaround bug 718107

https://reviewboard.mozilla.org/r/226696/#review235410

::: commit-message-b72ac:3
(Diff revision 15)
> +Bug 1444489 - Part III, Workaround bug 718107
> +
> +The videocontrols binding will be destoryed and reattached when the video

Nit: destroyed

::: toolkit/content/widgets/videocontrols.xml:232
(Diff revision 15)
>              this.muteButton.setAttribute("disabled", "true");
>            }
>          }
>  
> +        // We should lock the orientation if we are already in
> +        // fullscreen and was reattached because of bug 718107.

Nit: 'and were reattached'

::: toolkit/content/widgets/videocontrols.xml:1108
(Diff revision 15)
>            this.fullscreenButton.removeAttribute("fullscreened");
>          }
>        },
>  
> -      onFullscreenChange() {
> -        this.updateOrientationState(this.isVideoInFullScreen());
> +      // XXX This should be the place where we update the control states and
> +      // screen orientation upon enter/leave fullscreen.

Nit: upon entering/leaving

::: toolkit/content/widgets/videocontrols.xml:1111
(Diff revision 15)
> -        }
> -        this.setFullscreenButtonState();
> -      },
> +      // We therefore omit anything here and let the new binding to settle
> +      // in the corrected state from the constructor.
> +      //

Nit: We therefore don't do anything here and leave it to the new binding to set state correctly from its constructor.

::: toolkit/content/widgets/videocontrols.xml:1114
(Diff revision 15)
> +      // onFullscreenChange() {
> +      //   // Constructor and destructor will lock/unlock the orientation exactly
> +      //   // once. Doing so here again will cause the videocontrols to
> +      //   // lock-unlock-lock the orientation when entering the fullscreen.
> +      //   this.updateOrientationState(this.isVideoInFullScreen());
> +      //
> +      //   // This is already broken by bug 718107 (controls will be hidden
> +      //   // as soon as the video enters fullscreen).
> +      //   // We can think about restoring the behavior here once the bug is
> +      //   // fixed, or we could simply acknowledge the current behavior
> +      //   // after-the-fact and try not to fix this.
> +      //   if (this.isVideoInFullScreen()) {
> +      //     Utils._hideControlsTimeout = setTimeout(this._hideControlsFn, this.HIDE_CONTROLS_TIMEOUT_MS);
> +      //   }
> +      //
> +      //   // Constructor will handle this correctly on the new DOM content in
> +      //   // the new binding.
> +      //   this.setFullscreenButtonState();
> +      // },

Thanks for the extensive comments, this is helpful. Personally, I'd use a `/**/` style comment to comment out the function, to avoid the double `// //` but it's up to you...
Attachment #8957737 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8960883 [details]
Bug 1444489 - Part II, Replace touchControls with videoControls and remove touchControls

https://reviewboard.mozilla.org/r/229640/#review235408

> I'm assuming this was just because some of the necessary work here hadn't been implemented in the touch control binding, not because of limitations in where we ship VTT support? That is, have you tested text track support on Android? Do we have any tests that we could now enable on android, maybe, to ensure that things continue working?

That's my assumption too, although I couldn't find anything documented in bug 887934 when it was implemented.

The text tracks works manually and I have enabled applicable tests on this cset.
Thanks for the review. This is now ready to land, pending one final try result:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ac2f1ab651423b1816dc16fb70e6a5c4dedb1a
Comment on attachment 8957764 [details]
Bug 1444489 - Part VI, Enlarge the size of controls on mobile

https://reviewboard.mozilla.org/r/226750/#review235446
Attachment #8957764 - Flags: review+
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/264b7bdb6c6d
Part I, Convert noControls binding to HTML content r=Gijs
https://hg.mozilla.org/integration/autoland/rev/e0c831e58530
Part II, Replace touchControls with videoControls and remove touchControls r=Gijs
https://hg.mozilla.org/integration/autoland/rev/05d8a651e3d4
Part III, Workaround bug 718107 r=Gijs
https://hg.mozilla.org/integration/autoland/rev/2d3ee6cd29b1
Part IV, Implement Casting UI on videoControls r=Gijs
https://hg.mozilla.org/integration/autoland/rev/6bb4c1e01a69
Part V, Remove XUL videocontrols reflow code on Android r=dholbert
https://hg.mozilla.org/integration/autoland/rev/f13695146a00
Part VI, Enlarge the size of controls on mobile r=e7358d9c+590837,Gijs
https://hg.mozilla.org/integration/autoland/rev/0f95b59a3e75
Part VII, nit: Improve naming consistency in videocontrols.xml r=Gijs
https://hg.mozilla.org/integration/autoland/rev/99fc41ec7ce9
Part VIII, Transition the visibility property instead of using transitionend event r=Gijs
Depends on: 1448556
Component: XBL → XUL Widgets
Product: Core → Toolkit
Depends on: 1450013
Depends on: 1474574
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.