If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Parse accept="foo" in accordance with the HTML5 spec.

RESOLVED FIXED in mozilla2.0b3

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

({html5})

unspecified
mozilla2.0b3
html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Patch coming up.
Created attachment 453967 [details] [diff] [review]
Patch

This applies on top of Mounir's patch in Bug 565272.  This parses the accept attribute in accordance with the HTML 5 spec[0] and adds tests for that behavior.

[0] http://www.whatwg.org/specs/web-apps/current-work/multipage/number-state.html#attr-input-accept
Attachment #453967 - Flags: review?(jonas)
Attachment #453967 - Flags: feedback?(mounir.lamouri)
Blocks: 567323

Comment 2

7 years ago
Can't you use nsCharSeparatedTokenizer attributeTokenizer(aAttribute, ','); to parse it?
(In reply to comment #2)
> Can't you use nsCharSeparatedTokenizer attributeTokenizer(aAttribute, ','); to
> parse it?

I could have if I had known it existed :-)  I will change the patch.
Created attachment 454073 [details] [diff] [review]
Patch

Now with nsCharSeparatedTokenizer goodness.
Attachment #453967 - Attachment is obsolete: true
Attachment #454073 - Flags: superreview?(mounir.lamouri)
Attachment #454073 - Flags: review?(jonas)
Attachment #453967 - Flags: review?(jonas)
Attachment #453967 - Flags: feedback?(mounir.lamouri)
Attachment #454073 - Flags: superreview?(mounir.lamouri) → feedback?(mounir.lamouri)

Updated

7 years ago
Keywords: html5
OS: Linux → All
Hardware: x86 → All
Comment on attachment 454073 [details] [diff] [review]
Patch

># HG changeset patch
># User Kyle Huey <me@kylehuey.com>
>
>Bug 574570: Parse input tag's accept attribute in accordance with the HTML 5 spec. r?=sicking
>
>diff --git a/layout/forms/nsFileControlFrame.cpp b/layout/forms/nsFileControlFrame.cpp
>--- a/layout/forms/nsFileControlFrame.cpp
>+++ b/layout/forms/nsFileControlFrame.cpp
>@@ -848,30 +849,58 @@ NS_IMETHODIMP nsFileControlFrame::GetAcc
>   nsCOMPtr<nsIAccessibilityService> accService = do_GetService("@mozilla.org/accessibilityService;1");
>   if (!accService)
>     return NS_ERROR_FAILURE;
> 
>   return accService->CreateHTMLGenericAccessible(static_cast<nsIFrame*>(this), aAccessible);
> }
> #endif
> 
>+nsTArray<nsString>*
>+nsFileControlFrame::ParseAcceptAttribute() const
>+{
>+  nsTArray<nsString>* array;
>+  nsString accept;
>+  nsString substring;
>+  if (!mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::accept, accept))
>+    return NULL; // No accept attribute present
>+
>+  // accept=""?  If so, treat as if it were not set
>+  if (accept.IsEmpty())
>+    return NULL;

You can do:
if (!mContent->GetAttr(...) || accept.IsEmpty())

In addition, the coding style requires you to add braces even for a single-line block.

>+
>+  array = new nsTArray<nsString>();

You should do array declaration here (not at the beginning of the method).
By the way, |substring| doesn't look needed (I suppose it comes from the previous version of the patch).

>+  nsCharSeparatedTokenizer 
>+    tokenizer(accept, ',', nsCharSeparatedTokenizer::SEPARATOR_OPTIONAL);

This is wrong. AFAICT, the separator isn't optional and you need to check for HTML5 spaces. I've wrote the needed code for <input type='email'>. You should add it as a dependency (bug 555559) and create the tokenizer like this:
nsCharSeparatedTokenizerTemplate<nsContentUtils::IsHTMLWhitespace> tokenizer(aValue, ',');

>+  while (tokenizer.hasMoreTokens())
>+    array->AppendElement(tokenizer.nextToken());

Add braces here too, please.

>+
>+  return array;
>+}
>+
> PRInt32
> nsFileControlFrame::GetFileFilterFromAccept() const
> {
>-  nsAutoString accept;
>-  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::accept, accept);
>+  PRInt32 result = 0;
>+  nsAutoPtr<nsTArray<nsString> > acceptArray;
>+  acceptArray = ParseAcceptAttribute();
>+  if (!acceptArray)
>+    return result;

