Closed Bug 694696 Opened 13 years ago Closed 10 years ago

If HTML5 Volume is changed to minimum by arrow key, the icon of volume control should be indicated as 'Mute'

Categories

(Toolkit :: Video/Audio Controls, enhancement)

x86
Windows 7
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: alice0775, Assigned: tomasz, Mentored)

References

()

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 5 obsolete files)

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/9545b88eed82
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111014 Firefox/10.0a1 ID:20111014030948

If Volume is changed to minimum by arrow key, the icon of volume control should be indicated  as 'Mute'.

Reproducible: Always

Steps to Reproduce:
1. Start Firefox with clean profile
2  Open URL 
3. Click HTML5 Video to focus
4. Press and hold a "Down arrow key"

Actual Results:

  The icon of volume control does not change.

Expected Results:
   When volume is minimize, the icon of volume control should be indicated  as 'Mute'
Summary: If Volume is changed to minimum by arrow key, the icon of volume control should be indicated as 'Mute' → If HTML5 Volume is changed to minimum by arrow key, the icon of volume control should be indicated as 'Mute'
Severity: normal → enhancement
Hardware: x86_64 → x86
Flags: needinfo?(mark.finkle)
I'm not sure what this needinfo is for, but I agree that this is something we should fix. If there is a specific question for Mark, please re-add the needinfo and include some context here.
Flags: needinfo?(mark.finkle)
Reproducibility: 100%
Product: FFOS device

Steps:
1. Start the Youtube app or open the Youtube website on firefox browser using a FFOS device.
2. Play a movie and press the side volume-down key and make the video volume to be 0.
3. Touch the movie area in order to display the "controlsOverlay" and see the Mute button.
   (gecko/toolkit/content/widgets/videocontrols.xml)

Expected result:
- Because the actual volume of the movie is 0, the mute icon's state should be also "muted" (blue icon).

Actual result: 
- The mute button does not indicates "muted."


We expect that when we make the movie's volume to be 0, the mute button's state is also "Muted" (blue icon).

I think that the scenario about mute icon below would be good while playing video on the web.

1. Volume is downed to 0 by side volume key, the color of the mute icon also changed to blue.
2. muted state can be made by two different actions - pressing volume down key and touching the mute icon on the "controlsOverlay." 

3-1. muted by volume key
  internal volume: 0, 
  mute icon: does not work
  pressing volume up key: unmuted, volume increases from 0

3-2. muted by mute icon on the "controlsOverlay"
  internal volume: previous volume (but media volume is 0)
  mute icon: can work (if user touch the mute icon, unmuted.)
  pressing volume up or down key: unmuted, volume changes from the previous volume.
Flags: needinfo?(mark.finkle)
Flags: needinfo?(jaws)
I don't think this is your bug. Gaia uses a lot of code unrelated to Firefox desktop. It would be best to file a new bug with your STR in Firefox OS:Gaia Video.
Let's make this a mentor bug. Comment 2 does a good job providing a starting point for the spec. 
Calling all contributors!
Flags: needinfo?(mark.finkle)
Flags: needinfo?(jaws)
Whiteboard: [mentor=wesj][mentor=jaws][lang=js]
Mentor: wjohnston, jaws
Whiteboard: [mentor=wesj][mentor=jaws][lang=js] → [lang=js]
At first time, I thought modifying some javascript code in videocontrols.xml would fix this bug, but it did not work. 
(gecko/toolkit/content/widgets/videocontrols.xml)

Next, I tried on HTMLMediaElement.cpp, but I could not find the volume change logic by side volume key.
(gecko/content/html/content/src/HTMLMediaElement.cpp)

