567323 - add camera button to file input elements

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this patch needs to be localized and probably needs some UI love, but it works.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We only want to do this if accept="image/*" is there, right? Also, can we query to check if camera is really available. Seems silly to display the button when it's not.

We possibly also don't want to change the size of the <input> when displaying the camera button. That will likely break a lot of pages unfortunately.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 446682 [details] [diff] [review]
patch

Clearing this request for now. See comment #2

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This patch creates two interfaces, one to tell layout whether or not to display the Capture button (nsICaptureService) and another to display the actual capture dialog (nsICapturePicker).  This keeps the button from displaying on platforms where there is no capture support or where the capture service determines that there's no mic/camera attached.

Roc, can you sr the interfaces here?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Doh! forgot to qrefresh.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This appears to pass tests.  There was a lot of noise on try :-(

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think these interfaces should not live in widget. They don't really have anything to do with widgets. How about putting the implementations into toolkit/system or dom/system and the interfaces into xpcom/system or even layout/forms?

Can nsICapturePicker describe what sort of URI will be returned?

addParameter is very vague. Can we at least make it take the parameter name and value as separate string parameters? And document what some allowed parameters are? Ideally we would have proper attributes for setting parameters, e.g. a "type" attribute and a "frameRate" attribute.

Would it make sense to combine nsICaptureService with nsICapturePicker, and just instantiate nsICapturePicker and then interrogate it by asking what modes it supports, e.g. with a supportsMode(mode) method?

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The reason I put them in widget is because that's where nsIFilePicker lives.  I'm fine with moving them somewhere else.

We want nsICapturePicker to be able to return at least data, file, and moz-filedata URIs.  I can add that to the interface comments.

I'll see if I can come up with a list of relevant parameters we'd want.  I think at first all we're really interested in is the type.

The reason I have them as two separate interfaces at the moment is because determining whether or not you can capture content is different from actually capturing it, but I don't really have strong feelings either way.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Roc, I've addressed the comments on the interface.  Mind taking another look at it?

The actual code here needs some work, in particular we need to parse accept="" in an HTML 5 compliant manner.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks good, just one comment:

+  readonly attribute boolean stillsMaybeAvailable;
+  readonly attribute boolean audioMaybeAvailable;
+  readonly attribute boolean videoMaybeAvailable;

Why not just one method, "boolean modeMaybeAvailable(long mode)"?

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Because those modes would be different than the other modes (for determining what to capture).  Other than that I have no problem changing it.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+   * @param      flags      Mode and type flags for what to capture
+  void init(in nsIDOMWindow parent, in AString title, in long flags);

Shouldn't we call flags 'mode' instead?

Why would the mode in modeMaybeAvailable be different to the modes you pass into 'init'? Seems to me modeMaybeAvailable(mode) is going to return false when init(mode) is guaranteed to fail, right? Your modes are

+  const long modeStill = 0;            // Capture a still (image)
+  const long modeAudioClip = 1;        // Capture a clip of audio
+  const long modeVideoClip = 2;        // Capture a clip of video
+  const long modeVideoNoSoundClip = 3; // Capture a clip of video (no sound)

and your attributes are

+  readonly attribute boolean stillsMaybeAvailable;
+  readonly attribute boolean audioMaybeAvailable;
+  readonly attribute boolean videoMaybeAvailable;

So the only 'missing' attribute is videoNoSoundClipMaybeAvailable. Why we wouldn't we want to be able to ask about that? Surely there could be devices that don't have audio input but can capture video?

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah ok it does make more sense to do it that way.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I spun off parsing the accept attribute properly (as the HTML 5 spec says to) into Bug 574570.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+   boolean modeMaybeAvailable(in unsigned long mode);

2-space indent, and probably should be modeMayBeAvailable.

+  const long modeStill = 0;            // Capture a still (image)
+  const long modeAudioClip = 1;        // Capture a clip of audio
+  const long modeVideoClip = 2;        // Capture a clip of video
+  const long modeVideoNoSoundClip = 3; // Capture a clip of video (no sound)
+
+  // Return codes from the dialog
+  const long returnOK = 0;
+  const long returnCancel = 1;

Should probably be MODE_STIL, etc, and "OK" and "CANCEL".

+                        NS_LITERAL_STRING("Capture"), PR_FALSE);

"capture"

+  // only allow the left button
+  nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aMouseEvent);
+  nsCOMPtr<nsIDOMNSUIEvent> uiEvent = do_QueryInterface(aMouseEvent);
+  NS_ENSURE_STATE(uiEvent);
+  PRBool defaultPrevented = PR_FALSE;
+  uiEvent->GetPreventDefault(&defaultPrevented);
+  if (defaultPrevented) {
+    return NS_OK;
+  }
+
+  PRUint16 whichButton;
+  if (NS_FAILED(mouseEvent->GetButton(&whichButton)) || whichButton != 0) {
+    return NS_OK;
+  }
+
+  PRInt32 clickCount;
+  if (NS_FAILED(mouseEvent->GetDetail(&clickCount)) || clickCount > 1) {
+    return NS_OK;
+  }

Move this into a helper function that both mouse listeners can share?

+    nsCOMPtr<nsIFileURL> fileURL = do_QueryInterface(uri);
+    if (!fileURL)
+      return NS_ERROR_UNEXPECTED;

Is there some plan for follow-up work to support non-file URLs?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 454111 [details] [diff] [review]
Patch

This is looking great, though definitely want a layout person to look over the frame related changes.

Though I would *love* it if we could get someone in UX to design a better UI so that we can enable 'capture' on all input elements.

Another thing to keep in mind is that I think that on most desktop platforms, it's possible to extend the standard filepicker to add buttons. We could use that to add a "take picture" button in the normal filepicker.

This would definitely involve a lot of platform specific hacking though, so something for a separate bug.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 454111 [details] [diff] [review]
Patch

This needs some minor updates for my new patch on the blocking bug.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Carrying forward sicking's r+ because the changes are minor.

Addressed roc's comments.

There is a plan to support non-file URIs but that requires teaching the related code in content how do deal with things that aren't nsIFiles.

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

FWIW, as new is infallible, lines like

  NS_ENSURE_TRUE(mMouseListener, NS_ERROR_OUT_OF_MEMORY);
  NS_ENSURE_TRUE(mCaptureMouseListener, NS_ERROR_OUT_OF_MEMORY);

aren't necessary. (Asserting that could be useful, of course.)

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #20)
> FWIW, as new is infallible, lines like
> 
>   NS_ENSURE_TRUE(mMouseListener, NS_ERROR_OUT_OF_MEMORY);
>   NS_ENSURE_TRUE(mCaptureMouseListener, NS_ERROR_OUT_OF_MEMORY);
> 
> aren't necessary. (Asserting that could be useful, of course.)

Hit these.


http://hg.mozilla.org/mozilla-central/rev/c765493e9530

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out due to failures.

http://hg.mozilla.org/mozilla-central/rev/6c7a682b3faf

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Relanded with simple changes.  Factoring out the common code removed the abort early for non-left buttons, so I changed that method to return a boolean indicating whether or not to process the click.

http://hg.mozilla.org/mozilla-central/rev/8ec5010204bc