I think you can inline ParseAcceptAttribute into GetFileFilterFromAccept. I don't see why this function could be needed somewhere else and having it inlined could be helpful (like preventing the creation of the array).

>diff --git a/layout/forms/test/test_bug377624.html b/layout/forms/test/test_bug377624.html
>--- a/layout/forms/test/test_bug377624.html
>+++ b/layout/forms/test/test_bug377624.html
>@@ -12,16 +12,23 @@ https://bugzilla.mozilla.org/show_bug.cg
> </head>
> <body onload="runTests();">
> <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=377624">Mozilla Bug 377624</a>
> <p id="display"></p>
> <div id="content">
>   <input id='a' type='file' accept="image/*">
>   <input id='b' type='file' accept="audio/*">
>   <input id='c' type='file' accept="video/*">
>+  <input id='d' type='file' accept="image/*,audio/*">
>+  <input id='e' type='file' accept="image/*,video/*">
>+  <input id='f' type='file' accept="audio/*,video/*">
>+  <input id='g' type='file' accept="image/*,audio/*,video/*">
>+  <input id='h' type='file' accept="foo/baz,image/*,bogus/duh">
>+  <input id='i' type='file' accept="mime/type;parameter,video/*">
>+  <input id='z' type='file' accept="i/am,a,pathological,;,,,,test/case">

You are testing MIME types with and without parameters but there is no code checking if the MIME types have been correctly recognized and used for the file picker. I don't think you should test with valid MIME types with no parameters. Invalid MIME types (and valid with parameters) are fine because they should be ignored. So we are sure to check for the correct behavior into this test file.

I'm wondering why I thought the set of values separated by comma was specific to MIME types. It can understand why the specification lets the author specify "video or audio" but I hope "{audio,video,image}/*,something" will not be used too much because it will kill all attempts to have specific UI for audio/video/image. I'm especially worried about "audio/*,audio/someuncommonformat".