I hope I can find the point to implement the scenario I've suggested.
(In reply to easynow11 from comment #5)
> At first time, I thought modifying some javascript code in videocontrols.xml
> would fix this bug, but it did not work. 
> (gecko/toolkit/content/widgets/videocontrols.xml)
> 
> Next, I tried on HTMLMediaElement.cpp, but I could not find the volume
> change logic by side volume key.
> (gecko/content/html/content/src/HTMLMediaElement.cpp)
> 
> I hope I can find the point to implement the scenario I've suggested.

The code that you want to change is at http://hg.mozilla.org/mozilla-central/file/4f8f2ec43ef6/toolkit/content/widgets/videocontrols.xml#l1285

We handle the 'down' key at that point in the file, and it's also where we unmute the media. At this point, we should change:
> this.video.muted = false;
to:
> this.video.muted = !this.video.volume;

With this change, when the volume is 0 the negation will convert it to true, and if the volume is non-zero then the negation will convert the value to false.
(In reply to Jared Wein [:jaws] [Away for most of July] (please needinfo? me) from comment #6)
> (In reply to easynow11 from comment #5)
> 
> The code that you want to change is at
> http://hg.mozilla.org/mozilla-central/file/4f8f2ec43ef6/toolkit/content/
> widgets/videocontrols.xml#l1285
> 

Thanks for your advice.

I've changed the code as you mentioned, but it did not work as I expected.
I wonder that the event by pressing side volume up/down keys on FFOS device (phone) is different from the event by pressing up/down arrow keys on the desktop.

I've added the "console.log" at 
http://hg.mozilla.org/mozilla-central/file/4f8f2ec43ef6/toolkit/content/widgets/videocontrols.xml#l1277

to see the keystroke events, but logs expected was not printed on ddms.
Thanks for your advice.
(I'm worry about that I've made English sentences with bad manners in comment 7.)

But, it does not work.
I'm afraid that side volume keys on the ffos device and arrow keys on the desktop make different events.

I've added "console.log" codes at
http://hg.mozilla.org/mozilla-central/file/4f8f2ec43ef6/toolkit/content/widgets/videocontrols.xml#l1277

but logs I've added were not printed on ddms.
(Primally reported bug was on the desktop browser, but now, the bug is on the ffos device.)
Bug 674739 added support for VK_VOLUME_MUTE, VK_VOLUME_DOWN, and VK_VOLUME_UP, but only for Android and Windows. You will probably need to follow the patches in that bug to see how you can add support for these keys on FirefoxOS, and then add support for those keyCodes to the videocontrols.xml file.
What I found is like below:

** side volume keys' event flow

 - side volume key's key code: DOM_VK_PAGE_UP/DOWN

 1) when pressing, filterHardwareKeys (in shell.js) receives the key-press event and send it as mozChromeEvent
 2) This mozChromeEvent is received by buttonEventHandler (in hardware_buttons.js)
 3) pressing & releasing button -> converted as 'volumeup' or 'volumedown' event and fired by window.dispatchEvent().
 4) "sound_manager.js" receives this event and changes the "volume setting" of the content channel. 
    It does not change current content's volume itself, but volume setting.
 5) The side volume keys' event ends at this point.

The functions in videocontrols.xml do not receive the key event directly.
I think that they just see the volume in sound settings.
So I think that it is needed to pass these key events to the videocontrol for detecting the change of volume.

Isn't it possible that system app (sound_manager.js) sends a event when changeVolume function is called and videoconstrols.xml receives and handles this event ?

I'm afraid that I have no idea about what kind of event should be used.
I want to work on it. As there was no activity in the last month I assumed that you're not working on it anymore easynow11@gmail.com. If you are I'm sorry for duplicating your work. Feel free to reclaim this bug :-)
Assignee: nobody → tkolodziejski
Attached patch video-volume-test.patch (obsolete) — Splinter Review
This is a patch with a test that fails (as the new proposed behavior is a little different, see https://bugzilla.mozilla.org/show_bug.cgi?id=694696#c2).
Attachment #8474795 - Flags: review?(jaws)
Attached patch video-volume.patch (obsolete) — Splinter Review
And here's the proposed fix.
Attachment #8474799 - Flags: review?(jaws)
Please keep in mind that I only tested it on desktop. I'm not really sure how to compile/run/test on mobile device hence need more info.

BTW, is this ok to have those small comments? Maybe it's possible to add two attachments in one comment?
Status: NEW → ASSIGNED
Comment on attachment 8474799 [details] [diff] [review]
video-volume.patch

Review of attachment 8474799 [details] [diff] [review]:
-----------------------------------------------------------------

It is fine that these are kept as separate patches. There is no way to upload multiple patches as a single comment, no worries :)

::: toolkit/content/widgets/videocontrols.xml
@@ +1290,5 @@
>                                  break;
>                              case "downArrow": /* Volume decrease */
>                                  oldval = this.video.volume;
>                                  this.video.volume = (oldval < 0.1 ? 0 : oldval - 0.1);
> +                                this.video.muted = !this.video.volume;

We shouldn't be setting this muted property here, because as your test shows it will cause another volumechange event to fire.

We should instead just make the UI show that there is no volume, which will look the same as muted. To do this, you'll want to call setMuteButtonState(true) when the volumechange event happens and the volume is 0.
Attachment #8474799 - Flags: review?(jaws) → review-
Attached patch video-volume.patch (obsolete) — Splinter Review
Ok. So I switched the logic of the mute button state to say whether the effective volume is 0, that is: either volume is 0 or video is muted.

However, I need more info how to test it. I expected it to work as in the attached patch but it looks like there's no getAnonymousElementByAttribute in there.
Attachment #8474795 - Attachment is obsolete: true
Attachment #8474799 - Attachment is obsolete: true
Attached patch video-volume.patch (obsolete) — Splinter Review
I have changed the existing test to chrome test as we needed chrome powers to get into the anonymous content inside. The reason I changed the test instead of adding a new one is that I didn't want to copy-paste the constants like muteButtonCenterX as well as the whole local test style. If you want me to move it to separate test let me know.
Attachment #8475617 - Attachment is obsolete: true
Attachment #8477027 - Flags: review?(jaws)
Comment on attachment 8477027 [details] [diff] [review]
video-volume.patch

Review of attachment 8477027 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/tests/widgets/chrome.ini
@@ +2,5 @@
>  support-files =
>    tree_shared.js
>    popup_shared.js
>    window_menubar.xul
> +  ../../../../content/media/test/seek_with_sound.ogg

