828936 - [Video] Cannot remove a video

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect, P2)

Description

[David Teller [:Yoric] - still alive but not very active]

Steps to reproduce:
- transfer a video file to directory Download/ of the phone;
- open the video application;
- select "remove video" and confirm;
- video is still there.

Tested in French with 20130108070203.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I can reproduce.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Alexandre is already working on it. Assigning to him.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 7503

So the issue is that BrowserElementChild fires a click event from here:
  this._ctxHandlers[data.json.menuitem].click();
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChild.js#583

But this click event is killed by mouse event shim here:
  window.addEventListener('click', discardEvent, true);
  function discardEvent(e) {
    if (e.isTrusted) {
      e.stopImmediatePropagation(); // so it goes no further
      if (e.type === 'click')
        e.preventDefault(); // so it doesn't trigger a change event
    }
  }
https://github.com/mozilla-b2g/gaia/blob/master/shared/js/mouse_event_shim.js#L45-L53

So that the <menuitem> element from video app never gets its `click` event fired:
dom.deleteConfirmationButton.addEventListener('click',
                                                deleteSelectedVideoFile, false);
https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/video.js#L85-L86

    <menu type="context" id="thumbnail-menu">
      <menuitem label="Delete Video" data-l10n-id="delete-video" id="delete-confirmation-button"></menuitem>
    </menu>
https://github.com/mozilla-b2g/gaia/blob/master/apps/video/index.html#L15-L17



I'm just whitelisting menuitem click events in order to keep it simple.

Comment 4

[David Flanagan [:djf]]

I commented in github, but repeating here: it is too risky to make modifications to mouse_event_shim.

We should add documentation that it is not suitable for apps that use context menus.

And then, we should do one of these:

1) Modify the Video UX to add real delete buttons rather than hiding them in context menus. The current delete UX kind of sucks anyway.

2) Modify the video app so that it does not use the shim. This means porting the seek control/scrubber to use touch events.

I can do either one of these if my help is needed.

Comment 5

[David Flanagan [:djf]]

Taking this bug at Vivien's request

Comment 6

[David Flanagan [:djf]]

Attachment: link to patch on github

Dale,

This is a blocker for you to review.

The issue is that context menus apparently worked by sending a single click event to the context menu button. The mouse_event_shim we were using discards that event but can't synthetize a new one because no touch events are sent along with it.

I considered porting the app so the shim wouldn't be necessary.  But instead, I took an easier route. Since deletion is the only option in the context menu, you were essentially asking users to confirm the deletion twice.  So I just got rid of the context menu.  Now when we get a contextmenu event, the code goes straight to the confirm dialog.  Since there is no contextmenu element anymore, the click bug is no longer there.

In the process of fixing this, I discovered 829214, and included a workaround in this patch.

Comment 7

[Dale Harvey (:daleharvey)]

Comment on attachment 700631 [details]
link to patch on github

The contextmenu with a single prompt was strange, however now the user is getting a confirmation prompt for an action they never explicitly chose. I think both approaches really deserve a UI review and a different implementation, but as this fixes the issue and both arent really suitable happy to get this fixed. Cheers David

r+