Closed Bug 101019 (html5-keygen) Opened 23 years ago Closed 5 years ago

KEYGEN support does not belong in the parser

Categories

(Core :: DOM: Core & HTML, defect, P5)

defect

Tracking

()

RESOLVED WORKSFORME
Future
Tracking Status
blocking2.0 --- -

People

(Reporter: jonsmirl, Assigned: jkt)

References

Details

(Keywords: dev-doc-needed, html5)

Attachments

(2 files, 9 obsolete files)

70.15 KB, patch
Details | Diff | Splinter Review
113.46 KB, patch
Details | Diff | Splinter Review
KEYGEN is currently implemented by scanning the parse stream. When a KEYGEN tag 
is found a replacement SELECT is computed and inserted into the stream. This is 
a major kludge.

This would be better implemented by allowing the KEYGEN to pass though the 
parser and creating a DOM node for KEYGEN that would generate the key when the 
node is created. The KEYGEN DOM node could be derrived from SELECT. This would 
also make KEYGEN work in the editor.

See bug http://bugzilla.mozilla.org/show_bug.cgi?id=100896 for further 
discussion.
This bug has also been reported in a much more generic form 
http://bugzilla.mozilla.org/show_bug.cgi?id=51684

The current code causes the parser code to rely on the layout directory which 
it shouldn't need to do.
QA Contact: bsharma → moied
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.0
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)
Target Milestone: mozilla1.0 → mozilla1.0.1
This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration. 
Target Milestone: mozilla1.0.1 → Future
This implementation results in a real user-observable bug. If you save-as, the
saved file does not contain the keygen tag.

To recreate:

Create a file containing a keygen tag:
<form action="http://whatever.com/"
   method="post"
   enctype="application/x-www-form-urlencoded"
   name="Form">

   <keygen name="pub_key" challenge="CwDdf67Mpc" />
   <input type="submit" value="Submit Request" />
</form>

Open in Mozilla (or Firefox)

View source: keygen tag is visible as expected
Save-as, and view saved file: keygen tag has been replaced by:
<select name="pub_key" challenge="CwDdf67Mpc"><option>2048 (High
Grade)</option><option>1024 (Medium Grade)</option><option>512 (Low
Grade)</option></select>
Assignee: harishd → mrbkap
Status: ASSIGNED → NEW
This is needed for HTML5-compliance. Currently, the HTML5 parser will regress <keygen> support, because it treats <keygen> as a real element--not as a parser macro.

Maybe this bug belongs in DOM, Content or Layout instead of the parser?
Keywords: html5
Assignee: mrbkap → nobody
QA Contact: moied → parser
Marking [HTML5], because the HTML5 parser needs this.
Alias: html5-keygen
Summary: KEYGEN support does not belong in the parser → [HTML5] KEYGEN support does not belong in the parser
I think this isn't going to happen this quarter. Filed bug 545854 about getting parity with the old parser in the HTML5 parser.
Priority: P1 → P2
Not a must-have to ship a release.
Priority: P2 → P3
Please be clearer to avoid making PKI people as worried as I was a few minutes ago.

The clean implementation this bug requests is not a must-have for a release, *because* the keygen functionality itself is now available inside the HTML5 parser, through the work-around implemented by bug 545854.

Also, after this work-around, this bug should not block anymore bug html5-parsing right ?
blocking2.0: --- → ?
Removing the people I had started to add as CC of this bug, when I was thinking this bug was actually about removing the keygen functionality.
(In reply to comment #8)
> Also, after this work-around, this bug should not block anymore bug
> html5-parsing right ?

Right.
No longer blocks: html5-parsing
Not blocking 1.9.3 on this.
blocking2.0: ? → -
I assume this is the reason keygen is not supported for pages delivered as application/xhtml+xml
(In reply to comment #12)
> I assume this is the reason keygen is not supported for pages delivered as
> application/xhtml+xml

Yes.
yes, having keygen support in XHTML would be really useful. Otherwise we have to develop javascript kudges - problematic for sites that don't allow javascript.
Not a parser bug really. More like a content tree thing now.
Component: HTML: Parser → DOM: Core & HTML
Priority: P3 → --
QA Contact: parser → general
Summary: [HTML5] KEYGEN support does not belong in the parser → KEYGEN support does not belong in the parser
Sorry but I cannot reproduce the behaviour described in comment 3 with 4.0b8pre on MacOSX. I have created a XHTML5 using BlueGriffon and added exactly the comment 3's markup snippet.
First, BlueGriffon (based on mozilla-central) reads and saves the <keygen> element w/o any problem.
Then I switched to Minefield, opened the document, opened the view source, saved the document using Save As. Sorry, no <select> inside but the <keygen> element, correctly saved...

Time to close this bug?
(In reply to comment #16)
> Time to close this bug?
not as long ad the keygen element doesn't work in pages served as application/xhtml+xml
(In reply to comment #16)
> Time to close this bug?

No. This bug should be considered fixed once parsing <keygen> in HTML, parsing <keygen xmlns="http://www.w3.org/1999/xhtml"/> in XML and document.createElement("keygen") all produce a single element node that:
 1) Implements the DOM interfaces from the HTML5 spec.
 2) Renders the keygen UI when inserted into a document.
 3) Participates in form submission as specced in HTML5.
 4) Has no extra child nodes or _moz- attributes.
