1271765 - Visual refresh of media controls

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Justin Dolske [:Dolske]]

We last overhauled the appearance of the video controls in bug 681548. Steven started a mockup a while ago, but it's not complete: http://people.mozilla.org/~shorlander/files/video-controls-i03/videoControls-i02.html

This needs further design work before this is ready to implement.

Comment 2

[Ray Lin[:ralin]]

Attachment: latest nightly build interface.png

The attachment is the latest interface including closed caption button which should consider. Also see Bug 883122 for the possible followup change.

Comment 3

[Peko Chen [:pekochen]]

Hi,

I am the visual designer who responsible for visual refresh.
Just wondering, do we have any ux spec?
I would like to clarify how many functions should I design.
Thanks

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

This is the latest spec that I have, though it is missing the button for WebVTT.

http://people.mozilla.org/~shorlander/files/video-controls-i03/videoControls-i02.html

Comment 6

[Ray Lin[:ralin]]

Attachment: Media Control_01_subtitle.png

Peko has done some design concepts[0] for this bug. We like concept 01 after discussing offline.

Following is her notion of designs:
For concept 01, I place all the actions on one the panel. There is no hidden functions and easily for user to access. The panel's height is tall enough for the controls and icons to leave the most of space for viewing. Using the blue color for the progress state so it’s more clear and stand out. 

About the other two concepts:  
02. Seperate progress bar from the control panel  
Shows complete progress bar and spacious control panel.  

03.Try grey color to see different feeling. 


Feedback are welcome!! Thanks!


[0]https://drive.google.com/drive/u/1/folders/0B4K8q1qWmtAvcG42VVBFYUlaUXM

Comment 7

[Ray Lin[:ralin]]

We got some update about the design, please see below:

- Setting button: Compared between different browsers, we should consider essential features prior to impl setting button.
- Chapter button for VTT(bug 883122): No ETA for now. With a high chance be scheduled after visual refresh. 
- Closed caption button: Present if more than one text track is provided. The length of each text track’s title is variable, but we can define a maximum length and fill the stripped part with ...   Also we should provided different icon/style to tell the distinction between states.
- Error state: Replace with fun icon for lightening up error message instead of original tedious one.
- Video without audio track: Turn mute button into disabled. Also hide volume control. // original design
- Resize: Could not prevent user/developer from resizing video to extreme size. However, we could define the breakpoint to adjust our control bar. Like making minor buttons hidden or shrink the size of timeline, etc.
Audio only: was considering adding background image. But it only appears in edge case, also we fallback to audio controls if no video frame for <video>.

Peko is working on design 01 and about to get icons and spec soon.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

At small sizes, instead of completely hiding buttons can we move them to a gear menu?

Comment 9

[Justin Dolske [:Dolske]]

A couple bits of feedback:

* Your mockups probably just don't show this state, but will the time still be displayed in the thumb when manually  changing the playback position? I'm really hesitant to remove the current-time indicator from the scrubbing thumb, for two main reasons: (1) when you're changing locations, your eye doesn't have to bounce between two locations, and (2) other major video sites (notably YouTube) work this way, and I'd expect they've done some research on it. I do think it's ok to not show it there when the video is just playing normally (ie, the user isn't interacting with the thumb).

* I'm not sure having a slider for the volume is worth the space. We used to have a slider (albeit vertical, and as hover UI) before the current UI (which is much more compact), and that seems to be fine. YouTube has fiddled with this periodically; it used to be similar to our current UI (and other sites), and is now hover UI similar to what we originally had (albeit horizontal instead of vertical).

* The scrubber bar at the top (instead of being embedded in center of the bar) seems to be an increasingly popular style, and feels cleaner, so I like that. :)

* The current controls do make an effort to hide pieces at smaller and smaller sizes, but it's also worth thinking about what the UI should be at _larger_ sizes. (Perhaps specifically full-screen.). For really large videos, a tiny ribbon of controls at the bottom of the screen seems sub-optimal. Although now that I look at YouTube, they seem to just use their normal controls (at a somewhat larger size), so maybe that's an adequate solution to this.

Comment 10

[Peko Chen [:pekochen]]

Hi Justin

Thanks for your feedback.
For remove the current-time indicator,we will show current time and total time in our new design, 
so I think is it not so necessary to show current-time indicator. However if we can a do preview of the hovered position in the video while mouse over on the scrubber that would be great.
ref bug https://bugzilla.mozilla.org/show_bug.cgi?id=787881

For the volume and scrubber bar.
We think it's clear to show and easily to adjust the thumb.
If we hide the volume bar, it might be more complicated and too much steps to adjust the volume.
Also we want to avoid the UI looks too similar to YouTube.

For the large size.Ray is going o help to enlarge the control panel.

Comment 11

[Ray Lin[:ralin]]

Attachment: Bug 1271765 - Part 2: Desktop media control visual refresh.

Review commit: https://reviewboard.mozilla.org/r/72154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72154/

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review69724

::: toolkit/content/widgets/videocontrols.xml:1008
(Diff revision 1)
>                      if (element.classList.contains("controlBar") && fadeIn) {
>                          // Bug 493523, the scrubber doesn't call valueChanged while hidden,
>                          // so our dependent state (eg, timestamp in the thumb) will be stale.
>                          // As a workaround, update it manually when it first becomes unhidden.
>                          if (element.hidden)
> +#ifdef ANDROID

note that the preprocessor has been disabled for this file since bug 1233198. You can work around this by setting a property in the touchCountrols and using that in combination with the touchControlsGonk.

For example,
<binding id=touchCountrols>
<implementation>
<constructor>
this.isTouchControls = true;
</constructor>
</implementation>
</binding>

and then in this code you would do:
if (this.isTouchControls && !this.isGonk) {
}

Comment 13

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/1-2/

Comment 14

[Ray Lin[:ralin]]

Attachment: wip0822-videocontrol screenshot .png

The attachment is the latest screenshot.

few notable updates (or see WIP patch):
1. de-xul desktop video control
2. new SVG button icons
3. style mostly rewrite
4. use anonid instead of class for getAnonymousElement

todo:
1. new error page
2. css reorganization
3. adjust resize breakpoints


feedback is welcome :)

Comment 15

[Ray Lin[:ralin]]

Attachment: vidctrl-elem-width.png

Hi Justin

Peko, Carol and I had discussion about bug 495593. By comparison with Chrome and Safari(as attachment), we found their controls match element's width rather than video region's width. We're considering using the same behavior to make video element feel like a real video player. 

What do you think about it? Could you give me a feedback?

Thanks.

Comment 16

[Justin Dolske [:Dolske]]

