add camera button to file input elements

RESOLVED FIXED in mozilla2.0b3

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: blassey, Assigned: khuey)

Tracking

(Blocks: 1 bug)

unspecified
mozilla2.0b3
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
in-litmus ?

Firefox Tracking Flags

(fennec2.0+)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Comment hidden (empty)
Created attachment 446682 [details] [diff] [review]
patch

this patch needs to be localized and probably needs some UI love, but it works.
Assignee: nobody → blassey.bugs
Attachment #446682 - Flags: review?
(Reporter)

Updated

7 years ago
Attachment #446682 - Flags: review? → review?(jonas)
(Reporter)

Updated

7 years ago
Blocks: 451674
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 on attachment 446682 [details] [diff] [review]
patch

Clearing this request for now. See comment #2
Attachment #446682 - Flags: review?(jonas)
Created attachment 451760 [details] [diff] [review]
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?
Assignee: blassey.bugs → me
Attachment #446682 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #451760 - Flags: superreview?(roc)
Attachment #451760 - Flags: review?(jonas)
Attachment #451760 - Attachment is obsolete: true
Attachment #451760 - Flags: superreview?(roc)
Attachment #451760 - Flags: review?(jonas)
Created attachment 451762 [details] [diff] [review]
Patch

Doh! forgot to qrefresh.
Attachment #451762 - Flags: superreview?(roc)
Attachment #451762 - Flags: review?(jonas)
Created attachment 452151 [details] [diff] [review]
Patch

This appears to pass tests.  There was a lot of noise on try :-(
Attachment #451762 - Attachment is obsolete: true
Attachment #452151 - Flags: superreview?(roc)
Attachment #452151 - Flags: review?(jonas)
Attachment #451762 - Flags: superreview?(roc)
Attachment #451762 - Flags: review?(jonas)
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?
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.
Attachment #452151 - Attachment is obsolete: true
Attachment #452151 - Flags: superreview?(roc)
Attachment #452151 - Flags: review?(jonas)
Created attachment 453877 [details] [diff] [review]
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.
Attachment #453877 - Flags: superreview?(roc)
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)"?
Because those modes would be different than the other modes (for determining what to capture).  Other than that I have no problem changing it.
+   * @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?
Yeah ok it does make more sense to do it that way.
I spun off parsing the accept attribute properly (as the HTML 5 spec says to) into Bug 574570.
Depends on: 574570
Created attachment 454111 [details] [diff] [review]
Patch
Attachment #453877 - Attachment is obsolete: true
Attachment #454111 - Flags: superreview?(roc)
Attachment #454111 - Flags: review?(jonas)
Attachment #453877 - Flags: superreview?(roc)
+   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 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.
Attachment #454111 - Flags: review?(jonas) → review+
Comment on attachment 454111 [details] [diff] [review]
Patch

This needs some minor updates for my new patch on the blocking bug.
Attachment #454111 - Attachment is obsolete: true
Attachment #454111 - Flags: superreview?(roc)
Created attachment 459096 [details] [diff] [review]
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.
Attachment #459096 - Flags: superreview?
Attachment #459096 - Flags: review+
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Attachment #459096 - Flags: superreview? → superreview?(roc)
Attachment #459096 - Flags: superreview?(roc) → superreview+
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.)
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
Backed out due to failures.

http://hg.mozilla.org/mozilla-central/rev/6c7a682b3faf
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Flags: in-testsuite?
Flags: in-litmus?
Blocks: 593891

Updated

6 years ago
No longer blocks: 451674

Updated

6 years ago
Blocks: 692955
Depends on: 853475
You need to log in before you can comment on or make changes to this bug.