We should `hg copy` this file to this directory since our dependency on seek_with_sound.ogg won't be obvious to /content/ developers and it is fragile if the file gets moved.

::: toolkit/content/tests/widgets/test_videocontrols.html
@@ +48,5 @@
> +var testnum = 1;
> +var video = document.getElementById("video");
> +
> +function clickMuteButton() {
> +  synthesizeMouse(video, muteButtonCenterX, muteButtonCenterY, { });

Pulling this out to a separate function doesn't seem to help readability. synthesizeMouse is used elsewhere within this file to click on a button, so we should just continue to use synthesizeMoue directly instead of wrapping it with a `clickMuteButton` function.

@@ +56,5 @@
> +  var domUtil = Components.classes["@mozilla.org/inspector/dom-utils;1"]
> +                          .getService(Components.interfaces.inIDOMUtils);
> +  var kids = domUtil.getChildrenForNode(video, true);
> +  var videocontrols = kids[1];
> +  var muteButton = document.getAnonymousElementByAttribute(videocontrols, 'class', 'muteButton');

Refactor this function to:
> function getButtonByAttribute(aName, aValue) {
>   var domUtil = Components.classes["@mozilla.org/inspector/dom-utils;1"]
>                           .getService(Components.interfaces.inIDOMUtils);
>   var kids = domUtil.getChildrenForNode(video, true);
>   var videocontrols = kids[1];
>   return document.getAnonymousElementByAttribute(videocontrols, aName, aValue);
> }
this way it can get used by other tests within this file.

@@ +236,5 @@
> +
> +      // Volume is down to 0 and we expect the mute button do nothing.
> +      clickMuteButton();
> +
> +      is(video.volume, 0, "Volume still 0.");

We should make volume go to 0.5 when the button is clicked and it appears as "muted" prior to the click.

So make this test do:
is(video.volume, 0, "Volume should be 0.");
ok(isMuteButtonMuted(), "Mute button says it's muted");
synthesizeKey("VK_UP", {});

case XX:
is(event.type, "volumechange", "checking event type");
is(video.volume, 0.1, "Volume is increased");
ok(!isMuteButtonMuted(), "Mute button says it's not muted");
synthesizeKey("VK_DOWN", {});

case XX:
is(event.type, "volumechange", "checking event type");
is(video.volume, 0, "Volume should be 0.");
ok(isMuteButtonMuted(), "Mute button says it's muted");
synthesizeMouse(video, muteButtonCenterX, muteButtonCenterY, { });

case XX:
is(event.type, "volumechange", "checking event type");
is(video.volume, 0.5, "Volume should be 0.5");
ok(!isMuteButtonMuted(), "Mute button says it's not muted");
Attachment #8477027 - Flags: review?(jaws) → feedback+
As some background, YouTube has the following behavior:
Volume decreased to 0 -> click on volume button -> no visible action -> click on volume button -> volume gets changed to about 0.1

So YouTube appears to unmute to a minimum of 0.1, but changes to "muted" when the volume button is clicked regardless of the current volume.
Attached patch video-volume.patch (obsolete) — Splinter Review
I followed your suggestions jaws and also I added some more test cases. It should cover all the mute/unmute cases.

I also extracted the isEffectivelyMuted function which should make it clear how it's supposed to work.

Please have a look.
Attachment #8477027 - Attachment is obsolete: true
Attachment #8478466 - Flags: review?(jaws)
Comment on attachment 8478466 [details] [diff] [review]
video-volume.patch

Review of attachment 8478466 [details] [diff] [review]:
-----------------------------------------------------------------

Do you have access to tryserver? Can you push this patch there and make sure all tests still pass on all platforms (win32,macosx64,linux)?

r+ if all tests pass and with the following issues addressed.

::: toolkit/content/tests/widgets/test_videocontrols.html
@@ +278,5 @@
> +      ok(video.muted, "Video is muted.");
> +
> +      ok(isMuteButtonMuted(), "Mute button says it's muted");
> +
> +      synthesizeMouse(video, muteButtonCenterX, muteButtonCenterY, { });      

nit, trailing whitespace

::: toolkit/content/widgets/videocontrols.xml
@@ +1041,5 @@
>                          return;
>                      }
> +                    this.video.muted = !this.isEffectivelyMuted(); 
> +                    if (this.video.volume === 0) {
> +                      this.video.volume = 0.5; 

nit, trailing whitespace here and two lines above.

@@ +1137,5 @@
> +                  return this.video.muted || !this.video.volume;
> +                },
> +
> +                updateMuteButtonState : function() {
> +                  var aMuted = this.isEffectivelyMuted();

This variable should be renamed from `aMuted` to `muted`. We reserve the "aVariable" convention for arguments to the function.
Attachment #8478466 - Flags: review?(jaws) → review+
Just addressing the style issues.
Attachment #8478466 - Attachment is obsolete: true
Attachment #8478558 - Flags: review+
Thank you jaws for your help in this bug!
https://hg.mozilla.org/mozilla-central/rev/56672382b34b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1274104
You need to log in before you can comment on or make changes to this bug.