(In reply to comment #18)
> (In reply to comment #16)
> > Time to close this bug?
> 
> No. This bug should be considered fixed once parsing <keygen> in HTML, parsing
> <keygen xmlns="http://www.w3.org/1999/xhtml"/> in XML and
> document.createElement("keygen") all produce a single element node that:
>  1) Implements the DOM interfaces from the HTML5 spec.
>  2) Renders the keygen UI when inserted into a document.
>  3) Participates in form submission as specced in HTML5.
>  4) Has no extra child nodes or _moz- attributes.

Ah ok. But at least the bug reported in comment 3 does not exist any more.
That had to be said.
(In reply to comment #19)
> Ah ok. But at least the bug reported in comment 3 does not exist any more.

It exists in the text/html code path still.
Does this prevent us from showing up as having keygen support on html5test.com?
Yes, it does.
Blocks: 901848
Blocks: 941394
Assignee: nobody → wchen
Things that happen in this patch:
- Remove the parser keygen hacks.
- Created an interface for HTMLKeygenElement.
- Create a new frame for keygen elements.
- Moved the elements currently generated for keygen into the frame's anonymous content.

Henri, could you review the parser bits?
Attachment #8344431 - Flags: review?(hsivonen)
Who wants to review the layout and DOM bits and put this poor old bug to rest?

While working on this patch I noticed that keygen is the only consumer of nsIFormProcessor (implemented by nsKeygenHandler). It should be pretty easy to remove after my patch since it's now only being used in one place. I'll file a followup to remove it after this patch lands.
The most likely culprits are Olli and me, I guess.  Olli, you want to do it?
Flags: needinfo?(bugs)
Comment on attachment 8344431 [details] [diff] [review]
Remove keygen hack from parser and create HTMLKeygenElement interface.

Review of attachment 8344431 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/HTMLKeygenElement.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/HTMLKeygenElement.h"
> +#include "mozilla/dom/HTMLKeygenElementBinding.h"

Put this in the next block, and sort the list

@@ +16,5 @@
> +#include "mozilla/layout/KeygenFrame.h"
> +
> +NS_IMPL_NS_NEW_HTML_ELEMENT_CHECK_PARSER(Keygen)
> +
> +using namespace mozilla::dom;

Wrap the file in namespace mozilla { namespace dom { } } instead

@@ +54,5 @@
> +
> +HTMLSelectElement*
> +HTMLKeygenElement::GetSelectElement()
> +{
> +  KeygenFrame* keygenFrame = do_QueryFrame(GetPrimaryFrame());

Do we want to get this from the frame? This seems like it would probably make keygen { display: none } break the stuff below.

@@ +133,5 @@
> +{
> +  nsPresState* state = GetPrimaryPresState();
> +  if (state) {
> +    HTMLSelectElement* select = GetSelectElement();
> +    if (select) {

Use early returns here, please

@@ +189,5 @@
> +nsresult
> +HTMLKeygenElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
> +                                const nsAttrValue* aValue, bool aNotify)
> +{
> +  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::disabled)  {

Double space

::: content/html/content/src/HTMLKeygenElement.h
@@ +12,5 @@
> +#include "nsGkAtoms.h"
> +#include "nsStyleConsts.h"
> +#include "nsIAtom.h"
> +#include "nsIConstraintValidation.h"
> +#include "nsRuleData.h"

Sort

@@ +91,5 @@
> +    // Return the empty string for unsupported encryption types.
> +    if (!aKeytype.LowerCaseEqualsLiteral("rsa") &&
> +        !aKeytype.LowerCaseEqualsLiteral("dsa") &&
> +        !aKeytype.LowerCaseEqualsLiteral("ec")) {
> +      aKeytype.AssignLiteral("");

Truncate()

@@ +106,5 @@
> +    aType.AssignLiteral("keygen");
> +  }
> +
> +  // nsIConstraintValidation
> +  NS_IMETHOD GetWillValidate(bool* aWillValidate);

MOZ_OVERRIDEs here too

@@ +136,5 @@
> +  // a select element in anonymous content.
> +  int32_t GetInitialSelectIndex() { return mInitialSelectIndex; }
> +
> +  bool mInhibitStateRestoration;
> +  int32_t mInitialSelectIndex;

In general, putting members ordered from larger to smaller sizes avoids some memory waste.

::: dom/webidl/HTMLKeygenElement.webidl
@@ +28,5 @@
> +  readonly attribute boolean willValidate;
> +  readonly attribute ValidityState validity;
> +  readonly attribute DOMString validationMessage;
> +  boolean checkValidity();
> +  // boolean reportValidity();

?

::: layout/forms/KeygenFrame.cpp
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "KeygenFrame.h"

mozilla/layout/...

@@ +21,5 @@
> +#include "mozilla/dom/Element.h"
> +#include "mozilla/dom/HTMLKeygenElement.h"
> +#include "mozilla/dom/HTMLSelectElement.h"
> +#include "nsContentList.h"
> +#include "nsStyleSet.h"

Sort

@@ +87,5 @@
> +  nsHTMLReflowMetrics selectDesiredSize;
> +  nsIFrame* selectFrame = mSelect->GetPrimaryFrame();
> +  if (selectFrame) { // display:none?
> +    NS_ASSERTION(selectFrame == mFrames.FirstChild(), "huh?");
> +    rv = ReflowAnonymousContent(aPresContext, selectDesiredSize,

Declare rv here

@@ +88,5 @@
> +  nsIFrame* selectFrame = mSelect->GetPrimaryFrame();
> +  if (selectFrame) { // display:none?
> +    NS_ASSERTION(selectFrame == mFrames.FirstChild(), "huh?");
> +    rv = ReflowAnonymousContent(aPresContext, selectDesiredSize,
> +                                         aReflowState, selectFrame);

Indentation

@@ +156,5 @@
> +KeygenFrame::CreateAnonymousContent(nsTArray<ContentInfo>& aElements)
> +{
> +  nsCOMPtr<nsIDocument> doc = mContent->GetDocument();
> +  mSelect = doc->CreateHTMLElement(nsGkAtoms::select);
> +  if (!aElements.AppendElement(mSelect)) {

nsTArray is infallible.

@@ +161,5 @@
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  HTMLSelectElement* select = GetSelectElement();
> +  HTMLKeygenElement* keygenElt = static_cast<HTMLKeygenElement*>(mContent);

Add a HTMLKeygenElement::FromContent

@@ +165,5 @@
> +  HTMLKeygenElement* keygenElt = static_cast<HTMLKeygenElement*>(mContent);
> +  keygenElt->InjectKeygenOptions(select);
> +
> +  ErrorResult rv;
> +  select->SetSelectedIndex(keygenElt->GetInitialSelectIndex(), rv);

You're losing the exception here

::: layout/forms/KeygenFrame.h
@@ +50,5 @@
> +  }
> +#endif
> +
> +  virtual bool IsFrameOfType(uint32_t aFlags) const MOZ_OVERRIDE
> +  {

Please be consistent about where you put the { here; I think it's at end-of-line in layout/

@@ +63,5 @@
> +                                  nsHTMLReflowMetrics& aWrappersDesiredSize,
> +                                  const nsHTMLReflowState& aReflowState,
> +                                  nsIFrame* aOuterWrapperFrame);
> +
> +  nsCOMPtr<mozilla::dom::Element> mSelect;

Make this an HTMLSelectElement?

::: parser/htmlparser/tests/mochitest/html5_tree_construction_exceptions.js
@@ +4,5 @@
>   * html5lib_tree_construction/html5lib_license.txt for the license for these
>   * tests.
>   */
>  var html5Exceptions = {
> +//  "<example><entry><of><exception>" : true,

Hear, hear!
Comment on attachment 8344431 [details] [diff] [review]
Remove keygen hack from parser and create HTMLKeygenElement interface.

r+ on the parser parts.

Great to see progress on this bug. Thank you!
Attachment #8344431 - Flags: review?(hsivonen) → review+
I could review.
Flags: needinfo?(bugs)
Attachment #8344431 - Flags: review?(bugs)
Thanks for the comments Ms2ger!

Here is a new patch that is cleaned up and fixed so that it is green in try.

(In reply to :Ms2ger from comment #27)
> Comment on attachment 8344431 [details] [diff] [review]
> Remove keygen hack from parser and create HTMLKeygenElement interface.
> 
> Review of attachment 8344431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we want to get this from the frame? This seems like it would probably
> make keygen { display: none } break the stuff below.
> 
This seems to be how the other elements get the frame, and it doesn't look like anything particularly bad will happen if we don't get the frame.
>
> ::: dom/webidl/HTMLKeygenElement.webidl
> @@ +28,5 @@
> > +  readonly attribute boolean willValidate;
> > +  readonly attribute ValidityState validity;
> > +  readonly attribute DOMString validationMessage;
> > +  boolean checkValidity();
> > +  // boolean reportValidity();
> 
We don't seem to implement reportValidity() anywhere in Gecko yet and I don't think this bug is the right place to do it.
> 
> @@ +165,5 @@
> > +  HTMLKeygenElement* keygenElt = static_cast<HTMLKeygenElement*>(mContent);
> > +  keygenElt->InjectKeygenOptions(select);
> > +
> > +  ErrorResult rv;
> > +  select->SetSelectedIndex(keygenElt->GetInitialSelectIndex(), rv);
> 
> You're losing the exception here
> 
I think that's OK. This is done to restore the saved state of the form, and if that happens to fail for some reason it's probably better to leave it the way it is than to throw an exception.
> 
> @@ +63,5 @@
> > +                                  nsHTMLReflowMetrics& aWrappersDesiredSize,
> > +                                  const nsHTMLReflowState& aReflowState,
> > +                                  nsIFrame* aOuterWrapperFrame);
> > +
> > +  nsCOMPtr<mozilla::dom::Element> mSelect;
> 
> Make this an HTMLSelectElement?
> 
I'm going to keep it as Element otherwise passing it to nsContentUtils::DestroyAnonymousContent gets a little awkward. It's also more consistent with the other frames because they tend to store the anonymous content in a nsCOMPtr<Element>.
Attachment #8344431 - Attachment is obsolete: true
Attachment #8344431 - Flags: review?(bugs)
Attachment #8345147 - Flags: review?(bugs)
Attached patch parser v1 diff v2 (obsolete) — Splinter Review
Hi Henri,

There was a bug in the last patch, can you review this delta? Thanks.
Attachment #8345149 - Flags: review?(hsivonen)
Attached patch dom v1 diff v2 (obsolete) — Splinter Review
Here is a diff of the important changes to the DOM part in case you already started reviewing.
Attachment #8345147 - Attachment is obsolete: true
Attachment #8345147 - Flags: review?(bugs)
Attachment #8345153 - Flags: review?(hsivonen)
Attached patch dom v1 diff v2 (obsolete) — Splinter Review
Attachment #8345150 - Attachment is obsolete: true
Comment on attachment 8345153 [details] [diff] [review]
Remove keygen hack from parser and create HTMLKeygenElement interface. v2

r+ on the parser part. The rest was going to smaug and/or bz, right?
Attachment #8345153 - Flags: review?(hsivonen) → review+
Comment on attachment 8345153 [details] [diff] [review]
Remove keygen hack from parser and create HTMLKeygenElement interface. v2

(In reply to Henri Sivonen (:hsivonen) from comment #35)
> Comment on attachment 8345153 [details] [diff] [review]
> Remove keygen hack from parser and create HTMLKeygenElement interface. v2
> 
> r+ on the parser part. The rest was going to smaug and/or bz, right?

Oops, yeah. I messed up on the flags when attaching the bug.
Attachment #8345153 - Flags: review?(bugs)
Comment on attachment 8345153 [details] [diff] [review]
Remove keygen hack from parser and create HTMLKeygenElement interface. v2

I don't see how this can handle tabbing + focus correctly.
I think you need to add IsHTMLFocusable() and make sure tabbing doesn't focus
keygen but the inner native anonymous UI. Please add a test for that too.

But in general looks great.
Attachment #8345153 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #37)
> Comment on attachment 8345153 [details] [diff] [review]
> Remove keygen hack from parser and create HTMLKeygenElement interface. v2
> 
> I don't see how this can handle tabbing + focus correctly.
> I think you need to add IsHTMLFocusable() and make sure tabbing doesn't focus
> keygen but the inner native anonymous UI. Please add a test for that too.
> 
> But in general looks great.

IsHTMLFocusable is already doing the right thing in the super class. Focusing works correctly because I override the |Focus| method to focus the anonymous content.

I'm not sure how to write a good test for making sure the anonymous content is focused instead of the keygen. The ways that I can think of require making an assumption about the keygen element having a select element in its anonymous content, but it's not required that keygen be rendered as a select. Do you feel it is OK to make that assumption for a test or do you have a better idea?
Flags: needinfo?(bugs)
Depends on: 952198
(In reply to William Chen [:wchen] from comment #38)
> IsHTMLFocusable is already doing the right thing in the super class.
> Focusing works correctly because I override the |Focus| method to focus the
> anonymous content.
Why does tabbing work?
In HTMLInputElement::IsHTMLFocusable we have rather special code for number and file.
I'd expect something similar would be needed with keygey.
Did you test tabbing and various tabindexes?

nsFileControlFrame::AttributeChanged and nsNumberControlFrame::AttributeChanged both need to
deal with tabindex changes.
 
> I'm not sure how to write a good test for making sure the anonymous content
> is focused instead of the keygen. The ways that I can think of require
> making an assumption about the keygen element having a select element in its
> anonymous content, but it's not required that keygen be rendered as a
> select. Do you feel it is OK to make that assumption for a test or do you
> have a better idea?
Since our implementation has select, yes, such assumption is ok.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #39)
> Why does tabbing work?
> In HTMLInputElement::IsHTMLFocusable we have rather special code for number
> and file.
> I'd expect something similar would be needed with keygey.
> Did you test tabbing and various tabindexes?
> 
You were right, regular tabbing worked but tabindexes didn't. I've added tests for focus and tabbing.
Attachment #8345153 - Attachment is obsolete: true
Attachment #8357428 - Flags: review?(bugs)
Attached patch v2 diff v3 (obsolete) — Splinter Review
Attachment #8345149 - Attachment is obsolete: true
Attachment #8345154 - Attachment is obsolete: true
Comment on attachment 8357428 [details] [diff] [review]
Remove keygen hack from parser and create HTMLKeygenElement interface. v3

>+++ b/accessible/tests/mochitest/elm/test_HTMLSpec.html
>@@ -809,33 +809,16 @@
>         children: [
>           { role: ROLE_TEXT_LEAF }, // plain text
>           { role: ROLE_TEXT_LEAF } // HTML:kbd text
>         ]
>       };
>       testElm("kbd_container", obj);
> 
>       //////////////////////////////////////////////////////////////////////////
>-      // HTML:keygen
>-
>-      obj = {
>-        role: ROLE_COMBOBOX,
>-        states: STATE_COLLAPSED | STATE_HASPOPUP,
>-        extraStates: EXT_STATE_EXPANDABLE,
>-        actions: "open",
>-        children: [ 
>-          { COMBOBOX_LIST: [
>-            { role: ROLE_COMBOBOX_OPTION }, // high grade
>-            { role: ROLE_COMBOBOX_OPTION } // medium grade
>-          ] }
>-        ]
>-      };
>-      testElm("keygen", obj);
>-
Why this?
This kind of change needs a review from some a11y peer.


>+HTMLKeygenElement::IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
>+                                   int32_t* aTabIndex)
>+{
>+  if (nsGenericHTMLFormElementWithState::IsHTMLFocusable(aWithMouse,
>+    aIsFocusable, aTabIndex)) {
indentation is odd

>+HTMLKeygenElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor)
>+{
>+  ErrorResult rv;
>+  if (aVisitor.mEvent->message == NS_FOCUS_CONTENT) {
>+    Focus(rv);
>+  } else if (aVisitor.mEvent->message == NS_BLUR_CONTENT) {
>+    Blur(rv);
>+  }

You should check that mEvent is trusted here.
(And yes, looks like HTMLSelectElement::PostHandleEvent has similar issue)


>+    // Return the empty string for unsupported encryption types.
>+    if (!aKeytype.LowerCaseEqualsLiteral("rsa") &&
>+        !aKeytype.LowerCaseEqualsLiteral("dsa") &&
>+        !aKeytype.LowerCaseEqualsLiteral("ec")) {
>+      aKeytype.Truncate();
Could you please add some comment why those values are supported.
(Per spec only "rsa", but implementations can support more).
Perhaps mention nsKeygenFormProcessor


>diff --git a/content/html/content/test/test_focus_keygen.html b/content/html/content/test/test_focus_keygen.html
please test also tabindex -1



>+KeygenFrame::Reflow(nsPresContext* aPresContext,
>+                    nsHTMLReflowMetrics& aDesiredSize,
>+                    const nsHTMLReflowState& aReflowState,
>+                    nsReflowStatus& aStatus)
I'd prefer some layout peer to review this. dholbert perhaps


>+KeygenFrame::ReflowAnonymousContent(nsPresContext* aPresContext,
>+                                    nsHTMLReflowMetrics& aSelectDesiredSize,
>+                                    const nsHTMLReflowState& aParentReflowState,
>+                                    nsIFrame* aSelectFrame)
>+{
And this
Attachment #8357428 - Flags: review?(bugs) → review+
Updated patch with comments addressed.

surkov: Could you please review test_HTMLSpec.html?
dholbert: Could you please review KeygenFrame::Reflow and KeygenFrame::ReflowAnonymousContent?

Thanks.
Attachment #8357428 - Attachment is obsolete: true
Attachment #8357430 - Attachment is obsolete: true
Attachment #8359378 - Flags: review?(surkov.alexander)
Attachment #8359378 - Flags: review?(dholbert)
Comment on attachment 8359378 [details] [diff] [review]
keygen.patchRemove keygen hack from parser and create HTMLKeygenElement interface. v4

Review of attachment 8359378 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for a11y test
Attachment #8359378 - Flags: review?(surkov.alexander) → review+
So, one problem with the current patch -- it neuters styles on the <keygen> element.

e.g. The styling in these examples don't have any effect, with this patch applied:
 Should remove native theming:
 data:text/html,<keygen style="-moz-appearance: none">
 Should be super-tall:
 data:text/html,<keygen style="height: 400px">
...and this example should probably set the background of the <select> itself, but instead it sets the background *behind* the <select>:
 data:text/html,<keygen style="background: purple">abc

For this to work, we would need to either do away with the nesting altogether (not sure if we can), or else make the inner select content be a pseudo-element, so that we can style it to inherit a bunch of things (or use e.g. "width: 100%") in either forms.css or ua.css.

We also probably need some reftests asserting that <keygen> looks like <select> in these situations & others.
Comment on attachment 8359378 [details] [diff] [review]
keygen.patchRemove keygen hack from parser and create HTMLKeygenElement interface. v4

Disregarding comment 45 for the moment, here's my review feedback on the frame class:

>diff --git a/layout/forms/KeygenFrame.cpp b/layout/forms/KeygenFrame.cpp
>+#include "mozilla/layout/KeygenFrame.h"

No need to specify the full path there - that file lives right next to this one. Just make this #include "KeygenFrame.h"

>+NS_IMETHODIMP
>+KeygenFrame::Reflow(nsPresContext* aPresContext,
>+                    nsHTMLReflowMetrics& aDesiredSize,
>+                    const nsHTMLReflowState& aReflowState,
>+                    nsReflowStatus& aStatus)
[...]
>+  nsHTMLReflowMetrics selectDesiredSize(aReflowState.GetWritingMode());
>+  nsIFrame* selectFrame = mSelect->GetPrimaryFrame();
>+  if (selectFrame) { // display:none?
>+    NS_ASSERTION(selectFrame == mFrames.FirstChild(), "huh?");

Let's flip the assignment-vs-assertion source-of-truth for selectFrame -- i.e. set selectFrame to mFrames.FirstChild(), and then assert that it's equal to selectElement->GetPrimaryFrame().

(reasoning: mFrames.FirstChild() should be slightly cheaper, and in the unlikely event that these frames aren't the same, it should be safer/saner to reflow the one that's actually our child frame.)

Also: I'm pretty sure the assertion should be moved outside of the "if" check.

>+  // Match the desired size to whatever we got after reflowing the select.
>+  aDesiredSize.Width() = selectDesiredSize.Width() +
>+                         aReflowState.ComputedPhysicalBorderPadding().LeftRight();
>+  aDesiredSize.Height() = selectDesiredSize.Height() +
>+                          aReflowState.ComputedPhysicalBorderPadding().TopBottom();

These are longer than 80 characters. Maybe just de-indent the second line of each assignment to be indented 2 spaces more than "aDesiredSize.*"?

>diff --git a/layout/forms/KeygenFrame.h b/layout/forms/KeygenFrame.h
>+#ifndef mozilla_layout_KeygenFrame_h
>+#define mozilla_layout_KeygenFrame_h

Nit: drop the "_layout" here. (to mirror namespacing change mentioned below)

>+namespace mozilla {
[...]
>+namespace layout {
>+
>+class KeygenFrame MOZ_FINAL : public nsContainerFrame,
>+                              public nsIAnonymousContentCreator

Please drop the "namespace layout" here. (And hence: "using namespace mozilla::layout" elsewhere in this patch should be removed or replaced with "using namespace mozilla".)

(background: roc/dbaron briefly flirted with the idea of putting frame classes in mozilla::layout, but now we've decided that nested namespacing isn't worth it, and we're putting new frame classes directly in 'namespace mozilla'. For reference, see bug 940193 comment 3 through 5, and bug 940193 comment 11.)

>+  virtual bool IsLeaf() const MOZ_OVERRIDE
>+  {
>+    return false;
>+  }

I think should be "return true".  AIUI, IsLeaf should return true for any frame that doesn't expect to have children other than the ones it creates itself via nsIAnonymousContentCreator.  And that appears to be the case for this frame type.

The way it is now, it asserts in my debug build when I load this testcase:
 data:text/html,<style>keygen::after {content:"def"}</style><keygen>
(since ::after" creates a child, which we ignore.)

That data-URL doesn't fail any assertions if I make IsLeaf() return true here; hence, I'm pretty sure we should return true.

(Maybe include a version of that testcase as a crashtest?)

>+  KeygenFrame(nsStyleContext* aContext);
>+  virtual ~KeygenFrame();

Please declare the constructor/destructor before the "public:" section (which is implicitly 'private') as we do in e.g. nsRangeFrame. This is best-practice for frame classes, because it ensures that nobody can directly instantiate or delete a frame of this type.  (Frames are only supposed to be created via NS_NewXYZFrame() and destroyed via nsIFrame::Destroy().)

>+#ifdef DEBUG
>+  NS_IMETHOD GetFrameName(nsAString& aResult) const MOZ_OVERRIDE
>+  {
>+    return MakeFrameName(NS_LITERAL_STRING("Keygen"), aResult);
>+  }
>+#endif

As of bug 956447, this should now be #ifdef DEBUG_FRAME_DUMP.

Also: This frame class seems to lack a GetType() method. You probably should add one. (returning nsGkAtoms::keygenFrame, which you'll need to add to the " // Frame types" section of nsGkAtomList.h)

>+} // namespace layout
>+} // namespace dom
>+
>+#endif // mozilla_layout_KeygenFrame_h

s/dom/mozilla/
(In reply to Daniel Holbert [:dholbert] from comment #45)
> For this to work, we would need to either do away with the nesting
> altogether (not sure if we can)

Following up on this slightly: so, is there a reason we need to have a nested anonymous select element (and frame), rather than just using a derived frame type (or even sharing the frame type)?

We have a similar type of situation for <button> vs. <input type="submit">. <button> can have author-provided child elements (just like <select> can); <input type="submit"> cannot (just like <keygen> cannot), but they render roughly the same, with a bit of custom behavior on <input type="submit"> (just like <select> and <keygen).  We don't implement <input type="submit"> as a nsContainerFrame that contains a <button> -- instead, we have the class "nsHTMLButtonControlFrame" which is used by <button>, and which has a (poorly-named) subclass "nsGfxButtonControlFrame" that manages the specific behavior used by <input type="submit">.

Would something like that that be a reasonable way to do this? (I don't know enough about <keygen> and the content side of it to be sure, but if it's just a specialized <select> with values that we provide (as it superficially appears to be), then I'd imagine this could work.)
(In reply to Daniel Holbert [:dholbert] from comment #47)
> (In reply to Daniel Holbert [:dholbert] from comment #45)
> > For this to work, we would need to either do away with the nesting
> > altogether (not sure if we can)
> 
> Following up on this slightly: so, is there a reason we need to have a
> nested anonymous select element (and frame), rather than just using a
> derived frame type (or even sharing the frame type)?
> 
> We have a similar type of situation for <button> vs. <input type="submit">.
> <button> can have author-provided child elements (just like <select> can);
> <input type="submit"> cannot (just like <keygen> cannot), but they render
> roughly the same, with a bit of custom behavior on <input type="submit">
> (just like <select> and <keygen).  We don't implement <input type="submit">
> as a nsContainerFrame that contains a <button> -- instead, we have the class
> "nsHTMLButtonControlFrame" which is used by <button>, and which has a
> (poorly-named) subclass "nsGfxButtonControlFrame" that manages the specific
> behavior used by <input type="submit">.
> 
> Would something like that that be a reasonable way to do this? (I don't know
> enough about <keygen> and the content side of it to be sure, but if it's
> just a specialized <select> with values that we provide (as it superficially
> appears to be), then I'd imagine this could work.)

That seems reasonable. There is no particular reason for the way I did it, I'm just not familiar with frame code. I'll give it a shot implementing it the way you mentioned. Thanks for the review and the insight.
I'd assume making non-<select> DOM element working with almost-<select>'s frame code can be rather 
tricky.
There is the assumption that the mContent has <option> child elements and what not, and
<keygen> obviously doesn't have all that.
...in other words, in such case <option> elements would need to be in native anonymous content, and
that feels rather odd and would be tricky to handle since nsIFrame::mContent doesn't have those
elements as child nodes (because they are in the native anon content).
(In reply to Olli Pettay [:smaug] from comment #50)
> ...in other words, in such case <option> elements would need to be in native
> anonymous content

That's effectively what I had in mind, yeah.

> that feels rather odd and would be tricky to handle since nsIFrame::mContent
> doesn't have those
> elements as child nodes (because they are in the native anon content).

Ah, OK. That could still be handled via a frame subclass (with a "GetActualContent()" virtual method or something), but granted, it'd add some complexity and would probably require some tweaks to generify nsComboboxControlFrame (and would maybe require some refactoring on the content side to share code between keygen & select on that end, too).

I don't really know the content side of things well enough to evaluate the relative complexity / workability of the options here, but bz probably has the best big-picture view to make a recommendation on this. bz, what are your thoughts on comment 45 / comment 47?
Flags: needinfo?(bzbarsky)
Because <option>s would need to be in NAC, I don't see how that approach would be any simpler or better
than the approach in the current patch. In fact, it sounds a lot more complicated than just making
<select> NAC.
(In reply to Olli Pettay [:smaug] from comment #52)
> Because <option>s would need to be in NAC, I don't see how that approach
> would be any simpler or better
> than the approach in the current patch.

It's simpler from the perspective of the style system -- styles would Just Work, whereas with the current patch's approach, we need to add a pseudo-element and potentially inherit every (I think?) property to that pseudo-element. (though if we really do want every property to pass through, maybe we could just use "all: inherit", which wouldn't be so bad)

I'm not necessarily claiming that it's simpler overall.
For what it's worth, I think the chances of making Daniel's suggestion work and work in a future-proof way as we add new CSS properties are _much_ higher than trying to have a non-replaced box with an anonymous <select> inside.  Vastly higher.

> and potentially inherit every (I think?) property to that pseudo-element

That will fail badly for things like position and percentage sizes...  Doing that sort of thing is basically rocket science that has to be done on a property by property basis.  :(
Flags: needinfo?(bzbarsky)
Styling is indeed tricky. What do other browsers do with <keygen>?
(In reply to Boris Zbarsky [:bz] from comment #54)
> For what it's worth, I think the chances of making Daniel's suggestion work
> and work in a future-proof way as we add new CSS properties are _much_
> higher than trying to have a non-replaced box with an anonymous <select>
> inside.  Vastly higher.

Sounds like we should then improve nac handling in general.

Making keygen to work like select doesn't sounds exactly simple.
In DOM there would be some base class for them both and then in layout code
<select> would deal with <option>s and <keygen> nac <option>s...
(In reply to Boris Zbarsky [:bz] from comment #54)
> > and potentially inherit every (I think?) property to that pseudo-element
> 
> That will fail badly for things like position and percentage sizes...

Ah, right. I was pretty sure there were gotchas; I just couldn't remember them.

(In reply to Olli Pettay [:smaug] from comment #55)
> What do other browsers do with <keygen>?

Testing briefly on linux:

Chrome (dev channel) actually fails to propagate styles to the <select> element in the same way that we do with the current patch applied.

Opera 12.16 (Presto) seems to honor styles correctly. So does Gecko, currently.
Because current <keygen> in Gecko is just <select> in DOM tree.
It looks like if we want styling to work well we need to go with comment 47. In addition to the issues already mentioned about the solution in comment 45, select has a lot of styling applied in forms.css that we probably don't want to override with inherited values.

The select frame is a lot more complex than I expected and it looks like it's going to be a lot of work to use it with keygen. I've been working on this bug as a side project so I probably won't be able to get around to this anytime soon.
Attachment #8359378 - Flags: review?(dholbert) → review-
Depends on: 960888
I went a little further in trying to get <keygen> to use the combobox frames and decided that's it's not worth the effort. Hopefully we will have a way to handle this styling issue with anonymous content in the future.

I feel that the patch in this bug is still worthwhile because it gets rid of a parser hack, removes the hack in the select element and introduces the HTMLKeygenElement API. The styling is already kind of broken right now because the keygen will match styles for select and the children of keygen will match styles for the option element, after all, the parser just treats <keygen> as a <select> macro. Also, given that blink styling is busted in a similar way, web pages probably aren't very dependent on it.

dholbert: How do you feel about landing this patch and filing a followup to track the styling problem?
Flags: needinfo?(dholbert)
(In reply to William Chen [:wchen] from comment #60)
> dholbert: How do you feel about landing this patch and filing a followup to
> track the styling problem?

I'm not too keen on it. I suspect the followup would basically be "Rewrite KeygenFrame completely", which isn't really great as followup material.

Granted, the status quo isn't bug-free, but breaking styles entirely on <keygen> seems like a pretty big step backwards (even if it means we get to remove a hack and add a new API).

Having said that: if we really need this API added ASAP and don't have the cycles to implement the frame class the right way, then maybe it's worth it; I don't know enough about the situation to make that judgement call. (Though in that case, there's still more minor review feedback to be addressed in comment 46.)
Flags: needinfo?(dholbert)
(You're right that tagname-based style rules are already broken, e.g. this incorrectly has a red background in Gecko...
 data:text/html,<keygen><style>select{background:red}
...and for the same reason, this doesn't succeed at adding a lime background:
 data:text/html,<keygen><style>keygen{background:lime}

However, non-tagname-based styles (using IDs, classes, child selectors, etc) do work correctly, as do inline style, and this patch would break all of those. For example, this currently works (adds a lime background), but wouldn't with the current patch:
 data:text/html,<keygen class="foo"><style>.foo{background:lime}
Unassigning myself, no longer interested in working on this bug.
Assignee: wchen → nobody
Updated patch with review comments addressed.
Attachment #8359378 - Attachment is obsolete: true
For posterity, here is a rough WIP that uses combobox frames for the keygen element. There is a lot of stuff that still needs to be done including:
- Add the remaining code for HTMLKeygenElement to drive the combobox frames.
- Complete form submission and other form related code.
- Update accessibility code to find the anonymous option elements.
- Some platforms (android, b2g, possibly others) use native controls for the combobox, so there needs to be a platform specific implementation and tests.
I don't think it's worthwhile to continue down this path. It's probably better to create an anonymous content container or proxy frame that properly handles styling.
(In reply to William Chen [:wchen] from comment #63)
> Unassigning myself, no longer interested in working on this bug.

That's unfortunate.

(In reply to William Chen [:wchen] from comment #65)
> I don't think it's worthwhile to continue down this path. It's probably
> better to create an anonymous content container or proxy frame that properly
> handles styling.

I think we're making the wrong trade-off if we keep having this bug because the fix available isn't perfect styling-wise. I think we should land something that that makes client-side certificate enrollment work and not let the fix to be blocked by polishing the styling side.
SOWs are removed in Bug 825392.
Depends on: 825392
No longer depends on: 952198
IMO to make progress on this, it may be easier to split this issue into several smaller tasks like it was done for the date and time input types (bug 888320).
I see at least three parts:

- Refactoring of DOM parsing (do not replace <keygen> by <select>)
- Refactoring of backend functionality
- Implementation of DOM/content

There might be more parts in case UI needs to be touched or in case some attributes are currently not implemented.

Sebastian
All three of those points are already implemented by the patch attached to this bug. The issue remaining is figuring out what kind of frame we want to create for the keygen and how we want to style it.

To summarize the current situation, the patch in this bug implements keygen using a frame that creates a select element in anonymous content. Setting styles on the keygen frame does not style the anonymous select element and we currently don't have the ability to pass on the styles to something in anonymous content.

This turns out to be a regression because we got styling for free when we used to pretend <keygen> was a <select>, but we also got some <select> specific styling stuff that we don't want. Although, I'm not sure how important the regression is because at least one other major browser is broken in a similar way, and it hasn't been something we have tested at all.
(In reply to William Chen [:wchen] from comment #69)
> This turns out to be a regression because we got styling for free when we
> used to pretend <keygen> was a <select>, but we also got some <select>
> specific styling stuff that we don't want. Although, I'm not sure how
> important the regression is because at least one other major browser is
> broken in a similar way, and it hasn't been something we have tested at all.

I think we should just land with the style regression. A style regression for a rarely used and even more rarely styled element is less bad than a weird DOM.
I could agree with that, especially if other browsers do badly with styling too.
(In reply to Henri Sivonen (:hsivonen) from comment #70)
> I think we should just land with the style regression.

Depending on how important the API is, I'm open to (but not enthusiastic about) that possibility... See comment 61 for my thoughts.
(In reply to Daniel Holbert [:dholbert] from comment #72)
> (In reply to Henri Sivonen (:hsivonen) from comment #70)
> > I think we should just land with the style regression.
> 
> Depending on how important the API is, I'm open to (but not enthusiastic
> about) that possibility... See comment 61 for my thoughts.

<keygen> is very important for the super-specialized use case of x.509 client certificate enrollment process with a CA. Outside that use case, no one ever sees the element.

Considering the design of e.g. https://www.startssl.com/ (the only site I ever recall having used <keygen> on for non-test purposes), not having styling is not a real loss.
(In reply to Henri Sivonen (:hsivonen) from comment #73)
> (In reply to Daniel Holbert [:dholbert] from comment #72)
> > (In reply to Henri Sivonen (:hsivonen) from comment #70)
> > > I think we should just land with the style regression.
> > 
> > Depending on how important the API is, I'm open to (but not enthusiastic
> > about) that possibility... See comment 61 for my thoughts.
> 
> <keygen> is very important for the super-specialized use case of x.509
> client certificate enrollment process with a CA. Outside that use case, no
> one ever sees the element.
> 
> Considering the design of e.g. https://www.startssl.com/ (the only site I
> ever recall having used <keygen> on for non-test purposes), not having
> styling is not a real loss.

keygen can be used much more widely that for building CA based certificates.
It can easily used to create non-CA based certificates which are much cheaper
to make and could also be much more useful. We have shown this on the WebID
over TLS protocol and have quite a lot of implementations that are relying 
on this.

 http://www.w3.org/2005/Incubator/webid/spec/

Now I am not sure how this affects the styling. People would presumably only
use a keygen form very rarely - since one keygen generated certificate signed by their
own server should allow them to authenticate to any server worldwide. Still one
can imagine scenarios where people decide to once a year update their public key, or
do that if they feel that someone has broken into their server. 

Another reason someone could create more than one certificate would be for each of their devices - creation of a WebID X509 certificate is cheap - or for each browser if the browsers don't use the same OS keychain.
Google have removed KeyGen altogether:
https://bugs.chromium.org/p/chromium/issues/detail?id=514767

I thought Mozilla was following this (bad) idea as well?
Priority: -- → P5
Can this be duped to bug 1315460?
(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #76)
> Can this be duped to bug 1315460?

Not really. It's unclear if bug 1315460 can proceed more quickly than this one, given the use cases. If bug 1315460 gets fixed before this one, we can resolve this as WORKSFORME when bug 1315460 gets fixed.

Bug 1315460 resolved this by removing support completely to match the web standard and also Chrome.

Assignee: nobody → jkt
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.