(In reply to Peko Chen [:pekochen] from comment #10)

> For remove the current-time indicator,we will show current time and total
> time in our new design, 
> so I think is it not so necessary to show current-time indicator.

I am saddened by this direction, for the reasons I outlined in comment 9. 

> However if
> we can a do preview of the hovered position in the video while mouse over on
> the scrubber that would be great.
> ref bug https://bugzilla.mozilla.org/show_bug.cgi?id=787881

Note that this bug is WONTFIX.


(In reply to Ray Lin[:ralin] from comment #15)

> Peko, Carol and I had discussion about bug 495593. By comparison with Chrome
> and Safari(as attachment), we found their controls match element's width
> rather than video region's width. We're considering using the same behavior
> to make video element feel like a real video player. 
> 
> What do you think about it? Could you give me a feedback?

The behavior implemented by 495593 still seems desirable to me. I'd also generally suggest minimizing the changes not required by what's supposed to just be a visual refresh. That makes things easier to review, as well as track/fix regressions. [On that note, it would be great to get more test coverage of videocontrols.]

Comment 17

[Ray Lin[:ralin]]

Got it. It's hard to define the certain scope of visual refresh, but I think it's reasonable for visual designer to rethink what video control should look like in different situations. I'd like to save the discussion about 495593 to another bug in the future to have a space for designer to come out with which behavior is good for user. 


I hope my current patch(comment 15) doesn't exceed our expectation in visual refresh. One notable but seems to be out of scope is desktop De-XUL. I'm not sure it's proper to De-XUL here, but I could not find a good reason to be impl in XUL and convert to HTML just a while later this bug. 


(In reply to Justin Dolske [:Dolske] from comment #16)

> [On that note, it would be great to get more test coverage of videocontrols.]
np. I was trying to have some plan to make video control better and test coverage is one of the most important. 

Thanks for your feedback.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Ray Lin[:ralin] from comment #17)
> Got it. It's hard to define the certain scope of visual refresh, but I think
> it's reasonable for visual designer to rethink what video control should
> look like in different situations. I'd like to save the discussion about
> 495593 to another bug in the future to have a space for designer to come out
> with which behavior is good for user. 

Unfortunately I don't think a patch can land (and be enabled) without a decision made here. How can we make a decision here?

> I hope my current patch(comment 15) doesn't exceed our expectation in visual
> refresh. 
> One notable but seems to be out of scope is desktop De-XUL. I'm not
> sure it's proper to De-XUL here, but I could not find a good reason to be
> impl in XUL and convert to HTML just a while later this bug. 

HTML is fine. It make no sense to implement this in XUL.

> np. I was trying to have some plan to make video control better and test
> coverage is one of the most important. 

Yeah Ray and I did talk about this a few weeks ago. If that's not the scope of this bug already, we should create new bug(s) as placeholder of this work (and work on it in the future).

Comment 19

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/2-3/

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review72934

::: layout/generic/nsVideoFrame.cpp:358
(Diff revisions 1 - 3)
>          nsContentUtils::AddScriptRunner(event);
>        }
>  #endif
>      } else if (child->GetContent() == mCaptionDiv ||
>          child->GetContent() == mVideoControls) {
> -      // Reflow to caption div
> +      // Reflow to caption div and video controls frame

// Reflow the caption and control bar frames.

::: toolkit/content/widgets/videocontrols.xml
(Diff revisions 1 - 3)
>            this.setAttribute("showhours", val);
>            // If the duration becomes known while we're still showing the value
>            // for time=0, immediately update the value to show or hide the hours.
>            // It's less intrusive to do it now than when the user clicks play and
>            // is looking right next to the thumb.
> -          if (!this.timeLabel) return;

Why are you deleting the early return if timeLabel is undefined here?

::: toolkit/content/widgets/videocontrols.xml:133
(Diff revisions 1 - 3)
> -                    var percent = newValue / this.max;
> +                    let percent = newValue / this.max;
> -                    if (!isNaN(percent) && percent != Infinity) {
> -                      this.valueBar.value = Math.round(percent * 10000); // has max=10000
> +                    this.valueBar.value = Math.round(percent * 10000); // has max=10000
> -                    } else {
> -                      this.valueBar.removeAttribute("value");
> -                    }

I think you should leave this unchanged. Videos that are live streamed won't have a max so we shouldn't show a value.

::: toolkit/content/widgets/videocontrols.xml:451
(Diff revisions 1 - 3)
> +                    // can not get minimal width of flexible scrubber and clickToPlay
> +                    // at this moment. rewrite to a empirical value

// XXX Cannot get minimal width of flexible scrubber and clickToPlay. Rewrite to empirical value for now.

::: toolkit/content/widgets/videocontrols.xml:840
(Diff revisions 1 - 3)
>                      this.isPausedByDragging = false;
>                    }
>                  },
>  
>                  updateScrubberProgress() {
> +                  if (this.videocontrols.isGonk || this.videocontrols.isTouchControl) {

Any place where you have `if (isGonk || isTouchControls)` can be simplified to `if (isTouchControls)` since the Gonk controls extend the touch controls.

::: toolkit/content/widgets/videocontrols.xml:1030
(Diff revisions 1 - 3)
>                          // Keep the controls visible if the click-to-play is visible.
>                          if (!this.clickToPlay.hidden)
>                              return;
>  
>                          this.startFadeOut(this.controlBar, false);
> -                        this.textTrackList.setAttribute("hidden", "true");
> +                        this.textTrackList.hidden = true;

I'm not sure if you knew this, but we were using `.setAttribute("hidden", "true")` throughout this file instead of `.hidden=true` because the .hidden property requires the binding to be applied whereas the .setAttribute() function can run without the binding applied and will have the same effect.

Please revert these back to using .setAttribute() unless you can prove that these will run with the binding attached. You can use `hg blame` to find which bug changed to use .setAttribute if you are curious.

::: toolkit/content/widgets/videocontrols.xml:1090
(Diff revisions 1 - 3)
>  
>                      // Nothing to do when a fade *in* finishes.
>                      if (!element.hasAttribute("fadeout"))
>                          return;
>  
> -                    if (this.videocontrols.isTouchControl) {
> +                    if (this.videocontrols.isTouchControl || this.videocontrols.isGonk) {

This change is not necessary.

::: toolkit/content/widgets/videocontrols.xml:1353
(Diff revisions 1 - 3)
> +                        !this.videocontrols.isTouchControl &&
> +                        !this.videocontrols.isGonk;

You can remove the isGonk check here because it is already covered by isTouchControl

::: toolkit/content/widgets/videocontrols.xml:1543
(Diff revisions 1 - 3)
> +                    return;
> +                  }
>  
> -                    // The scrubber has |flex=1|, therefore |minScrubberWidth|
> -                    // was generated by empirical testing.
> -                    let minScrubberWidth = 25;
> +                  const videoWidth = this.video.clientWidth;
> +                  const videoHeight = this.video.clientHeight;
> +                  const minVideoSideLength = Math.min(videoWidth, videoHeight);

Please move this declaration closer to its first use.

::: toolkit/content/widgets/videocontrols.xml:1570
(Diff revisions 1 - 3)
> +                    this.volumeStack.isWanted = false;
> -                    }
> +                  }
> -                    */
>  
> -                    if ((this._overlayPlayButtonHeight + this._controlBarHeight) > videoHeight ||
> -                        this._overlayPlayButtonWidth > videoWidth) {
> +                  let widthUsed = minControlBarPaddingWidth;
> +                  let preventAppendControl = false;

`preventAppendControl` is unnecessary here. The check for `widthUsed` less than `videoWidth` is sufficient here.

::: toolkit/content/widgets/videocontrols.xml:1601
(Diff revisions 1 - 3)
> +                  // as long as muteButton hidden, which means only play button presents.
> +                  // hide spacer and make playButton centered.
> +                  this.controlBarSpacer.hidden = !this.scrubberStack.hidden || this.muteButton.hidden;
> +
> +                  // adjust clickToPlayButton size
> +									const clickToPlayScaledSize = Math.max(this.clickToPlay.minWidth,

nit, convert tabs to spaces here

::: toolkit/content/widgets/videocontrols.xml:1604
(Diff revisions 1 - 3)
> +                  if (clickToPlayScaledSize >= videoWidth ||
> +                    (clickToPlayScaledSize + this.controlBar.minHeight / 2 > videoHeight / 2 )) {

Both left and right sides of the comparison are dividing by two so the division of two can be factored out.

Create a local variable for `clickToPlayScaledSize + this.controlBar.minHeight` and line up the if-condition.

Also, why is the width comparison using `>=` but the height comparison is using `>` ?

let clickToPlayAndControlBarHeight = clickToPlayScaledSize + this.controlBar.minHeight;
if (clickToPlayScaledSize >= videoWidth ||
    (clickToPlayAndControlBarHeight >= videoHeight)) {

::: toolkit/themes/shared/media/videocontrols.css
(Diff revisions 1 - 3)
> -  height: 100%;
> -  width: 100%;
>    left: 0;
>    bottom: 0;
> +  width: 100%;
> +  height: 100%;

Please keep these in unchanged order to preserve mercurial history.

::: toolkit/themes/shared/media/videocontrols.css:41
(Diff revisions 1 - 3)
> +.statusOverlay {
>    display: flex;
>    flex-direction: column;
>    justify-content: center;
>    align-items: center;
> -  background-color: rgba(0,0,0,.55);
> +  background-color: #505050;

We should keep this translucent so the video frame behind it can be seen, even if just slightly.

::: toolkit/themes/shared/media/videocontrols.css:78
(Diff revisions 1 - 3)
> +  height: 100%;
> +  min-height: 30px;
> +  min-width: 30px;

Why does height:100% need to be set here?

::: toolkit/themes/shared/media/videocontrols.css:129
(Diff revisions 1 - 3)
> +.muteButton[muted]:active {
> +  background-image: url(chrome://global/skin/media/muteButton.svg#mute-active);
>  }
> -
>  .muteButton[noAudio] {
> +  background-image: url(chrome://global/skin/media/muteButton.svg#mute);

We should not be showing the "muted" icon when there is no audio.

In fact, what is currently named the "muted" icon is actually the icon that we were using for "noAudio" before. The "muted" icon should be the speaker with no sound waves coming from it.

::: toolkit/themes/shared/media/videocontrols.css:223
(Diff revisions 1 - 3)
> -  width: 100%;
> -  height: 100%;
> +.volumeBackground,
> +.volumeControl {
> -  position: absolute;
>    bottom: 0;
>    left: 0;
> -  border: none;
> +  position: absolute;
> +  width: 100%;
> +  height: 100%;
>    padding: 0;
> +  border: 0;
> +  border-radius: 2.5px;
>    margin: 0;
>    background-color: transparent;
> -  border-radius: 2.5px;

Please keep these in original order to preserve mercurial history.

::: toolkit/themes/shared/media/videocontrols.css:327
(Diff revisions 1 - 3)
> -.durationBox {
> +.labelBox {
>    white-space: nowrap;
>    font-size: 13px;

This should use `font: message-box;` and it should be defined before `font-size: 13px;`.

::: toolkit/themes/shared/media/videocontrols.css:337
(Diff revisions 1 - 3)
> +.durationBox {
> +  padding-right: 8px;

This is not RTL friendly. Please use padding-inline-end instead.

::: toolkit/themes/shared/media/videocontrols.css:343
(Diff revisions 1 - 3)
> +  padding-right: 8px;
> +}
> +
>  .durationLabel,
>  .durationLabel::before {
>    color: #929292;

This color is not enough contrast with the background behind it. I'm not sure why we wouldn't use the same color here as the positionLabel.

::: toolkit/themes/shared/media/videocontrols.css:346
(Diff revisions 1 - 3)
>  .durationLabel,
>  .durationLabel::before {
>    color: #929292;
>  }
>  .durationLabel::before {
>    content: "/ ";

We should not be using generated content for text that people will be reading.

This isn't localizer friendly nor screen reader friendly.

::: toolkit/themes/shared/media/videocontrols.css:407
(Diff revisions 1 - 3)
>  .clickToPlay[fadeout] {
> -  background-size: auto, 192px 192px;
> +  transform: scale(1.4);
>    opacity: 0;

This transform is pretty hard to notice. I think it's because the scale is still pretty close to 1.

Also, the math above uses .15 for the clickToPlayViewRatio. Shouldn't that include the 1.4 transform scale too to know if the animation will fit within the video?

Comment 21

[Peko Chen [:pekochen]]

Please see the link as below for the latest visual spec
Thanks

https://drive.google.com/open?id=0B4K8q1qWmtAvM3ZXVUlHRkdRVVU

Comment 22

[Peko Chen [:pekochen]]

For control match the width of element rather than the  video region.
We asked Stephen's opinion,
he also agree that the video control set will be fixed with the window length 
while you resize the video is the nicer behavior.

So currently we are still keep the original behavior and will create a new bug in the future.

Comment 23

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/3-4/

Comment 24

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review72934

Thanks for the review. learned a lot :)

> `preventAppendControl` is unnecessary here. The check for `widthUsed` less than `videoWidth` is sufficient here.

To make sure the layout in control, this flag here is intended. I don't want to break the loop but need to stop adding control once a higher priority control could not fit in. That means, if the available width can not fulfill scrubberStack, we just skip durationBox and volumeStack even unused space is enough for them.

> Please keep these in unchanged order to preserve mercurial history.

I don't know if this affects since the diff revisions is 1 - 3 :)

> We should keep this translucent so the video frame behind it can be seen, even if just slightly.

Here Peko hopes to preserve solid color if possible. Set to 0.99 in new revision.

> Why does height:100% need to be set here?

The controlBar's height is 40px, I think the region under/over buttons should be clickable as well.

> We should not be showing the "muted" icon when there is no audio.
> 
> In fact, what is currently named the "muted" icon is actually the icon that we were using for "noAudio" before. The "muted" icon should be the speaker with no sound waves coming from it.

Peko provided new mute button and noAudio button. Now we got more consistent style icon and state for noAudio.

> This color is not enough contrast with the background behind it. I'm not sure why we wouldn't use the same color here as the positionLabel.

this design is on purpose to make differences between two labels. Impl according to spec (see attachment).

> This transform is pretty hard to notice. I think it's because the scale is still pretty close to 1.
> 
> Also, the math above uses .15 for the clickToPlayViewRatio. Shouldn't that include the 1.4 transform scale too to know if the animation will fit within the video?

hideClickToPlay function revised. Now scale is set to 3.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review73674

Looking better, still need to make some small changes but it's getting close.

We should make the scrubber thumb blue on hover to match the other controls.

::: toolkit/content/widgets/videocontrols.xml:130
(Diff revision 4)
>                      // Update the time shown in the thumb.
>                      this.thumb.setTime(newValue);

If you're removing the time from the thumb then this should be removed, right?

::: toolkit/content/widgets/videocontrols.xml:216
(Diff revision 4)
>                  <label class="errorLabel" anonid="errorAborted">&error.aborted;</label>
>                  <label class="errorLabel" anonid="errorNetwork">&error.network;</label>
>                  <label class="errorLabel" anonid="errorDecode">&error.decode;</label>
>                  <label class="errorLabel" anonid="errorSrcNotSupported">&error.srcNotSupported;</label>
>                  <label class="errorLabel" anonid="errorNoSource">&error.noSource2;</label>
>                  <label class="errorLabel" anonid="errorGeneric">&error.generic;</label>

These should be <span> instead of <label> since <xul:label> has much different semantics than <html:label>

::: toolkit/content/widgets/videocontrols.xml:257
(Diff revision 4)
> -                      <scale class="volumeControl" movetoclick="true"/>
> -                    </stack>
> +                          <progress anonid="volumeBackground" class="volumeBackground"
> +                                    min="0" value="0" max="100">

This should use <html:meter> as that is the correct semantic element here.

::: toolkit/content/widgets/videocontrols.xml:803
(Diff revision 4)
>  
>                      if (isNaN(duration) || isInfinite)
>                          duration = this.maxCurrentTimeSeen;
>  
>                      // Format the duration as "h:mm:ss" or "m:ss"
> -                    let timeString = isInfinite ? "" : this.formatTime(duration);
> +                    let timeString = isInfinite ? "" : "/ " + this.formatTime(duration);

This is still not localizer friendly and there is not equal spacing on both sides of the slash.

You should create a string in videocontrols.dtd like how is done for &scrubberScale.nameFormat;

<!-- LOCALIZATION NOTE (positionAndDuration.nameFormat): the #1 string is the current
media position, and the #2 string is the total duration. For example, when at
the 5 minute mark in a 6 hour long video, #1 would be "5:00" and #2 would be
"6:00:00", result string would be "5:00 / 6:00:00".
-->
<!ENTITY positionAndDuration.nameFormat "#1 / #2">

::: toolkit/content/widgets/videocontrols.xml:1618
(Diff revision 4)
> +                  // use flexible spacer to separate controls when scrubber is hidden
> +                  // as long as muteButton hidden, which means only play button presents.
> +                  // hide spacer and make playButton centered.

Nit, please use sentence case with a period at the end for all of the comments.

::: toolkit/content/widgets/videocontrols.xml:1626
(Diff revision 4)
> +                  this.controlBarSpacer.hidden = !this.scrubberStack.hidden || this.muteButton.hidden;
> +
> +                  // adjust clickToPlayButton size
> +                  const minVideoSideLength = Math.min(videoWidth, videoHeight);
> +                  const clickToPlayViewRatio = 0.15;
> +									const clickToPlayScaledSize = Math.max(this.clickToPlay.minWidth,

These tabs still need to be converted to spaces.

::: toolkit/themes/shared/media/videocontrols.css:41
(Diff revision 4)
> +.statusOverlay {
> +  display: flex;
> +  flex-direction: column;
> +  justify-content: center;
> +  align-items: center;
> +  background-color: rgba(80, 80, 80, 0.99);

Sorry, I was thinking of the .controlsSpacer when I left the comment on the previous version. This should change back to `background-color: rgb(80,80,80);`, and note that there should be no spaces after the commas inside of the color functions.

::: toolkit/themes/shared/media/videocontrols.css:141
(Diff revision 4)
> -  background-image: url(chrome://global/skin/media/closeCaptionButton.png);
> -  background-position: 4px;
> +  background-image: url(chrome://global/skin/media/closedCaptionButton.svg#cc-off);
> +}
> +.closedCaptionButton:hover {
> +  background-image: url(chrome://global/skin/media/closedCaptionButton.svg#cc-off-hover);
> +}
> +.closedCaptionButton:active {

All of these buttons that use :active need to use :hover with them, for example:
.closedCaptionButton:hover:active

::: toolkit/themes/shared/media/videocontrols.css:334
(Diff revision 4)
> -  font-size: 10px;
> -  text-shadow: rgba(0,0,0,.3) 0 1px;
> +  font-size: 13px;
> +  padding: 1px;
> -  padding-top: 7px;
>  }
>  
> -%ifdef XP_MACOSX
> +.positionLabel {

The position and duration need to have `font: message-box;`

::: toolkit/themes/shared/media/videocontrols.css:437
(Diff revision 4)
> -    1px -1px 0 #000,
> -    -1px 1px 0 #000,
> -    1px 1px 0 #000;
>    padding: 0 10px;
>    text-align: center;
> +  font-family: Helvetica, Arial, sans-serif;

This needs to be `font: message-box;`

Comment 26

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review73674

Thanks :)

Now the color of scrubber thumb turn into blue when hover.

> This is still not localizer friendly and there is not equal spacing on both sides of the slash.
> 
> You should create a string in videocontrols.dtd like how is done for &scrubberScale.nameFormat;
> 
> <!-- LOCALIZATION NOTE (positionAndDuration.nameFormat): the #1 string is the current
> media position, and the #2 string is the total duration. For example, when at
> the 5 minute mark in a 6 hour long video, #1 would be "5:00" and #2 would be
> "6:00:00", result string would be "5:00 / 6:00:00".
> -->
> <!ENTITY positionAndDuration.nameFormat "#1 / #2">

Thanks for giving me this example.

Use format from locale in new revision, but I'm not sure whether I'm using it right.
Should we update duration and position at same time? Since we split the them into showDuration and showPosition and update separatly, it's hard to generate string in correct format if localizer re-order #1 and #2 or add add string at the end or at the begin.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review73674

> Thanks for giving me this example.
> 
> Use format from locale in new revision, but I'm not sure whether I'm using it right.
> Should we update duration and position at same time? Since we split the them into showDuration and showPosition and update separatly, it's hard to generate string in correct format if localizer re-order #1 and #2 or add add string at the end or at the begin.

We should make the position and duration one string and update it each time the position or duration changes. You can embed <span> tags in to the DTD text and add a localization note to surround the duration with the spans. For example:
<!ENTITY positionAndDuration.nameFormat "#1 <span class='remainder'>/ #2</span>">

Comment 28

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review73674

> We should make the position and duration one string and update it each time the position or duration changes. You can embed <span> tags in to the DTD text and add a localization note to surround the duration with the spans. For example:
> <!ENTITY positionAndDuration.nameFormat "#1 <span class='remainder'>/ #2</span>">

I was thinking what kinds of format it will be and how it fit in new style (half white half grey). 

A few couple considerations about making position and duration one string (or l10n):
1. #2 would be unavailable while streaming or server doesn't provide the information. Localizer need to know more than just translatation. 
2. new style conceptually split string into two parts. 
3. need hacks to avoid affecting Fennec if we change the way update position and duration labels.  

Got a offline discussion with Peko. It seems to us that being splited by "/" is normal and widely use format in different video player, so we think this issue might not be critical to be a block to landing. 

Sorry for making this opinion so late and please feel free to correct me if the intention here were minunderstood by me. 

Thanks :)

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

A couple rebuttals here,

1. If we don't know the duration, then .remainder can be set to display:none;
2. The styles here can still be split visually into two parts with different colors.
3. What you are describing is not really a hack but just differences in the two bindings. There are other differences between the bindings that we already implement. This would not be much different.

Comment 30

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review72934

> Both left and right sides of the comparison are dividing by two so the division of two can be factored out.
> 
> Create a local variable for `clickToPlayScaledSize + this.controlBar.minHeight` and line up the if-condition.
> 
> Also, why is the width comparison using `>=` but the height comparison is using `>` ?
> 
> let clickToPlayAndControlBarHeight = clickToPlayScaledSize + this.controlBar.minHeight;
> if (clickToPlayScaledSize >= videoWidth ||
>     (clickToPlayAndControlBarHeight >= videoHeight)) {

Sorry for the late reply to this issue.

Currently the clickToPlay is on the center of video element rather than controlsSpacer.

Comment 31

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/4-5/

Comment 32

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/5-6/

Comment 33

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review73674

> If you're removing the time from the thumb then this should be removed, right?

Add back in diff 6. Mobile still uses scrubber binding, and position label's value come from thumb(next line).
This could be removed in mobile de-xul process

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review75182

::: toolkit/content/widgets/videocontrols.xml:229
(Diff revisions 4 - 6)
>              <div anonid="controlsOverlay" class="controlsOverlay stackItem">
>                  <div class="controlsSpacerStack">
>                    <div anonid="controlsSpacer" class="controlsSpacer stackItem"></div>
>                    <div anonid="clickToPlay" class="clickToPlay" hidden="true"></div>
>                  </div>
>                  <div anonid="controlBar" class="controlBar desktopControl" hidden="true">

The desktopControl class seems unnecesary. You should just check this.isTouchControl instead.

Would it be sufficient to just write:

> if (!this.videocontrols.isTouchControl) {
>  ...
> } else {
>  ...
> }

instead of:

> if (!this.controlBar.classList.contains("desktopControl")) {
>  ...
> } else {
>  ...
> }

Also, while you are here please rename isTouchControl to isTouchControls.

::: toolkit/content/widgets/videocontrols.xml:238
(Diff revisions 4 - 6)
>                              pauselabel="&playButton.pauseLabel;"/>
>                      <div anonid="scrubberStack" class="scrubberStack progressContainer">
>                        <div class="progressBackgroundBar stackItem">
>                          <div class="progressStack">
> -                          <progress anonid="bufferBar" class="bufferBar"></progress>
> -                          <progress anonid="progressBar" class="progressBar" value="0" max="100"></progress>
> +                          <meter anonid="bufferBar" class="bufferBar"></meter>
> +                          <meter anonid="progressBar" class="progressBar" value="0" max="100"></meter>

These were fine as <progress> because they show how far the viewer has watched in the video (progression).

The volume slider should be a meter because it doesn't have a natural direction to it.

::: toolkit/content/widgets/videocontrols.xml:467
(Diff revisions 4 - 6)
> +                              control.setAttribute("hidden", "true");
> +                            } else {
> +                              control.removeAttribute("hidden");
> +                            }
> +
> +                            control._isHideByAdjustment= !!v;

The name should be changed to "\_isHidden" instead of "\_isHide" and the lack of space before the equals sign should cause an ESLint error.

You can run \`mach lint toolkit/content/widgets/videocontrols.xml\` to see if there are any static analysis errors.

control.\_isHiddenByAdjustment = \!\!v;

::: toolkit/content/widgets/videocontrols.xml:817
(Diff revisions 4 - 6)
> +                initPositionDurationBox : function () {
> +                  if (!this.controlBar.classList.contains("desktopControl")) {
> +                    return;
> +                  }
> +
> +                  const positionTextNode = Array.prototype.find.call(
> +                    this.positionDurationBox.childNodes, (n) => !!~n.textContent.search("#1"));
> +                  const durationAndRemainder = this.durationAndRemainder;
> +                  const remainderFormat = durationAndRemainder.textContent;
> +                  const positionFormat = positionTextNode.textContent;
> +
> +                  Object.defineProperties(this.positionDurationBox, {
> +                    durationAndRemainder: {
> +                      value: durationAndRemainder
> +                    },
> +                    position: {
> +                      set: (v) => {
> +                        positionTextNode.textContent = positionFormat.replace("#1", v);
> +                      }
> +                    },
> +                    duration: {
> +                      set: (v) => {
> +                        if (!v) {
> +                          durationAndRemainder.setAttribute("hidden", "true");
> +                        } else {
> +                          durationAndRemainder.textContent = remainderFormat.replace("#2", v);
> +
> +                          if (!durationAndRemainder.hideByAdjustment) {
> +                            durationAndRemainder.removeAttribute("hidden");
> +                          }
> +                        }
> +                        durationAndRemainder.isWanted = !!v;
> +                      }
> +                    }
> +                  });
> +                },

You should not be searching the locale text for #1 and #2.

You should create a property the same way that scrubberNameFormat is created, and then use String.replace to generate the new string. For example:

> this.positionAndDurationFormat = ]]>"&positionAndDurationFormat.formatString;"<!CDATA[;
> let positionAndDurationString = this.positionAndDurationFormat.replace(/#1/, position).replace(/#2/, duration);

Then you can insert that string in to the textContent of the box.

Also, we should not be removing the hidden attribute here because we don't know if there will be room for the new value. Anything that hides or shows the controls should be done in the same area of code.

::: toolkit/content/widgets/videocontrols.xml:951
(Diff revisions 4 - 6)
> -                    if (this.videocontrols.isTouchControl) {
> -                      return;
> +                    if (this.controlBar.classList.contains("desktopControl")) {
> +                      this.positionDurationBox.position = positionTime;
> -                    }
> -                    this.updateScrubberProgress();
> +                      this.updateScrubberProgress();
> +                    } else {
> +                      this.positionLabel.setAttribute("value", positionTime);
> +                    }

positionLabel and durationLabel should get hidden if this is not touch controls.

::: toolkit/content/widgets/videocontrols.xml:1005
(Diff revisions 4 - 6)
>                          endTime = Math.round(buffered.end(index) * 1000);
>                      }
>                      this.bufferBar.max = duration;
>                      this.bufferBar.value = endTime;
>  
> -                    if (Math.round(endTime / duration * 100) !== 100) {
> +                    if (Math.ceil(endTime / duration * 100) === 100 || duration === endTime) {

You can factor out the 100 here.

if (Math.ceil(endTime / duration) == 1 || duration == endTime) {
 ...
} else {
 ...
}

::: toolkit/content/widgets/videocontrols.xml:1664
(Diff revisions 4 - 6)
>                      if (!control.isWanted) {
> -                      control.setAttribute("hidden", "true");
> +                      control.hideByAdjustment = true;
>                        continue;
>                      }
>  
> -                    if (widthUsed + control.minWidth < videoWidth && !preventAppendControl) {
> +                    control.hideByAdjustment = (preventAppendControl || widthUsed + control.minWidth > videoWidth);

Please remove the parentheses here as they aren't necessary, and move the second half to a new line.

control.hiddenByAdjustment = preventAppendControl ||
                             widthUsed + controlUsed > videoWidth;

::: toolkit/content/widgets/videocontrols.xml:1673
(Diff revisions 4 - 6)
> -                  // hide control bar
> +                  // Hide control bar if cannot fullfill the minimal size.
>                    if (videoHeight < this.controlBar.minHeight ||
>                        widthUsed === minControlBarPaddingWidth) {

This code here seems self-explanatory enough to not need this comment.

::: toolkit/content/widgets/videocontrols.xml:1780
(Diff revisions 4 - 6)
> +                      addListener(this.scrubber, "input", this.onScrubberInput);
> +                      addListener(this.scrubber, "change", this.onScrubberChange);
> +                      addListener(this.volumeControl, "input", this.updateVolume);

Why do we need to listen for the "input" event here? The "change" event should be sufficient for both the scrubber and volumeControl.

::: toolkit/themes/shared/media/videocontrols.css:336
(Diff revisions 4 - 6)
> -.labelBox {
> +.timeLabel {
> +  display: none;
> +}

It looks like .timeLabel is always display:none. Why do we need it if it is never displayed or made available to screen readers?

::: toolkit/locales/en-US/chrome/global/videocontrols.dtd:49
(Diff revision 6)
> +"6:00:00", result string would be "5:00 / 6:00:00".
> +Note that #2 is not always avaiable. For example, when at the 5 minute mark in an
> +unknown duration video, #1 would be "5:00" and string which is surrounded by <span>
> +would be deleted, result string would be "5:00".
> +-->
> +<!ENTITY positionAndDuration.nameFormat "#1<span class='durationAndRemainder'> / #2</span>">

The class for the span here should just be "duration". Duration and remainder are two similar concepts but we only use duration. Remainder would imply a value that decreases as the playing of the media moves forward.

Comment 35

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review75182

> The desktopControl class seems unnecesary. You should just check this.isTouchControl instead.
> 
> Would it be sufficient to just write:
> 
> > if (!this.videocontrols.isTouchControl) {
> >  ...
> > } else {
> >  ...
> > }
> 
> instead of:
> 
> > if (!this.controlBar.classList.contains("desktopControl")) {
> >  ...
> > } else {
> >  ...
> > }
> 
> Also, while you are here please rename isTouchControl to isTouchControls.

I changed to this way was because showDuration and showPosition are called at setupInitialState(), which can not determine isTouchControl at that time.
Fennec would be crashed by it.

> You should not be searching the locale text for #1 and #2.
> 
> You should create a property the same way that scrubberNameFormat is created, and then use String.replace to generate the new string. For example:
> 
> > this.positionAndDurationFormat = ]]>"&positionAndDurationFormat.formatString;"<!CDATA[;
> > let positionAndDurationString = this.positionAndDurationFormat.replace(/#1/, position).replace(/#2/, duration);
> 
> Then you can insert that string in to the textContent of the box.
> 
> Also, we should not be removing the hidden attribute here because we don't know if there will be room for the new value. Anything that hides or shows the controls should be done in the same area of code.

For the reason why searching the locale text is that we don't know the exact string will look like after localization. It could be something like  "XXX <span>#2 ###</span> #1 ###", then the second textNode is the only place need to be changed when position update. 

It seems the locale string need other method like innerHTML to parse HTML(<span> part). Also to consider the performace and the frequency of update, especially "timeupdate", I think the cost of replacing textNode might be less than remove chidren under box and insert new nodes either by .textContent or .innerHTML.

I agree we should put hides and shows in the same area. Instead of hiding it, replace textConent with emtpy string and update its minWidth would be enough.

> Why do we need to listen for the "input" event here? The "change" event should be sufficient for both the scrubber and volumeControl.

I think it because "change" event only triggered when mouse up. We should make muteButton and positionLabel aware of change while dragging.

> It looks like .timeLabel is always display:none. Why do we need it if it is never displayed or made available to screen readers?

.timeLabel is shared class of positionLabel and durationLabel.

I think I should change to like this, which is more clear than .timeLabel

.positionLabel,
.durationLabel {
  display: none;
}

Comment 36

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review75182

> For the reason why searching the locale text is that we don't know the exact string will look like after localization. It could be something like  "XXX <span>#2 ###</span> #1 ###", then the second textNode is the only place need to be changed when position update. 
> 
> It seems the locale string need other method like innerHTML to parse HTML(<span> part). Also to consider the performace and the frequency of update, especially "timeupdate", I think the cost of replacing textNode might be less than remove chidren under box and insert new nodes either by .textContent or .innerHTML.
> 
> I agree we should put hides and shows in the same area. Instead of hiding it, replace textConent with emtpy string and update its minWidth would be enough.

correct: `It could be something like  "XXX <span>#2 XXX</span> #1 XXX"`

Comment 37

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/6-7/

Comment 38

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review75182

> These were fine as <progress> because they show how far the viewer has watched in the video (progression).
> 
> The volume slider should be a meter because it doesn't have a natural direction to it.

Got that, I didn't know there's subtle difference until now. It's really matter if we want to deliver meaningful and declarative markup.

> The name should be changed to "\_isHidden" instead of "\_isHide" and the lack of space before the equals sign should cause an ESLint error.
> 
> You can run \`mach lint toolkit/content/widgets/videocontrols.xml\` to see if there are any static analysis errors.
> 
> control.\_isHiddenByAdjustment = \!\!v;

Appreciate it! Very useful command :)


btw, I created a web page for testing video size: http://people.mozilla.org/~ralin/vtt/

Comment 39

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ray Lin[:ralin] from comment #35)
> > Also, while you are here please rename isTouchControl to isTouchControls.
> 
> I changed to this way was because showDuration and showPosition are called
> at setupInitialState(), which can not determine isTouchControl at that time.
> Fennec would be crashed by it.

this.isTouchControl is set to true in the #touchControls constructor. Can you try moving this to a <field>? I think that code will run sooner. <field name="isTouchControls">true</field>

Comment 40

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ray Lin[:ralin] from comment #35)
> > Why do we need to listen for the "input" event here? The "change" event should be sufficient for both the scrubber and volumeControl.
> 
> I think it because "change" event only triggered when mouse up. We should
> make muteButton and positionLabel aware of change while dragging.

Okay this is fine then, I was mistaken.

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review75558

I want to wait to review this until you have tried my suggestion in comment #39.

Comment 42

[Ray Lin[:ralin]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #41)
> Comment on attachment 8781828 [details]
> Bug 1271765 - Desktop video control visual refresh.
> 
> https://reviewboard.mozilla.org/r/72154/#review75558
> 
> I want to wait to review this until you have tried my suggestion in comment
> #39.

Sorry for that. I didn't have testable devices in hand, so could not test right after receiving your review.
---
I've tried <field> in video control today, but no matter I change the order or name attribute of <field>, I cannot access the value from "this" context in constructor (always return undefined). And then I test on a local XBL file with same approach, it works.
I can not figure out how come the behaviors are different. Is this issue related to same problem mentioned in the comment[0]? 

[0] https://dxr.mozilla.org/mozilla-central/rev/ab70808cd4b6c6ad9a57a9f71cfa495fcea0aecd/toolkit/content/widgets/videocontrols.xml#1613-1616

Comment 43

[Jared Wein [:jaws] (please needinfo? me)]

I didn't think that this would have the same problem as randomID because randomID needs to be the same ID each time the binding is re-attached whereas this only needs to be set to true each time which will be the same every time the binding is attached.

But it seems the problem is that the base binding refers to the value during its constructor, but that value isn't set until later when the derived binding's constructor has run. We could let the constructor initialize state variables like these and then run the initialization code in a followup task via setTimeout(this.videocontrols.init, 0);

Please file a follow-up bug to perform this refactoring and remove the desktopControl class.

Comment 44

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review75996

This is very close to r+, I just noticed two things when testing this out.

The volume thumb should turn blue when the slider is hovered, the same way that the progress thumb turns blue.

When using your video test page, http://people.mozilla.org/~ralin/vtt/, I couldn't see the buffered progress bar while the video was playing. Once I clicked on the progress bar or moved the playback position using the keyboard Left or Right arrows, the buffered progress bar became fully filled in.

Comment 45

[Jared Wein [:jaws] (please needinfo? me)]

I had to make the following changes to fix the bufferBar,

> diff --git a/toolkit/content/widgets/videocontrols.xml b/toolkit/content/widgets/videocontrols.xml
> --- a/toolkit/content/widgets/videocontrols.xml
> +++ b/toolkit/content/widgets/videocontrols.xml
> @@ -229,17 +229,17 @@
>                  <div anonid="controlBar" class="controlBar desktopControl" hidden="true">
>                      <button anonid="playButton"
>                              class="playButton"
>                              playlabel="&playButton.playLabel;"
>                              pauselabel="&playButton.pauseLabel;"/>
>                      <div anonid="scrubberStack" class="scrubberStack progressContainer">
>                        <div class="progressBackgroundBar stackItem">
>                          <div class="progressStack">
> -                          <meter anonid="bufferBar" class="bufferBar"></meter>
> +                          <progress anonid="bufferBar" class="bufferBar" value="0" max="100"></progress>
>                            <progress anonid="progressBar" class="progressBar" value="0" max="100"></progress>
>                          </div>
>                        </div>
>                        <input type="range" anonid="scrubber" class="scrubber"/>
>                      </div>
>                      <span anonid="positionLabel" class="positionLabel" role="presentation"></span>
>                      <span anonid="durationLabel" class="durationLabel" role="presentation"></span>
>                      <div anonid="positionDurationBox" class="positionDurationBox">
> @@ -654,16 +654,17 @@
>                                  this.setupStatusFader();
> 
>                              // If the user is dragging the scrubber ignore the delayed seek
>                              // responses (don't yank the thumb away from the user)
>                              if (this.scrubber.isDragging)
>                                  return;
> 
>                              this.showPosition(currentTime, duration);
> +                            this.showBuffered();
>                              break;
>                          case "emptied":
>                              this.bufferBar.value = 0;
>                              this.showPosition(0, 0);
>                              break;
>                          case "seeking":
>                              this.showBuffered();
>                              this.statusIcon.setAttribute("type", "throbber");
> @@ -987,22 +988,16 @@
>                      var buffered = this.video.buffered;
>                      var index = bsearch(buffered, currentTime, bufferedCompare);
>                      var endTime = 0;
>                      if (index >= 0) {
>                          endTime = Math.round(buffered.end(index) * 1000);
>                      }
>                      this.bufferBar.max = duration;
>                      this.bufferBar.value = endTime;
> -
> -                    if (Math.ceil(endTime / duration) == 1 || duration == endTime) {
> -                      this.bufferBar.removeAttribute("buffering");
> -                    } else {
> -                      this.bufferBar.setAttribute("buffering", "");
> -                    }
>                  },
> 
>                  _controlsHiddenByTimeout : false,
>                  _showControlsTimeout : 0,
>                  SHOW_CONTROLS_TIMEOUT_MS: 500,
>                  _showControlsFn : function () {
>                      if (Utils.video.matches("video:hover")) {
>                          Utils.startFadeIn(Utils.controlBar, false);
> diff --git a/toolkit/themes/shared/media/videocontrols.css b/toolkit/themes/shared/media/videocontrols.css
> --- a/toolkit/themes/shared/media/videocontrols.css
> +++ b/toolkit/themes/shared/media/videocontrols.css
> @@ -237,17 +237,17 @@ video > xul|videocontrols {
>  }
> 
>  .bufferBar,
>  .volumeBackground {
>    background-color: #000000;
>    opacity: 0.7;
>  }
> 
> -.bufferBar::-moz-meter-bar,
> +.bufferBar::-moz-progress-bar,
>  .progressBar::-moz-progress-bar,
>  .volumeBackground::-moz-meter-bar {
>    height: 100%;
>    padding: 0;
>    margin: 0;
>    border: 0;
>    border-radius: 2.5px;
>    background: none;
> @@ -276,23 +276,19 @@ video > xul|videocontrols {
>  }
> 
>  .volumeControl::-moz-range-progress {
>    height: 5px;
>    border-radius: 2.5px;
>    background-color: #ffffff;
>  }
> 
> -.bufferBar::-moz-meter-bar {
> -  background-color: #ffffff;
> -  opacity: 0.3;
> -}
> -
> -.bufferBar[buffering]::-moz-meter-bar {
> -  border-radius: 2.5px 0 0 2.5px;
> +.bufferBar::-moz-progress-bar {
> +  background-color: rgba(255,255,255,0.3);
> +  border-radius: 2.5px;
>  }
> 
>  .progressBar::-moz-progress-bar {
>    background-color: #00b6f0;
>  }
> 
>  .volumeBackground::-moz-meter-bar {
>    background-color: transparent;

Comment 46

[Carol Huang [:Carol]]

Hello Stephen, 
Based on our offline discussion (comment 22), could you comment your opinions about the video control set width? Currently the set width is following the video size. We would like to propose to change it to be fixed with the window length. 
Thanks!

Comment 47

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/7-8/

Comment 48

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review76370

Looks good, thanks!

Comment 49

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Carol Huang [:Carol] from comment #46)
> Hello Stephen, 
> Based on our offline discussion (comment 22), could you comment your
> opinions about the video control set width? Currently the set width is
> following the video size. We would like to propose to change it to be fixed
> with the window length. 
> Thanks!

Can you file a new bug for this discussion? Now that we have this patch ready to go I don't want to hold it up and I also worry that the conversation could get lost in the stream of other comments here.

Comment 50

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review76382

Actually, I just noticed now that this patch regresses bug 1239372.

Comment 51

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/8-9/

Comment 52

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review76382

Sorry, I didn't notice that regression. 

This revision should fix the problem. Thanks!

Comment 53

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/9-10/

Comment 54

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review76690

Thanks for the quick fix.

Comment 55

[Carol Huang [:Carol]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #49)
> (In reply to Carol Huang [:Carol] from comment #46)
> > Hello Stephen, 
> > Based on our offline discussion (comment 22), could you comment your
> > opinions about the video control set width? Currently the set width is
> > following the video size. We would like to propose to change it to be fixed
> > with the window length. 
> > Thanks!
> 
> Can you file a new bug for this discussion? Now that we have this patch
> ready to go I don't want to hold it up and I also worry that the
> conversation could get lost in the stream of other comments here.

Sure, we will file a new bug for video control set width discussion! 
thanks!

Comment 56

[Ray Lin[:ralin]]

I noticed that a few tests failure caused by this patch, and I was trying to make them pass. Unfortunately, I found case 25 in "test_videocontrols.html" couldn't get the right value since we defer the initialization process as comment 43. I think it's because that re-initialization(fullscreenchgne) execute later than "SimpleTest.executionSoon". Need a workaround here.

Comment 57

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/10-11/

Comment 58

[Ray Lin[:ralin]]

Sorry to be a nuisance, but I got a problem about <audio>. 

I didn't think of <audio> until today, and its layout is incorrect now. I soon realized that I forgot to change the "display" of <audio> to "inline-block", so I corrected it. However, it make Firefox crashed right after I entered a page with <audio>. 

I thought <video> and <audio> are using the same reflow code. Do you know any possible reason for why <audio> cannot work with "inline-block"? Thanks a lot!

Comment 59

[Jared Wein [:jaws] (please needinfo? me)]

I don't know why that would cause a crash. Do you have a stack from the crash or any more details?

Comment 60

[Ray Lin[:ralin]]

Firefox hangs for a short while and crashes when entering web page.

### The stack obtained from lldb: 
* thread #1: tid = 0x1a5afb, 0x000000010cf7d940 XUL`nsBox::DoesNeedRecalc(this=0x0000000123070d70, aSize=0x0000000000000000) at nsBox.cpp:377, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x000000010cf7d940 XUL`nsBox::DoesNeedRecalc(this=0x0000000123070d70, aSize=0x0000000000000000) at nsBox.cpp:377 [opt]
    frame #1: 0x000000010ce6ff58 XUL`nsFrame::GetXULPrefSize(this=0x0000000123070d70, aState=0x00007fff5585b778) + 72 at nsFrame.cpp:8818 [opt]
    frame #2: 0x000000010ceec775 XUL`nsVideoFrame::GetVideoIntrinsicSize(this=0x0000000123070b78, aRenderingContext=0x00007fff5585d618) + 325 at nsVideoFrame.cpp:624 [opt]
    frame #3: 0x000000010ceec537 XUL`nsVideoFrame::ComputeSize(this=0x0000000123070b78, aRenderingContext=0x00007fff5585d618, aWM=<unavailable>, aCBSize=<unavailable>, aAvailableISize=<unavailable>, aMargin=<unavailable>, aBorder=<unavailable>, aPadding=<unavailable>, aFlags=<unavailable>) + 39 at nsVideoFrame.cpp:538 [opt]
  * frame #4: 0x000000010ce2848a XUL`mozilla::ReflowInput::InitConstraints(this=0x00007fff5585bab8, aPresContext=<unavailable>, aContainingBlockSize=<unavailable>, aBorder=<unavailable>, aPadding=<unavailable>, aFrameType=<unavailable>) + 2906 at ReflowInput.cpp:2369 [opt]
    frame #5: 0x000000010ce260ea XUL`mozilla::ReflowInput::Init(this=<unavailable>, aPresContext=<unavailable>, aContainingBlockSize=<unavailable>, aBorder=<unavailable>, aPadding=<unavailable>) + 698 at ReflowInput.cpp:390 [opt]
    frame #6: 0x000000010ce17e7a XUL`nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, mozilla::ReflowOutput*, bool&) [inlined] void mozilla::Maybe<mozilla::ReflowInput>::emplace<nsPresContext*&, mozilla::ReflowInput const&, nsIFrame*&, mozilla::LogicalSize&>(aArgs=0x00007fff5585bd40, aArgs=<unavailable>) + 38 at Maybe.h:432 [opt]
    frame #7: 0x000000010ce17e54 XUL`nsLineLayout::ReflowFrame(this=<unavailable>, aFrame=<unavailable>, aReflowStatus=<unavailable>, aMetrics=<unavailable>, aPushedFrame=<unavailable>) + 676 at nsLineLayout.cpp:883 [opt]
    frame #8: 0x000000010ce41587 XUL`nsBlockFrame::ReflowInlineFrame(this=0x0000000118e7abf8, aState=0x00007fff5585c138, aLineLayout=0x00007fff5585bd40, aLine=<unavailable>, aFrame=0x0000000123070b78, aLineReflowStatus=0x00007fff5585bcb4) + 71 at nsBlockFrame.cpp:4092 [opt]
    frame #9: 0x000000010ce40e86 XUL`nsBlockFrame::DoReflowInlineFrames(this=<unavailable>, aState=<unavailable>, aLineLayout=<unavailable>, aLine=<unavailable>, aFloatAvailableSpace=<unavailable>, aAvailableSpaceHeight=<unavailable>, aFloatStateBeforeLine=<unavailable>, aKeepReflowGoing=0x0000000000000002, aLineReflowStatus=<unavailable>, aAllowPullUp=<unavailable>) + 582 at nsBlockFrame.cpp:3894 [opt]
    frame #10: 0x000000010ce3e71e XUL`nsBlockFrame::ReflowInlineFrames(this=<unavailable>, aState=<unavailable>, aLine=<unavailable>, aKeepReflowGoing=<unavailable>) + 494 at nsBlockFrame.cpp:3760 [opt]


### CSS rules:
video > xul|videocontrols,
audio > xul|videocontrols {
  display: inline-block;
}

### video control Reflow:
https://reviewboard.mozilla.org/r/72154/diff/11/download/2228524/new/


I think it's something related to reflow or sizing problem, but had a hard time figuring it out.

Comment 61

[Jared Wein [:jaws] (please needinfo? me)]

Inside of nsBox::DoesNeedRecalc, aSize is set to nullptr. The issue looks to stem from your changes to layout/generic/nsVideoFrame.cpp, which I should have mentioned before but that will need review from a layout peer, of which I am not.

Why do we need to make those changes to nsVideoFrame.cpp? Why do we need to switch the display property here?

cpearce, can you help Ray out with the nsVideoFrame.cpp changes here?

Comment 62

[Ray Lin[:ralin]]

Due to converting all xul to html, the changes of nsVideoFrame.cpp and display property are based on the comment 1 and 2 in Bug 1222273.

Comment 63

[Jared Wein [:jaws] (please needinfo? me)]

Ah thanks for reminding me. dholbert, you had looked at bug 1222273, do you see what's going wrong here?

Comment 64

[Daniel Holbert [:dholbert]]

Can you clarify how to trigger the crash? I built with the latest patch[1] attached, and then visited https://upload.wikimedia.org/wikipedia/commons/1/1d/Demo_chorus.ogg , and I didn't see any crash.

[1] https://reviewboard-hg.mozilla.org/gecko/rev/db28b67af1017f5fc0ee6e212a2d070316db7d2b

Comment 65

[Daniel Holbert [:dholbert]]

Ah, OK -- if I additionally tweak videocontrols.css to add "audio > xul|videocontrols" selector to the inline-block rule (per end of comment 60), I do indeed crash when loading this minimal testcase:
  data:text/html,<audio src="https://upload.wikimedia.org/wikipedia/commons/1/1d/Demo_chorus.ogg" controls>

I'll see what I can find out...

Comment 66

[Daniel Holbert [:dholbert]]

It looks like the problem is that nsVideoFrame::GetVideoIntrinsicSize has this code:
>  nscoord prefHeight = mFrames.LastChild()->GetXULPrefSize(boxState).height;
...and in this case mFrames.LastChild() is a nsBlockFrame, whereas in the old code I expect it was some sort of XUL frame.

GetXULPrefSize triggers an assertion and then a null-deref crash because it tries to look up some cached XUL state on the frame.

I think GetVideoIntrinsicSize needs a rewrite to avoid a dependency on that XUL method (and perhaps to avoid thinking it can look up the height up-front, before it's been reflowed).

Comment 67

[Daniel Holbert [:dholbert]]

I think the only caller of nsVideoFrame::GetVideoIntrinsicSize that cares about the height is nsVideoFrame::ComputeSize.

We might just want to have that frame fall back to the inherited method (nsContainerFrame::ComputeSize, which is really nsFrame::ComputeSize), in the no-video-element scenario.

Note that this will give us a computed height of NS_INTRINSICSIZE, though (i.e. the sentinel value 2^30 == nscoord_MAX).  And we'll need to rewrite nsVideoFrame::Reflow to handle that sentinel value & just shrinkwrap the child's height.

nsNumberControlFrame::Reflow might be a useful thing to crib from for that. And these frame-class changes might perhaps all belong in bug 1222273 (though they presumably need to land at the same time as this bug).

Comment 68

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #67)
> We might just want to have that frame fall back to the inherited method

sorry, s/that frame/that method/

Comment 69

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #61)
> Inside of nsBox::DoesNeedRecalc, aSize is set to nullptr. The issue looks to
> stem from your changes to layout/generic/nsVideoFrame.cpp, which I should
> have mentioned before but that will need review from a layout peer, of which
> I am not.
> 
> Why do we need to make those changes to nsVideoFrame.cpp? Why do we need to
> switch the display property here?
> 
> cpearce, can you help Ray out with the nsVideoFrame.cpp changes here?

Someone on layout should really look at nsVideoFrame.cpp. I don't really understand how layout works.

Comment 70

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/11-12/

Comment 71

[Ray Lin[:ralin]]

Jared, I got a little workaround to inline-block problem.

We could remain the display of <audio> to "-moz-box", but set the outermost DIV height to certain value: 40px instead.
Is this is a proper way for now? It would be great if we could land this patch, and fix layout problem straight afterwards.

Thanks.

Comment 72

[Jared Wein [:jaws] (please needinfo? me)]

We should just fix the layout problem in one step. Can you try following the steps in comment 67 to fix the layout bug? You can needinfo dholbert for help if you get stuck.

Also, this patch cannot land until it has review from a layout peer anyways since you've changed some code under layout and I'm not a layout peer.

Comment 74

[Ray Lin[:ralin]]

It's clear to me now. Thank you very much :)

Comment 75

[Astley Chen (inactive)]

I'll suggest to split the patch into 2. The changes in nsVideoFrame.cpp could be reviewed by layout peer, say dholbert. It would be a lot easier to maintain your patch during review iterations.

Comment 76

[Daniel Holbert [:dholbert]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

https://reviewboard.mozilla.org/r/72154/#review81084

::: layout/generic/nsVideoFrame.cpp:341
(Diff revision 12)
>        ReflowChild(imageFrame, aPresContext, kidDesiredSize, kidReflowInput,
>                    posterRenderRect.x, posterRenderRect.y, 0, aStatus);
>        FinishReflowChild(imageFrame, aPresContext,
>                          kidDesiredSize, &kidReflowInput,
>                          posterRenderRect.x, posterRenderRect.y, 0);
> +#ifdef ANDROID

ralin, I'm curious about the "#ifdef ANDROID" in the video-controls reflow code here.

Why does this patch need different reflow behavior for Android video-controls vs. desktop video controls? Are we migrating one to HTML but not the other?

If so: ideally, it'd be great if we could migrate both simultaneously....  I'd rather not have two parallel reflow code-paths here. (And if we tried to have two codepaths, I think we'd need more than just this one #ifdef.)

Comment 77

[Ray Lin[:ralin]]

Sorry for no update for days.

I didn't expect that the de-xul process come with these derived issues. In addition to mochitests, I noticed that a11y test also fails due to different way of accessible tree traversal. In this week, I've been fixing the test failures but not yet make them green on all the platforms up to now. 

dholbert, yes, we're migrate only desktop for now. I thought the reflow code was ready for de-xul at beginning, so I planned to de-xul video control incrementally: desktop first and then file a followup bug for Android if the process in desktop went fine. Since we've gone this far on de-xul, I'd like to finish it if possible. Can we try to have two codepaths first, and see how minimal of changes and side effect it could be?

Comment 78

[Ray Lin[:ralin]]

Jared, can we use monospaced font in duration/position label? The variable time of position label causes scrubber width vibrate while dragging. Also this subtle issue seems to lead case 13 in "test_videocontrols.html" failed as it depends on scrubber width precisely.

Thanks.

Comment 79

[Jared Wein [:jaws] (please needinfo? me)]

A monospace font will still cause the scrubber to change widths when the position moves from 59 minutes to 1 hour 1 minute, right?

I think the better approach would be to set a minimum width of the duration/position label and use `ch` units, each of which are equal to the width of the '0' character.

Comment 80

[Ray Lin[:ralin]]

Thank you Jared!! `ch` unit does help a lot.

This is the last try I push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c74c4c38aea2e9ab46dfeaa33d6a02f722ec262
Main front-end mochitest are green now, but I've been struggling with accessible tree in a11y test. 

expected accessible tree order: 
> audio
>> pushbutton, progressbar, progressbar, slider, pushbutton, slider
actual order:
> audio
>> pushbutton, progressbar, progressbar, slider, #text, #text, pushbutton, slider

I got stuck to eliminate the two #text generated from duration and position label. I've tried `aria-hidden` or some role like `none` or `presentation` to hide them from accessible tree but failed. 


Thank to :dholbert, according to comment 67, I think I basically can get BSize from ChildReflow but the height returns 0, which might caused by the positioned flexbox:

HTML:
<audio>
.....
<div class="controlsOverlay">
  <div class="controlBar"></div>
</div>
.....
<audio>

CSS:
.controlsOverlay {
  display: flex;
  position: absolute;
  width: 100%;
  height: 100%;
  bottom: 0;
  left: 0;
}

.controlBar {
  display: flex;
  height: 40px;
}

It seems that .controlsOverlay doesn't expand to fit the content's height of .controlBar. 
:dholbert, does flexbox compatible with position:absolute? Thanks :)

Comment 81

[Daniel Holbert [:dholbert]]

(In reply to Ray Lin[:ralin] from comment #80)
> Thank to :dholbert, according to comment 67, I think I basically can get
> BSize from ChildReflow but the height returns 0, which might caused by the
> positioned flexbox:

Yeah, that's due to the way abspos works -- it pulls items out of flow, which means they don't influence sizing/positioning of their parent or siblings anymore.  (At a technical level, they're replaced with a 0-height-and-width "placeholder frame", and the abspos frame gets put in a different special child list on a potentially-different frame.)

The zero-height that you're seeing is from the placeholder frame (and if you're using position:absolute, that's expected/correct for it to have zero height & not influence its parent's auto-sizing behavior).

> It seems that .controlsOverlay doesn't expand to fit the content's height of
> .controlBar.

Given the "height/width/bottom/left" properties that you set, it should expand to fit the size of its *containing block*, which is the nearest ancestor with the "position" property set to anything other than the default ("static").

So I'd expect it's probably *larger* than .controlBar right now... (if not, something's wonky)

Does it help if you set "position: relative" on .controlBar?

Comment 82

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ray Lin[:ralin] from comment #80)
> Thank you Jared!! `ch` unit does help a lot.
> 
> This is the last try I push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9c74c4c38aea2e9ab46dfeaa33d6a02f722ec262
> Main front-end mochitest are green now, but I've been struggling with
> accessible tree in a11y test. 
> 
> expected accessible tree order: 
> > audio
> >> pushbutton, progressbar, progressbar, slider, pushbutton, slider
> actual order:
> > audio
> >> pushbutton, progressbar, progressbar, slider, #text, #text, pushbutton, slider
> 
> I got stuck to eliminate the two #text generated from duration and position
> label. I've tried `aria-hidden` or some role like `none` or `presentation`
> to hide them from accessible tree but failed. 

Are you sure that we actually *want* to hide these from the accessibility tree? We should ask someone from a11y for advice here.

David, with this refactoring of the video controls the current position and duration of the media shows up in the accessibility tree as two separate text nodes. Should we actively try to hide these from the a11y tree or leave them as-is so a11y users have the ability to read them and know what they are saying?

Comment 83

[David Bolter [:davidb] (NeedInfo me for attention)]

Good instincts. Alex probably wrote these tests so bumping NI over to him, thanks.

Comment 84

[Ray Lin[:ralin]]

Attachment: Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

Review commit: https://reviewboard.mozilla.org/r/85216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85216/

Comment 85

[Ray Lin[:ralin]]

Attachment: Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review commit: https://reviewboard.mozilla.org/r/85218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85218/

Comment 86

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/12-13/

Comment 87

[Ray Lin[:ralin]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #82)
> Are you sure that we actually *want* to hide these from the accessibility
> tree? We should ask someone from a11y for advice here.

My fault, I thought we have to preserve it as current a11y tree structure.
Thanks for help.

(In reply to Daniel Holbert [:dholbert] from comment #81)
> (In reply to Ray Lin[:ralin] from comment #80)
> Does it help if you set "position: relative" on .controlBar?

Yes, <audio> fit the .controlbar's size correctly now.


---
I just updated patches, could you help me to review? Thanks you very much :)

Comment 88

[Marco Zehe (:MarcoZ)]

(In reply to Ray Lin[:ralin] from comment #87)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #82)
> > Are you sure that we actually *want* to hide these from the accessibility
> > tree? We should ask someone from a11y for advice here.
> 
> My fault, I thought we have to preserve it as current a11y tree structure.
> Thanks for help.
No, if there are new features or controls, and they're exposed, adding them to the relevant tests is perfectly fine. Asking someone from the accessibility team is always a good idea, but accessibility can be adapted/improved by anyone. :-)

Comment 89

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8800232 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

https://reviewboard.mozilla.org/r/85218/#review83806

::: toolkit/content/tests/widgets/test_videocontrols.html:237
(Diff revision 1)
>       */
>      case 13:
>        is(event.type, "seeked", "checking event type");
>        ok(true, "video position is at " + video.currentTime);
>        var expectedTime = videoDuration / 2;
> -      ok(Math.abs(video.currentTime - expectedTime) < 0.1, "checking expected playback position");
> +      ok(Math.abs(video.currentTime - expectedTime) < 0.25, "checking expected playback position");

Do you know why the variance here needed to be increased?

::: toolkit/content/tests/widgets/test_videocontrols_vtt.html:49
(Diff revision 1)
> -      is(ccBtn.getAttribute("hidden"), "", "CC button should show");
> -      is(ccBtn.getAttribute("enabled"), "", "CC button should be disabled");
> +      is(!!ccBtn.getAttribute("hidden"), false, "CC button should show");
> +      is(!!ccBtn.getAttribute("enabled"), false, "CC button should be disabled");

These lines shouldn't need to change.

If you still think they need to change, can we use ccBtn.hasAttribute() instead?

Comment 90

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #88)
> (In reply to Ray Lin[:ralin] from comment #87)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #82)
> > > Are you sure that we actually *want* to hide these from the accessibility
> > > tree? We should ask someone from a11y for advice here.
> > 
> > My fault, I thought we have to preserve it as current a11y tree structure.
> > Thanks for help.
> No, if there are new features or controls, and they're exposed, adding them
> to the relevant tests is perfectly fine. Asking someone from the
> accessibility team is always a good idea, but accessibility can be
> adapted/improved by anyone. :-)

I second this. If those #text are labels for duration and position then they probably need to have 'label' role and serve as labels for those controls.

Marco, did you have a chance to try out the new implementation?

Comment 91

[Ray Lin[:ralin]]

Comment on attachment 8800232 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

https://reviewboard.mozilla.org/r/85218/#review83806

> Do you know why the variance here needed to be increased?

Yes. Duration and position box squeeze the width of slider; moreover slider is squeezed to around 60px, so small variance of font-size can highly affects. 

In Windows and Linux, `font-size-adjust` can adjust the size, but the major problem is that the system font of OS X 10.10 & 10.7 are different and no way to determine by text preprocessor's directive. The variance normally would be < 0.1 (~ 0.088) except for OS X 10.10, which get 0.24.

> These lines shouldn't need to change.
> 
> If you still think they need to change, can we use ccBtn.hasAttribute() instead?

it return null instead of empty string in HTML, so made this change.
ah, hasAttribute() is definitely more proper, thanks!!

Comment 92

[Ray Lin[:ralin]]

(In reply to Marco Zehe (:MarcoZ) from comment #88)
> No, if there are new features or controls, and they're exposed, adding them
> to the relevant tests is perfectly fine. Asking someone from the
> accessibility team is always a good idea, but accessibility can be
> adapted/improved by anyone. :-)

(In reply to alexander :surkov from comment #90)

I greatly appreciated your help :)

I have a hard time understanding how the a11y tree are generated in tests by which standard or tool. Could you give me a clue how to get the tree that tests expect to compare with? such as { grouping: [ pushbutton: [ ] , progressbar: [ ] , progressbar: [ ] , slider: [ ] , pushbutton: [ ] , slider: [ ] ] } in test[0]. The tool I know so far is only extension `DOM inspector`. Thanks.

[0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d066044e165c1560fb792988552e4729d2ab7a9&selectedJob=29025740

Comment 93

[Marco Zehe (:MarcoZ)]

(In reply to Ray Lin[:ralin] from comment #92)
> I have a hard time understanding how the a11y tree are generated in tests by
> which standard or tool. Could you give me a clue how to get the tree that
> tests expect to compare with? such as { grouping: [ pushbutton: [ ] ,
> progressbar: [ ] , progressbar: [ ] , slider: [ ] , pushbutton: [ ] ,
> slider: [ ] ] } in test[0]. The tool I know so far is only extension `DOM
> inspector`. Thanks.

Yes, that's the one you should use. However you need to inspect the Chrome tree, so you can get at the accessibles generated by anonymous XBL content, and you may have to turn off E10S since I believe DOM inspector isn't fit for E10S.

Alex, or any other hints you are using nowadays as a sighted, non-screen-reader, user?

Comment 94

[alexander :surkov (:asurkov)]

DOMi is quite useful tool and possibly it doesn't have alternatives in a11y tests development. I rely on on it a lot these days.

Having said that the accessible tree is a secondary issue. The most important question is whether the new design is accessible or not. As you probably know the accessible UI is presented by accessible tree, which is not necessary unique, i.e. different accessible trees may represent same UI. Mostly we add a11y tests for UI stuff to make sure UI is not changed, which means stays accessible. If UI has changed, then our tests should be adjusted. Before you make a change, it makes sense to ask Marco or somebody else experienced to check whether new UI is still accessible.

In your case, I'd say the #text nodes should be wrapped by role='label' accessibles as I mentioned in comment #90, so I would expect the tree like (note this assumption is based on comment #80, I didn't see the actual UI):

audio
  pushbutton,
  progressbar,
  progressbar,
  slider,
  label
    #text,
  label
   #text,
  pushbutton,
  slider

but if those text nodes are not annnounced by screen readers, then you could ignore them, and add just add them into the test.

Comment 95

[Ray Lin[:ralin]]

Attachment: audio UI and its a11y tree

Glad to know more about a11y and how a11y test works. The attachment is new <audio> UI, and its a11y tree. Also, the implementation of position/duration box is changed to this way(so as you can see on the screenshot, the text nodes are under a same parent):

<span class="positionAndDurationBox">
 0:00<span class="duration"> / 0:18</span>
</span>

(In reply to alexander :surkov from comment #94)
> In your case, I'd say the #text nodes should be wrapped by role='label'
New UI is refactored to HTML. According to comment 25, <xul:label> has much different semantics than <html:label>, do you think it make sense to use <span>(ROLE_TEXT_CONTAINER) instead of <label>? 

Thanks.

Comment 96

[Daniel Holbert [:dholbert]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

https://reviewboard.mozilla.org/r/85216/#review85282

Sorry -- so unfortunately, this existing reflow code (for the caption, which you're co-opting for the controls as well) is very broken, and I'm uneasy about reusing it without fixing it to be correct -- since any existing brokenness will be more catastrophic/visible if it breaks the controls (as it now can do) instead of breaking the caption (which is all it used to be able to do).

Specific notes on brokenness below.  A lot of this fixup should ideally happen in its own patch(es), independent from the refresh here (though it might only be visible after the refresh has taken effect).

::: layout/generic/nsVideoFrame.cpp:342
(Diff revision 1)
>        ReflowChild(imageFrame, aPresContext, kidDesiredSize, kidReflowInput,
>                    posterRenderRect.x, posterRenderRect.y, 0, aStatus);
>        FinishReflowChild(imageFrame, aPresContext,
>                          kidDesiredSize, &kidReflowInput,
>                          posterRenderRect.x, posterRenderRect.y, 0);
> +#ifdef ANDROID

Above this #ifdef ANDROID line, please include a code-comment with a reference to a bug number for wherever we'll be removing this Android-specific code (i.e. wherever we'll be doing this refresh for Android).

(Please file a bug on that, if such a bug doesn't already exist.)

::: layout/generic/nsVideoFrame.cpp:357
(Diff revision 1)
>                                         aReflowInput.ComputedHeight()));
>        if (child->GetSize() != size) {
>          RefPtr<Runnable> event = new DispatchResizeToControls(child->GetContent());
>          nsContentUtils::AddScriptRunner(event);
>        }
> -    } else if (child->GetContent() == mCaptionDiv) {
> +#endif

Add "// ANDROID" after #endif here

::: layout/generic/nsVideoFrame.cpp:358
(Diff revision 1)
> -      // Reflow to caption div
> +    } else if (child->GetContent() == mCaptionDiv ||
> +        child->GetContent() == mVideoControls) {

Fix indentation (so that "child" on this new line is lined up with "child" on the previous line)

::: layout/generic/nsVideoFrame.cpp:361
(Diff revision 1)
>        }
> -    } else if (child->GetContent() == mCaptionDiv) {
> -      // Reflow to caption div
> +#endif
> +    } else if (child->GetContent() == mCaptionDiv ||
> +        child->GetContent() == mVideoControls) {
> +      // Reflow the caption and control bar frames.
>        ReflowOutput kidDesiredSize(aReflowInput);

This is a (preexisting) typo.

When we create a ReflowOutput for the child, we should be passing in *that child's* ReflowInput. Right now, we're passing in the nsVideoFrame's ReflowInput instead -- that's wrong.

So, this line needs to look like:
  ReflowOutput kidDesiredSize(kidReflowInput);
...and it of course needs to move down below where we declare kidReflowInput.

::: layout/generic/nsVideoFrame.cpp:363
(Diff revision 1)
>        LogicalSize availableSize = aReflowInput.AvailableSize(wm);
>        LogicalSize cbSize = aMetrics.Size(aMetrics.GetWritingMode()).
>                               ConvertTo(wm, aMetrics.GetWritingMode());

These two LogicalSize variables are also bogus:
 (1) we shouldn't be reusing the video frame's available-space as the available-space for its children.  E.g. if the video frame happens to have "width: 100px" and its parent is 9999px wide, then aReflowInput.AvailableSize(wm) will reflect that huge parent-width here.  We shouldn't be telling its components that they each have 2000px of available width. We should instead be using the <video> frame's own content-box width/height as the available space for its children.

 (2) We shouldn't be trusting aMetrics to have anything useful at this point... aMetrics is the outparam of this function that we're in, and we maybe haven't set it to anything useful yet.

::: layout/generic/nsVideoFrame.cpp:373
(Diff revision 1)
>                                         child,
>                                         availableSize,
>                                         &cbSize);
>        nsSize size(aReflowInput.ComputedWidth(), aReflowInput.ComputedHeight());
>        size.width -= kidReflowInput.ComputedPhysicalBorderPadding().LeftRight();
>        size.height -= kidReflowInput.ComputedPhysicalBorderPadding().TopBottom();

This |size| math seems suspicious and wrong, and I'm uneasy about co-opting existing suspicious code for a new use.

In particular:
 (1) aReflowInput.ComputedWidth() and/or aReflowInput.ComputedHeight() (whichever one is in the block axis) could be NS_INTRINSICSIZE here, if our <video> has e.g "height: auto".  (Or if it has "writing-mode:vertical-lr; width:auto".)

We should not be doing subtraction from that sentinel value, and we should definitely not be passing around the result of that subtraction.

 (2) Disregarding that NS_INTRINSICSIZE stuff: the use of aReflowInput and border/padding subtraction is *very suspect* here. It looks like we're creating a ReflowInput for our caption (and control) frames -- with sizes taken from their styling -- but then we're stomping on those sizes with different sizes that we take from the outer <video> frame (via aReflowInput, which is the outer frame).  I think we *really* should be setting useful sizes for these children in CSS (perhaps "height: 100%; width: 100%"?) and then honoring those sizes here instead of stomping on them...

(I suspect this code is currently hacking around the problem created by the fact that we've got the wrong availableSpace, as noted above, which means "height: 100%; width: 100%" won't do the right thing until we've fixed the availableSpace issue.)

::: layout/generic/nsVideoFrame.cpp:375
(Diff revision 1)
> +      if (child->GetContent() == mVideoControls && child->GetSize() != size) {
> +        RefPtr<Runnable> event = new DispatchResizeToControls(child->GetContent());
> +        nsContentUtils::AddScriptRunner(event);
> +      }

This is copypasted from the #ifdef ANDROID section.

Instead of copypasting this code, let's share it instead. Specifically, let's:
 * Create a local variable "nsSize oldChildSize = child->GetSize()" at the very beginning of this for-loop.
 * Shift this "DispatchResizeToControls" clause to the very end of the for-loop (and compare child->GetSize() against oldChildSize -- since the child->GetSize() call will return the new size at that point.  FinishReflowChild is what commits the new size to the child.)

That refactoring should ideally happen in its own patch.

::: layout/generic/nsVideoFrame.cpp:391
(Diff revision 1)
> +      if (contentBoxBSize == NS_INTRINSICSIZE) {
> +        contentBoxBSize = kidDesiredSize.BSize(wm);
> +        contentBoxBSize = NS_CSS_MINMAX(contentBoxBSize,
> +                                        aReflowInput.ComputedMinBSize(),
> +                                        aReflowInput.ComputedMaxBSize());
> +        aMetrics.Height() = contentBoxBSize;

A few things are wrong here:
 (1) aMetrics.Height() is the outparam of our nsVideoFrame reflow, and it represents the height of the *entire* <video> (or <audio>) frame. It doesn't make sense that we're assigning it here, because:
  (1a) we're assigning it multiple times (on each time through this loop)
  (1b) we're assigning it to the size of this one arbitrary child (whichever child we happen to loop over last) -- and it's not obvious to me that this ends up producing a height that's tall enough for all of the children (e.g. does it give us enough space for the video as well as the controls?)

 (2) We're assigning Height() to a BSize value, which is only correct if we have a horizontal writing-mode.  This will produce broken layout (specifically, the child's width being co-opted as the parent's height) if we have "writing-mode: vertical-rl" or "vertical-lr".

You probably need to:
  * create a LogicalSize called e.g. "logicalDesiredSize" to represent the desired size of the nsVideoFrame (both its i-size and its b-size), in terms of writing mode "wm"
  * call aMetrics.SetSize(wm, logicalDesiredSize)

See e.g. https://dxr.mozilla.org/mozilla-central/rev/7c8216f48c38a8498f251fe044509b930af44de6/layout/forms/nsNumberControlFrame.cpp#235 for sample code that does this.

Comment 97

[alexander :surkov (:asurkov)]

(In reply to Ray Lin[:ralin] from comment #95)
> Created attachment 8801194 [details]
> audio UI and its a11y tree
> 
> Glad to know more about a11y and how a11y test works. The attachment is new
> <audio> UI, and its a11y tree. Also, the implementation of position/duration
> box is changed to this way(so as you can see on the screenshot, the text
> nodes are under a same parent):
> 
> <span class="positionAndDurationBox">
>  0:00<span class="duration"> / 0:18</span>
> </span>
> 
> (In reply to alexander :surkov from comment #94)
> > In your case, I'd say the #text nodes should be wrapped by role='label'
> New UI is refactored to HTML. According to comment 25, <xul:label> has much
> different semantics than <html:label>, do you think it make sense to use
> <span>(ROLE_TEXT_CONTAINER) instead of <label>? 

not that I'm aware of :)

anyway, since those position and duration are likely redundant because they duplicate info from the preceding slider/progressbar controls, then quite possibly, the screen readers don't need them. Also it's possible their presence doesn't harm anyone.

So I can see two options:
1) ni? :marcoz asking to run the try build, to check whether there are any issues with the tree from the screen readers perspective, and if no issues, then just update the test, and that's it.
2) add aria-hidden="true" on that span. aria-hidden does not affect on the accessible tree, but still exposed to screen readers, and many of them know how to process it. In this case, you may want to add 'attributes: { "aria-hidden": "true" }' to the test something like:

tree = {
  role: AUDIO,
  children: [
    // existing children
    {
       role: ROLE_TEXT_CONTAINER,
       attributes: { "aria-hidden": "true" },
       children: [
          {
            role: TEXT_LEAF // position text
          },
          {
            role: TEXT_LEAF // duration text
          }
       ]
    }
};

Comment 98

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85216/diff/1-2/

Comment 99

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/13-14/

Comment 100

[Ray Lin[:ralin]]

Comment on attachment 8800232 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85218/diff/1-2/

Comment 101

[Ray Lin[:ralin]]

quickfix: part 3

updated accessible/tests/mochitest/tree/test_media.html as comment 97. Thank you Alex :)
then I think all the test failures caused by front-end refactoring are fixed now.

Comment 102

[Daniel Holbert [:dholbert]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

(assuming review flag was toggled by accident; doesn't look like this patch has changed since comment 96, so my notes & r- from that comment still stand)

Comment 103

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85216/diff/2-3/

Comment 104

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/14-15/

Comment 105

[Ray Lin[:ralin]]

Comment on attachment 8800232 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85218/diff/2-3/

Comment 106

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

https://reviewboard.mozilla.org/r/85216/#review85282

Sorry for being like spamming, I should figure out how mozreview pushlishing multiple patch. (or publish partial diff only)

> This is copypasted from the #ifdef ANDROID section.
> 
> Instead of copypasting this code, let's share it instead. Specifically, let's:
>  * Create a local variable "nsSize oldChildSize = child->GetSize()" at the very beginning of this for-loop.
>  * Shift this "DispatchResizeToControls" clause to the very end of the for-loop (and compare child->GetSize() against oldChildSize -- since the child->GetSize() call will return the new size at that point.  FinishReflowChild is what commits the new size to the child.)
> 
> That refactoring should ideally happen in its own patch.

Sorry I actually copypasted these lines of code from desktop to mobile, that is it is desktop specific originally since Android doesn't react to `controlsizechnage` event. So I removed it from Android in this revision.

Comment 107

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85216/diff/3-4/

Comment 108

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/15-16/

Comment 109

[Ray Lin[:ralin]]

Comment on attachment 8800232 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85218/diff/3-4/

Comment 110

[Daniel Holbert [:dholbert]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

https://reviewboard.mozilla.org/r/85216/#review87902

Sorry for the delay here - thanks for being patient through my vacation.

This is much closer! Notes below:

::: layout/generic/nsVideoFrame.cpp:302
(Diff revision 4)
>    NS_PRECONDITION(mState & NS_FRAME_IN_REFLOW, "frame is not in reflow");
>  
>    aStatus = NS_FRAME_COMPLETE;
>  
> -  aMetrics.Width() = aReflowInput.ComputedWidth();
> -  aMetrics.Height() = aReflowInput.ComputedHeight();
> +  const WritingMode myWM = aReflowInput.GetWritingMode();
> +  const nscoord contentBoxISize = aReflowInput.ComputedISize();

Nit: let's simplify a bit by getting rid of |contentBoxISize|, and just directly call aReflowInput.ComputedISize() at the one place where we use it.

(when defining borderBoxISize)

The "ReflowInput" struct exists to precompute/encapsulate a lot of reflow information, and it's nice to just use its encapsulated data directly when possible, rather than creating a local copy of one of its members.

(You're probably doing this for symmetry with contentBoxBSize, which is admirable.  But ISize/BSize are inherently different, so it's OK to take an optimally-clean/short codepath for ISize and have more complicated stuff for BSize.)

::: layout/generic/nsVideoFrame.cpp:308
(Diff revision 4)
> +  if (contentBoxBSize != NS_INTRINSICSIZE) {
> +    borderBoxBSize = contentBoxBSize +
> +      aReflowInput.ComputedLogicalBorderPadding().BStartEnd(myWM);
> +  }

Let's change this to:
  const bool isBSizeShrinkWrapping = (contentBoxBSize == NS_INTRINSICSIZE);
  if (!isBSizeShrinkWrapping) {
    ...
  }

And then we'll use isBSizeShrinkWrapping again further down. (more on that later)

::: layout/generic/nsVideoFrame.cpp:315
(Diff revision 4)
>    // Reflow the child frames. We may have up to two, an image frame
>    // which is the poster, and a box frame, which is the video controls.

This comment needs several updates:
 (1) The phrase "and a box frame" is no longer correct, as of this bug ("box" here means "display:-moz-box" i.e. XUL)
 (2) The count here is not correct (which is a problem that predates this bug).  We may have three frames (not two) -- the poster, the controls, and the caption.  This comment must predate our VTT (subtitle/caption) support.

Let's just update this to say something like:
"We may have up to three: an image frame (for the poster image), a container frame for the controls, and a container frame for the caption."

(I'm using "container frame" to be intentionally vague, because we don't necessarily know/care precisely what type of frames those ones are.)

::: layout/generic/nsVideoFrame.cpp:347
(Diff revision 4)
> +// We will be removing this hack in Bug 1310907
> +#ifdef ANDROID

Extend this comment slightly, to explain what the point of "this hack" is.

E.g.
  // Android still uses XUL media controls & hence needs this XUL-friendly
  // custom reflow code. This will go away in bug 1310907.

::: layout/generic/nsVideoFrame.cpp:376
(Diff revision 4)
> +      if (contentBoxBSize == NS_INTRINSICSIZE) {
> +        contentBoxBSize = kidDesiredSize.BSize(wm);
> +        contentBoxBSize = NS_CSS_MINMAX(contentBoxBSize,
> +                                        aReflowInput.ComputedMinBSize(),
> +                                        aReflowInput.ComputedMaxBSize());
> +        borderBoxBSize = contentBoxBSize +
> +          aReflowInput.ComputedLogicalBorderPadding().BStartEnd(wm);
> +      }

tl;dr of my suggestions:
 (1) add a "child->GetContent() == mVideoControls" check to this if-check.
 (2) Shift the NS_CSS_MINMAX and borderBoxBSize assignment to happen a bit later, after a fallback bsize-still-unconstrained check (sample code below)

My concerns, that motivate these ^^ suggestions:
 CONCERN 1: This code (setting contentBoxBSize) might get traversed *twice* (once for mCaptionDiv, once for mVideoControls).  And if that happens, with the current patch, then whichever child happens to be first will "win". That seems arbitrary and probably-failure-prone. We need something more deterministic. The scenario where this matters is when we have an unconstrained height (which means we didn't get our size from our intrinsic ratio... not sure if that ever happens, in practice, but let's say it does since we're already handling it here in this patch).  In that case, we probably want to size to the controls (not to the VTT), I think?  So we probably want to add "&& child->GetContent() == mVideoControls" to this check.  And we might as well also replace the NS_INTRINSICSIZE check with "isBSizeShrinkWrapping" since we've got that bool now, per a suggestion above.

CONCERN 2: This code might get traversed *zero* times, if we have no subtitles and no video controls. And then we'll be stuck with a contentBoxBSize that stays at NS_INTRINSICSIZE, which is bad (we'd end up with a frame 2^31 app units tall).

To prevent that, let's add a fallback case just after the "for" loop:
  if (isBSizeShrinkWrapping) {
    if (contentBoxBSize == NS_INTRINSICSIZE) {
      // We didn't get a BSize from our intrinsic size/ratio, nor did we
      // get one from our controls. Just use BSize of 0.
      contentBoxBSize = 0;
    }
    contentBoxBSize = NS_CSS_MINMAX(...);
    borderBoxBSize = ...;
  }

This means you don't need the NS_CSS_MINMAX() and borderBBox lines that I quoted from your patch above -- you can just share this chunk in the post-for-loop isBSizeShrinkWrapping clause for that.

Hopefully that all makes sense. :) Ping me if not.

::: layout/generic/nsVideoFrame.cpp:547
(Diff revision 4)
> +  if (!HasVideoElement()) {
> +    return nsContainerFrame::ComputeSize(aRenderingContext,
> +                                         aWM,

Does this new early-return break the Android <audio controls> experience, by any chance?  I'm a bit surprised that we can use "#ifdef ANDROID" in *just* Reflow, without also keeping the old intrinsic-sizing codepath for Android as well...  I suspect we might need similar #ifdeffing (and links to bug 1310907) here and also around the code that you're currently removing in GetVideoIntrinsicSize -- i.e. I suspect we need to preserve those old codepaths, on Android, for now.

::: layout/generic/nsVideoFrame.cpp
(Diff revision 4)
> -  if (!HasVideoElement()) {
> -    if (!mFrames.FirstChild()) {
> -      return nsSize(0, 0);
> -    }
> -
> -    // Ask the controls frame what its preferred height is
> -    nsBoxLayoutState boxState(PresContext(), aRenderingContext, 0);
> -    nscoord prefHeight = mFrames.LastChild()->GetXULPrefSize(boxState).height;
> -    return nsSize(nsPresContext::CSSPixelsToAppUnits(size.width), prefHeight);
> -  }

This code might need to stick around on Android, as I noted above.

But setting that problem aside for the moment: this removal means we'll return an intrinsic size of 300 by 150 for <audio> frames. That's kinda bad.  It doesn't make too much of a difference, since you're not letting it influence ComputeSize anymore.  BUT, it does come into play via some of the intrinsic-size callsites if we have a vertical writing-mode (in which case GetPrefISize returns a height value).  Like in this testcase:
  data:text/html,<audio style="writing-mode: vertical-rl; border: 5px solid black;" controls>

This testcase renders with a box which is (unnecessarily) 150px tall, if this patch is applied. Whereas in Nightly, it's just as tall as the controls.  It's also got an infinite-width controls frame, which is also broken. :)  That's likely a form of flexbox bug 1282940, but it's something we can & should avoid here, since the design probably doesn't work right even if that bug were fixed.

Let's address this as follows:
 (1) Please style the video controls with "writing-mode: horizontal-tb" (assuming your design depends on the controls laying out horizontally -- and I assume it does).
 (2) Please guard all calls to this function with "HasVideoElement()" checks (like the one that you're now adding in ComputeSize(), and the one we've already got in GetIntrinsicRatio())
 (3) In the callsites that you changed in (2), please return something reasonable in the no-video-element case. Perhaps do what nsNumberFrame does -- I think its calls to nsLayoutUtils::IntrinsicForContainer is similar to what you'd want here -- you probably want to call that for the controls frame, if one is present.  (That'll do the equivalent of the XUL "Ask the controls frame" code that you're removing).

Suggestion (1) here should fix the infinite-width-controls issue in my testcase above. And suggestions 2/3 should fix the awkward 150px height.  (THOUGH: suggestion (2) is incompatible with keeping this code around for Android. :(  This is the sort of preserving-two-codepaths complexity that I was worried about... Maybe we don't really need this code on Android after all? I don't know...)

Comment 111

[Daniel Holbert [:dholbert]]

Comment on attachment 8800232 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

https://reviewboard.mozilla.org/r/85218/#review87966

::: toolkit/themes/shared/media/videocontrols.css:12
(Diff revision 4)
>  
>  
>  video > xul|videocontrols,
>  audio > xul|videocontrols {
> +  width: 100%;
> +  height: 100%;

(As we discovered over email, this change belongs in part 2, not in part 3.)

Comment 112

[Daniel Holbert [:dholbert]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

https://reviewboard.mozilla.org/r/85216/#review85282

> Sorry I actually copypasted these lines of code from desktop to mobile, that is it is desktop specific originally since Android doesn't react to `controlsizechnage` event. So I removed it from Android in this revision.

Late reply/follow-up to this earlier comment about this resize event:

Are you really sure Android doesn't react to / care about this event? It looks like the event is called "resizevideocontrols", and it's got two listeners:
 * one in toolkit/content/widgets/videocontrols.xml
 * one in dom/html/TextTrackManager.cpp
DXR link: https://dxr.mozilla.org/mozilla-central/search?q=resizevideocontrols

Neither of those are obviously desktop-specific to me...

Comment 113

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85216/diff/4-5/

Comment 114

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/16-17/

Comment 115

[Ray Lin[:ralin]]

Comment on attachment 8800232 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85218/diff/4-5/

Comment 116

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

https://reviewboard.mozilla.org/r/85216/#review85282

> Late reply/follow-up to this earlier comment about this resize event:
> 
> Are you really sure Android doesn't react to / care about this event? It looks like the event is called "resizevideocontrols", and it's got two listeners:
>  * one in toolkit/content/widgets/videocontrols.xml
>  * one in dom/html/TextTrackManager.cpp
> DXR link: https://dxr.mozilla.org/mozilla-central/search?q=resizevideocontrols
> 
> Neither of those are obviously desktop-specific to me...

Thanks for reminding me that. Android does care about "resizevideocontrols", but its handler is broken now. (it should be fixed in a followup bug.)

Here I changed back to as your first suggestion in latest revision.

Comment 117

[Daniel Holbert [:dholbert]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

https://reviewboard.mozilla.org/r/85216/#review89054

r=me with nits addressed:

::: layout/generic/nsVideoFrame.cpp:382
(Diff revision 5)
> -
>        ReflowChild(child, aPresContext, kidDesiredSize, kidReflowInput,
>                    mBorderPadding.left, mBorderPadding.top, 0, aStatus);
> +
> +      if (child->GetContent() == mVideoControls && isBSizeShrinkWrapping) {
> +        contentBoxBSize = kidDesiredSize.BSize(wm);

s/wm/myWM/ here.

(Reason: we need to pass in the <video>/<audio> outer frame's writing-mode (which is myWM), not the child's, because:
 (1) contentBoxBSize is declared & used in terms of myWM, not in terms of wm. (And if myWM and wm are orthogonal, then asking for a wm-based value would give you the length in the wrong axis.)
 (2) I believe ReflowOutput (this object) is actually stored in terms of the parent's writing mode, not the child's, by convention. So it'll be *expecting* us to provide it with the parent's writing-mode when we query it. (and IIRC it'll assert if we give it a different writing-mode)
)

ALSO: please do add a crashtest (or if you like & can, a reftest) that would've caught this -- with e.g. <audio controls style="writing-mode: vertical-lr">. I expect that this hypothetical-crashtest would have tripped an assertion with this current version of the patch, and it should not assert once you've addressed this. (So it would've caught this issue automatically.)

::: layout/generic/nsVideoFrame.cpp:601
(Diff revision 5)
> +  if (!HasVideoElement()) {
> +    nsIFrame* kid = mFrames.LastChild();
> +    if (kid) {
> +      result = nsLayoutUtils::IntrinsicForContainer(aRenderingContext,

This needs a comment (and perhaps an assertion) to clarify what we're expecting |kid| to be, here.  (It's a bit hand-wavy to just arbitrarily grab our last child and use it for sizing, without any context about what that child is expected to be.)

ALSO: IMO this would be easier to read if we flipped the logic, so that it's:
 if (HasVideoElement()) {
   [code for when we have video elem]
 } else {
   [code for when we don't have video elem]
 }

::: layout/generic/nsVideoFrame.cpp:621
(Diff revision 5)
> +  if (!HasVideoElement()) {
> +    nsIFrame* kid = mFrames.LastChild();
> +    if (kid) {
> +      result = nsLayoutUtils::IntrinsicForContainer(aRenderingContext,

My above comments for GetMinISize apply here, too (in the similar GetPrefISize code).

Comment 118

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85216/diff/5-6/

Comment 119

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/17-18/

Comment 120

[Ray Lin[:ralin]]

Comment on attachment 8800232 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85218/diff/5-6/

Comment 121

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

https://reviewboard.mozilla.org/r/85216/#review89054

Thank you so much for all the reviews. :)

> This needs a comment (and perhaps an assertion) to clarify what we're expecting |kid| to be, here.  (It's a bit hand-wavy to just arbitrarily grab our last child and use it for sizing, without any context about what that child is expected to be.)
> 
> ALSO: IMO this would be easier to read if we flipped the logic, so that it's:
>  if (HasVideoElement()) {
>    [code for when we have video elem]
>  } else {
>    [code for when we don't have video elem]
>  }

comment added. I'm  not sure we need a assertion here, since it is acceptable to have no controls(kid) if "controls" attribute is not present.

Comment 122

[Ryan VanderMeulen [:RyanVM]]

Please mark all pending issues as resolved in MozReview so this can autoland.

Comment 123

[Ray Lin[:ralin]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #122)
> Please mark all pending issues as resolved in MozReview so this can autoland.

Sorry for the late reply. All issues marked as resolved, thanks.

Comment 124

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/088113647629
Part 1: Remove XUL-specific reflow code of video control. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/02d34b18d76b
Part 2: Desktop video control visual refresh. r=jaws
https://hg.mozilla.org/integration/autoland/rev/235ea1c681db
Part 3. Update tests for video control visual refresh. r=jaws

Comment 125

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/60af44ebfa62 - at least for some builds on some platforms, if not all, that gives warning-as-errors failures from nsVideoFrame.cpp:357:14: error: variable 'size' set but not used [-Werror=unused-but-set-variable], https://treeherder.mozilla.org/logviewer.html#?job_id=6272616&repo=autoland

Comment 126

[Phil Ringnalda (:philor)]

And also an eslint failure, https://treeherder.mozilla.org/logviewer.html#?job_id=6272049&repo=autoland

Comment 127

[Phil Ringnalda (:philor)]

And also a parseable css failure, https://treeherder.mozilla.org/logviewer.html#?job_id=6273846&repo=autoland

Comment 128

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85216/diff/6-7/

Comment 129

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/18-19/

Comment 130

[Ray Lin[:ralin]]

Comment on attachment 8800232 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85218/diff/6-7/

Comment 131

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/19-20/

Comment 132

[Ray Lin[:ralin]]

Comment on attachment 8800232 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85218/diff/7-8/

Comment 133

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85216/diff/7-8/

Comment 134

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/20-21/

Comment 135

[Ray Lin[:ralin]]

Attachment: Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review commit: https://reviewboard.mozilla.org/r/91658/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91658/

Comment 136

[Ray Lin[:ralin]]

Comment on attachment 8808946 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

Sorry for some sort of misoperations that led previous part 3 patch obsoleted....

change to r+ because of no update/changes to this patch, it's exactly the same as previous one.

Comment 137

[Ray Lin[:ralin]]

(In reply to Phil Ringnalda (:philor) from comment #125)

These issues are fixed now, thank you :philor :-)

Comment 138

[Carsten Book [:Tomcat]]

Hi, part 3 still need review + in mozreview to be able to use autoland

Comment 139

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Carsten Book [:Tomcat] from comment #138)
> Hi, part 3 still need review + in mozreview to be able to use autoland

Ray intend to carry over the r+ but MozReview somehow mess up. Can we manually land this? Thanks!

Comment 140

[Carsten Book [:Tomcat]]

Hi Tim, in general because MozReview should be able to Just Work and we should fix it when there are issues.

Comment 141

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8808946 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

https://reviewboard.mozilla.org/r/91658/#review91660

Comment 142

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Update: after discussion with Softvision engineers we decided to land this patch after Aurora 52 branches out, so we would get a full Night 53 cycle to test this out.

Comment 143

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85216/diff/8-9/

Comment 144

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/21-22/

Comment 145

[Ray Lin[:ralin]]

Comment on attachment 8808946 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91658/diff/1-2/

Comment 146

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57317a080c99
Part 1: Remove XUL-specific reflow code of video control. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1d96f03289f8
Part 2: Desktop video control visual refresh. r=jaws
https://hg.mozilla.org/integration/autoland/rev/022a4a018996
Part 3. Update tests for video control visual refresh. r=jaws

Comment 147

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6665745&repo=autoland on Windows 8.

https://hg.mozilla.org/integration/autoland/rev/2be680105fcd756c8789cc38571ad00d4dd20a97

Comment 148

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85216/diff/9-10/

Comment 149

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/22-23/

Comment 150

[Ray Lin[:ralin]]

Comment on attachment 8808946 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91658/diff/2-3/

Comment 151

[Ray Lin[:ralin]]

(In reply to Wes Kocher (:KWierso) from comment #147)
> I had to back these out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=6665745&repo=autoland
> on Windows 8.
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 2be680105fcd756c8789cc38571ad00d4dd20a97

Thank you :KWierso 

Spot the subtle inconsistent width on Windows 8 due to different system default font. Let position check looser but still make sense to every platform. Tested on try server, and ready to land again.

Comment 152

[Ryan VanderMeulen [:RyanVM]]

Autoland couldn't rebase this for landing.

Comment 153

[Ray Lin[:ralin]]

Comment on attachment 8800231 [details]
Bug 1271765 - Part 1: Remove XUL specific reflow code of video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85216/diff/10-11/

Comment 154

[Ray Lin[:ralin]]

Comment on attachment 8781828 [details]
Bug 1271765 - Part 2: Desktop media control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72154/diff/23-24/

Comment 155

[Ray Lin[:ralin]]

Comment on attachment 8808946 [details]
Bug 1271765 - Part 3. Update tests for video control visual refresh.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91658/diff/3-4/

Comment 156

[Ray Lin[:ralin]]

Thanks :RyanVM :)

It seems two eslint related patches landed after my last rebase. Conflicts are solved now. 
Could you help me to land it again? Thanks :-)

ES & M8 Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7755a15d97a40e7d1adb39499e37829e9e398b1a

Comment 157

[Ray Lin[:ralin]]

Sorry 

:%s/M8/M5/

Comment 158

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb010a5d59e7
Part 1: Remove XUL specific reflow code of video control. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/083604641e50
Part 2: Desktop media control visual refresh. r=jaws
https://hg.mozilla.org/integration/autoland/rev/8d2eecb7ea5a
Part 3. Update tests for video control visual refresh. r=jaws

Comment 159

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bb010a5d59e7
https://hg.mozilla.org/mozilla-central/rev/083604641e50
https://hg.mozilla.org/mozilla-central/rev/8d2eecb7ea5a

Comment 160

[areya]

https://hg.mozilla.org/mozilla-central/rev/bb010a5d59e7
https://hg.mozilla.org/mozilla-central/rev/083604641e50
https://hg.mozilla.org/mozilla-central/rev/8d2eecb7ea5a

Comment 161

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

It would be great if QA can help to verify :

1. The basic functions(e.g. play/stop/pause, drag the play progress bar, mute, volume control, full screen) all work properly
2. The warning or contextual hints messages can still be displayed properly when the network is done or when users try to play an un-support medias.

Thanks!

6:44 PM 2. 網路不穩或斷線時 影片是否會顯示讀取中的畫面 or 影片格式不支援時是否會出現錯誤訊息 (這點比較難 test....

Comment 162

[Justin Dolske [:Dolske]]

Congrats on getting this landed!

Comment 163

[Sylvestre Ledru [:Sylvestre]]

Release Note Request (optional, but appreciated)
[Why is this notable]: New user facing change
[Affects Firefox for Android]: No, this is done in 52 if I remember correctly
[Suggested wording]: Refresh of the medial controls user interface
[Links (documentation, blog post, etc)]: ?

Comment 164

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx53 Aurora release notes.

Comment 165

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Hello! Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1340523#c4, https://bugzilla.mozilla.org/show_bug.cgi?id=1340522#c2, https://bugzilla.mozilla.org/show_bug.cgi?id=1340479#c1 and https://bugzilla.mozilla.org/show_bug.cgi?id=1340460#c1, the spec document https://drive.google.com/file/d/0B4K8q1qWmtAvM3ZXVUlHRkdRVVU/view is obsolete. Can you please provide an updated one? Thanks!

Comment 166

[Ray Lin[:ralin]]

Attachment: Media Control spec_0.4.pdf

Comment 169

[Andrei Vaida [:avaida]]

[Test Plan]:
https://wiki.mozilla.org/QA/Video_Play_Visual_Refresh

Comment 170

[Liz Henry (:lizzard) (relman/hg->git project)]

Dolske, jaws, there are still more regressions from this popping up. Do you think any of them should  block this moving to release? Are you comfortable with the amount of testing we have done and the sorts of regressions we're seeing? I can keep uplifting fixes next week, but would like to double check with you this is still in a good state for 53.

Comment 171

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #170)
> Dolske, jaws, there are still more regressions from this popping up. Do you
> think any of them should  block this moving to release? Are you comfortable
> with the amount of testing we have done and the sorts of regressions we're
> seeing? I can keep uplifting fixes next week, but would like to double check
> with you this is still in a good state for 53.

We (Ray/Vance/Luke/Evelyn/me) had a brief discussion within Taipei at the beginning of the week. We think the current regressions are manageable and should not be considered blocking. Obviously dolske or jaws or releng can decide otherwise and we will respect that.

Comment 172

[:Gijs (out for now; he/him)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #171)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #170)
> > Dolske, jaws, there are still more regressions from this popping up. Do you
> > think any of them should  block this moving to release? Are you comfortable
> > with the amount of testing we have done and the sorts of regressions we're
> > seeing? I can keep uplifting fixes next week, but would like to double check
> > with you this is still in a good state for 53.
> 
> We (Ray/Vance/Luke/Evelyn/me) had a brief discussion within Taipei at the
> beginning of the week. We think the current regressions are manageable and
> should not be considered blocking. Obviously dolske or jaws or releng can
> decide otherwise and we will respect that.

I concur with this, and would also add that I think at this point, the risk of backing out *all* of the relevant patches also seems nontrivial, which is an additional reason to leave this in for 53.

Comment 173

[Jared Wein [:jaws] (please needinfo? me)]

I agree with Tim and Gijs. At this point, I would be more worried of what the backout would mean versus getting more attention on the various bugs that have been found.

Comment 174

[684sigma]

Bug 1352879 and Bug 1346432 seem to be caused by this bug too.