And sorry for the delay, I was on vacation.
Attachment #454073 - Flags: feedback?(mounir.lamouri) → feedback-
(In reply to comment #5)
> >+  nsCharSeparatedTokenizer 
> >+    tokenizer(accept, ',', nsCharSeparatedTokenizer::SEPARATOR_OPTIONAL);
> 
> This is wrong. AFAICT, the separator isn't optional and you need to check for
> HTML5 spaces. I've wrote the needed code for <input type='email'>. You should
> add it as a dependency (bug 555559) and create the tokenizer like this:
> nsCharSeparatedTokenizerTemplate<nsContentUtils::IsHTMLWhitespace>
> tokenizer(aValue, ',');
> 

Yes, you're right.  I misunderstood what that does.

> I think you can inline ParseAcceptAttribute into GetFileFilterFromAccept. I
> don't see why this function could be needed somewhere else and having it
> inlined could be helpful (like preventing the creation of the array).

I use it in Bug 567323 to display a different UI if we can capture that format.  We can probably find a way to do it that doesn't involve creating the array though.

> >diff --git a/layout/forms/test/test_bug377624.html b/layout/forms/test/test_bug377624.html
> >--- a/layout/forms/test/test_bug377624.html
> >+++ b/layout/forms/test/test_bug377624.html
> >@@ -12,16 +12,23 @@ https://bugzilla.mozilla.org/show_bug.cg
> > </head>
> > <body onload="runTests();">
> > <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=377624">Mozilla Bug 377624</a>
> > <p id="display"></p>
> > <div id="content">
> >   <input id='a' type='file' accept="image/*">
> >   <input id='b' type='file' accept="audio/*">
> >   <input id='c' type='file' accept="video/*">
> >+  <input id='d' type='file' accept="image/*,audio/*">
> >+  <input id='e' type='file' accept="image/*,video/*">
> >+  <input id='f' type='file' accept="audio/*,video/*">
> >+  <input id='g' type='file' accept="image/*,audio/*,video/*">
> >+  <input id='h' type='file' accept="foo/baz,image/*,bogus/duh">
> >+  <input id='i' type='file' accept="mime/type;parameter,video/*">
> >+  <input id='z' type='file' accept="i/am,a,pathological,;,,,,test/case">
> 
> You are testing MIME types with and without parameters but there is no code
> checking if the MIME types have been correctly recognized and used for the file
> picker. I don't think you should test with valid MIME types with no parameters.
> Invalid MIME types (and valid with parameters) are fine because they should be
> ignored. So we are sure to check for the correct behavior into this test file.

I don't understand this.  The patch checks that the proper combination of filters gets set in the file picker.  If you mean that we don't handle "accept='image/png'" properly you're correct, but that wasn't a goal of mine at the moment.  I think we have to be more careful about that to avoid passing arbitrary text to system APIs.
 
> I'm wondering why I thought the set of values separated by comma was specific
> to MIME types. It can understand why the specification lets the author specify
> "video or audio" but I hope "{audio,video,image}/*,something" will not be used
> too much because it will kill all attempts to have specific UI for
> audio/video/image. I'm especially worried about
> "audio/*,audio/someuncommonformat".

I think you would want to go from most to least specific MIME type.  The spec doesn't specifically say that order should matter but I think it's only natural that "accept='image/png,image/*'" means that you would prefer a image/png to any other image/ format.  There is room for the UA to be smart here (display the video UI for something like "accept='video/webm,video/*'") but if a webpage author wants to do something like "accept='video/webm,image/jpeg'" then that's their problem IMO.

> And sorry for the delay, I was on vacation.

No worries.
Created attachment 458184 [details] [diff] [review]
Patch for nsCharSeparatedTokenizer

This patch is part of patch in bug 555559. I've extracted this patch to prevent this bug being blocked by bug 555559.

r=smaug,sicking sr=jst
Kyle, what is the status here? Given Mounir's r-, will you be supplying a new patch? Or do you still want me to review the existing one?
Comment on attachment 454073 [details] [diff] [review]
Patch

I have another version of this in my queue.  I'll post it soon.
Attachment #454073 - Attachment is obsolete: true
Attachment #454073 - Flags: review?(jonas)
Created attachment 458760 [details] [diff] [review]
Patch V2

Addressed Mounir's comments
Attachment #458760 - Flags: review?(jonas)
Attachment #458760 - Flags: feedback?(mounir.lamouri)
Comment on attachment 458760 [details] [diff] [review]
Patch V2

>+  nsCharSeparatedTokenizerTemplate<nsContentUtils::IsHTMLWhitespace>
>+    tokenizer(accept, ',');
>+  // Empty loop body because aCallback is doing the work
>+  while (tokenizer.hasMoreTokens() &&
>+         (*aCallback)(tokenizer.nextToken(), aClosure));

Seems like "killing a fly with a hammer" like we say in french ;)
But I guess it's needed for other callers of |ParseAcceptAttribute|.

f=me

By the way, you should add showfunc=1 and unified=8 in your hgrc [diff] section. It's easier to read :)
Attachment #458760 - Flags: feedback?(mounir.lamouri) → feedback+
Comment on attachment 458760 [details] [diff] [review]
Patch V2

>diff -r 87f1bf5f25c3 layout/forms/nsFileControlFrame.cpp

>+nsFileControlFrame::ParseAcceptAttribute(AcceptAttrCallback aCallback,
>+                                         void* aClosure) const
>+{
>+  nsAutoString accept;
>+  if (!mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::accept, accept) ||
>+      accept.IsEmpty()) {
>+    return;
>+  }

This performance optimization is probably not worth it. Simply do

nsAutoString accept;
mContent->GetAttr(...);

accept will be empty if the attribute doesn't exist or is empty, which will cause the loop below to bail very quickly.

r=me with that.
Attachment #458760 - Flags: review?(jonas) → review+
Thanks for the quick reviews!
Attachment #458184 - Flags: approval2.0?
Comment on attachment 458760 [details] [diff] [review]
Patch V2

HTML5 spec compliance and tests you say?  It's almost too good to be true!
Attachment #458760 - Flags: approval2.0?
Comment on attachment 458184 [details] [diff] [review]
Patch for nsCharSeparatedTokenizer

a=beltzner
Attachment #458184 - Flags: approval2.0? → approval2.0+
Comment on attachment 458760 [details] [diff] [review]
Patch V2

a=beltzner
Attachment #458760 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/a7a688b6e106
http://hg.mozilla.org/mozilla-central/rev/c698f5cadfc1
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
Also pushed http://hg.mozilla.org/mozilla-central/rev/b336127c29d7 for sicking's comment that I missed the first time around.
You need to log in before you can comment on or make changes to this bug.