[Video] Cannot remove a video

RESOLVED FIXED in B2G C4 (2jan on)

Status

Firefox OS
Gaia::Video
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: djf)

Tracking

({regression})

unspecified
B2G C4 (2jan on)
x86
Mac OS X
regression

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

108 bytes, text/html
daleharvey
: review+
Details
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.
blocking-basecamp: ? → +
Keywords: regression
Priority: -- → P2
Target Milestone: --- → B2G C4 (2jan on)
Alexandre is already working on it. Assigning to him.
Assignee: nobody → poirot.alex
Component: Gaia::Video → General
QA Contact: mozillamarcia.knous
Component: General → Gaia::Video
QA Contact: mozillamarcia.knous
Created attachment 700442 [details]
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.
Attachment #700442 - Flags: review?(21)
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Taking this bug at Vivien's request
Assignee: poirot.alex → dflanagan
(Assignee)

Comment 6

5 years ago
Created attachment 700631 [details]
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.
Attachment #700442 - Attachment is obsolete: true
Attachment #700631 - Flags: review?(dale)
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+
Attachment #700631 - Flags: review?(dale) → review+
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.