Last Comment Bug 101019 - (html5-keygen) KEYGEN support does not belong in the parser
(html5-keygen)
: KEYGEN support does not belong in the parser
Status: NEW
: dev-doc-needed, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 25 votes (vote)
: Future
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 822627 (view as bug list)
Depends on: 960888 825392
Blocks: html5test 901848 941394 322657 515255
  Show dependency treegraph
 
Reported: 2001-09-21 14:07 PDT by Jon Smirl
Modified: 2016-03-21 15:30 PDT (History)
35 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Remove keygen hack from parser and create HTMLKeygenElement interface. (55.33 KB, patch)
2013-12-08 18:24 PST, William Chen [:wchen]
hsivonen: review+
Details | Diff | Review
Remove keygen hack from parser and create HTMLKeygenElement interface. (61.29 KB, patch)
2013-12-09 22:51 PST, William Chen [:wchen]
no flags Details | Diff | Review
parser v1 diff v2 (1.99 KB, patch)
2013-12-09 22:53 PST, William Chen [:wchen]
hsivonen: review+
Details | Diff | Review
dom v1 diff v2 (2.69 KB, patch)
2013-12-09 22:54 PST, William Chen [:wchen]
no flags Details | Diff | Review
Remove keygen hack from parser and create HTMLKeygenElement interface. v2 (60.62 KB, patch)
2013-12-09 23:00 PST, William Chen [:wchen]
hsivonen: review+
bugs: review-
Details | Diff | Review
dom v1 diff v2 (2.02 KB, patch)
2013-12-09 23:01 PST, William Chen [:wchen]
no flags Details | Diff | Review
Remove keygen hack from parser and create HTMLKeygenElement interface. v3 (68.84 KB, patch)
2014-01-08 14:59 PST, William Chen [:wchen]
bugs: review+
Details | Diff | Review
v2 diff v3 (14.00 KB, patch)
2014-01-08 15:00 PST, William Chen [:wchen]
no flags Details | Diff | Review
keygen.patchRemove keygen hack from parser and create HTMLKeygenElement interface. v4 (70.63 KB, patch)
2014-01-13 12:07 PST, William Chen [:wchen]
surkov.alexander: review+
dholbert: review-
Details | Diff | Review
Remove keygen hack from parser and create HTMLKeygenElement interface. v5 (with styling bugs) (70.15 KB, patch)
2014-03-03 11:37 PST, William Chen [:wchen]
no flags Details | Diff | Review
WIP with <keygen> using combobox frames (113.46 KB, patch)
2014-03-03 11:53 PST, William Chen [:wchen]
no flags Details | Diff | Review

Description Jon Smirl 2001-09-21 14:07:04 PDT
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.
Comment 1 Asa Dotzler [:asa] 2001-12-03 10:56:04 PST
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)
Comment 2 harishd 2002-04-02 12:04:48 PST
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. 
Comment 3 John Hartnup 2004-03-26 05:50:36 PST
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>
Comment 4 Henri Sivonen (:hsivonen) 2009-04-09 05:33:29 PDT
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?
Comment 5 Henri Sivonen (:hsivonen) 2009-10-12 06:26:34 PDT
Marking [HTML5], because the HTML5 parser needs this.
Comment 6 Henri Sivonen (:hsivonen) 2010-02-12 05:57:43 PST
I think this isn't going to happen this quarter. Filed bug 545854 about getting parity with the old parser in the HTML5 parser.
Comment 7 Henri Sivonen (:hsivonen) 2010-02-25 05:43:28 PST
Not a must-have to ship a release.
Comment 8 Jean-Marc Desperrier 2010-02-26 06:13:40 PST
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 ?
Comment 9 Jean-Marc Desperrier 2010-02-26 06:17:04 PST
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.
Comment 10 Henri Sivonen (:hsivonen) 2010-02-26 06:41:06 PST
(In reply to comment #8)
> Also, after this work-around, this bug should not block anymore bug
> html5-parsing right ?

Right.
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2010-05-19 14:34:47 PDT
Not blocking 1.9.3 on this.
Comment 12 reto bachmann-gmuer 2010-07-28 04:52:26 PDT
I assume this is the reason keygen is not supported for pages delivered as application/xhtml+xml
Comment 13 Henri Sivonen (:hsivonen) 2010-07-28 05:05:16 PDT
(In reply to comment #12)
> I assume this is the reason keygen is not supported for pages delivered as
> application/xhtml+xml

Yes.
Comment 14 Henry Story 2010-07-28 05:28:26 PDT
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.
Comment 15 Henri Sivonen (:hsivonen) 2010-09-06 04:40:26 PDT
Not a parser bug really. More like a content tree thing now.
Comment 16 Daniel Glazman (:glazou) 2010-10-18 03:03:21 PDT
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?
Comment 17 reto bachmann-gmuer 2010-10-18 03:48:08 PDT
(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
Comment 18 Henri Sivonen (:hsivonen) 2010-10-18 04:30:42 PDT
(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.
Comment 19 Daniel Glazman (:glazou) 2010-10-18 05:47:54 PDT
(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.
Comment 20 Henri Sivonen (:hsivonen) 2010-10-18 05:52:43 PDT
(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.
Comment 21 JP Rosevear [:jpr] 2012-07-31 11:52:39 PDT
Does this prevent us from showing up as having keygen support on html5test.com?
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-31 12:16:55 PDT
Yes, it does.
Comment 23 :Ms2ger 2012-12-18 08:26:44 PST
*** Bug 822627 has been marked as a duplicate of this bug. ***
Comment 24 William Chen [:wchen] 2013-12-08 18:24:08 PST
Created attachment 8344431 [details] [diff] [review]
Remove keygen hack from parser and create HTMLKeygenElement interface.

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?
Comment 25 William Chen [:wchen] 2013-12-08 18:30:23 PST
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.
Comment 26 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-12-08 18:34:29 PST
The most likely culprits are Olli and me, I guess.  Olli, you want to do it?
Comment 27 :Ms2ger 2013-12-09 00:19:13 PST
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 28 Henri Sivonen (:hsivonen) 2013-12-09 04:05:26 PST
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!
Comment 29 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2013-12-09 04:25:18 PST
I could review.
Comment 30 William Chen [:wchen] 2013-12-09 22:51:16 PST
Created attachment 8345147 [details] [diff] [review]
Remove keygen hack from parser and create HTMLKeygenElement interface.

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>.
Comment 31 William Chen [:wchen] 2013-12-09 22:53:32 PST
Created attachment 8345149 [details] [diff] [review]
parser v1 diff v2

Hi Henri,

There was a bug in the last patch, can you review this delta? Thanks.
Comment 32 William Chen [:wchen] 2013-12-09 22:54:51 PST
Created attachment 8345150 [details] [diff] [review]
dom v1 diff v2

Here is a diff of the important changes to the DOM part in case you already started reviewing.
Comment 33 William Chen [:wchen] 2013-12-09 23:00:48 PST
Created attachment 8345153 [details] [diff] [review]
Remove keygen hack from parser and create HTMLKeygenElement interface. v2
Comment 34 William Chen [:wchen] 2013-12-09 23:01:31 PST
Created attachment 8345154 [details] [diff] [review]
dom v1 diff v2
Comment 35 Henri Sivonen (:hsivonen) 2013-12-10 03:14:48 PST
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?
Comment 36 William Chen [:wchen] 2013-12-10 09:28:13 PST
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.
Comment 37 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2013-12-17 14:43:41 PST
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.
Comment 38 William Chen [:wchen] 2013-12-18 12:29:22 PST
(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?
Comment 39 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2014-01-07 16:29:12 PST
(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.
Comment 40 William Chen [:wchen] 2014-01-08 14:59:41 PST
Created attachment 8357428 [details] [diff] [review]
Remove keygen hack from parser and create HTMLKeygenElement interface. v3

(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.
Comment 41 William Chen [:wchen] 2014-01-08 15:00:08 PST
Created attachment 8357430 [details] [diff] [review]
v2 diff v3
Comment 42 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2014-01-12 15:56:23 PST
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
Comment 43 William Chen [:wchen] 2014-01-13 12:07:22 PST
Created attachment 8359378 [details] [diff] [review]
keygen.patchRemove keygen hack from parser and create HTMLKeygenElement interface. v4

Updated patch with comments addressed.

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

Thanks.
Comment 44 alexander :surkov 2014-01-13 12:10:19 PST
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
Comment 45 Daniel Holbert [:dholbert] 2014-01-14 17:17:49 PST
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 46 Daniel Holbert [:dholbert] 2014-01-14 17:31:22 PST
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/
Comment 47 Daniel Holbert [:dholbert] 2014-01-14 17:51:40 PST
(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.)
Comment 48 William Chen [:wchen] 2014-01-14 18:02:36 PST
(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.
Comment 49 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2014-01-15 02:44:17 PST
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.
Comment 50 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2014-01-15 02:47:52 PST
...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).
Comment 51 Daniel Holbert [:dholbert] 2014-01-15 06:26:54 PST
(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?
Comment 52 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2014-01-15 06:58:38 PST
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.
Comment 53 Daniel Holbert [:dholbert] 2014-01-15 07:13:12 PST
(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.
Comment 54 Boris Zbarsky [:bz] (Out June 25-July 6) 2014-01-15 07:18:55 PST
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.  :(
Comment 55 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2014-01-15 07:21:03 PST
Styling is indeed tricky. What do other browsers do with <keygen>?
Comment 56 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2014-01-15 07:25:14 PST
(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...
Comment 57 Daniel Holbert [:dholbert] 2014-01-15 07:29:08 PST
(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.
Comment 58 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2014-01-15 07:35:12 PST
Because current <keygen> in Gecko is just <select> in DOM tree.
Comment 59 William Chen [:wchen] 2014-01-15 10:51:30 PST
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.
Comment 60 William Chen [:wchen] 2014-02-28 15:24:34 PST
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?
Comment 61 Daniel Holbert [:dholbert] 2014-02-28 18:40:59 PST
(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.)
Comment 62 Daniel Holbert [:dholbert] 2014-02-28 18:56:23 PST
(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}
Comment 63 William Chen [:wchen] 2014-03-03 11:36:36 PST
Unassigning myself, no longer interested in working on this bug.
Comment 64 William Chen [:wchen] 2014-03-03 11:37:17 PST
Created attachment 8384772 [details] [diff] [review]
Remove keygen hack from parser and create HTMLKeygenElement interface. v5 (with styling bugs)

Updated patch with review comments addressed.
Comment 65 William Chen [:wchen] 2014-03-03 11:53:07 PST
Created attachment 8384785 [details] [diff] [review]
WIP with <keygen> using combobox frames

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.
Comment 66 Henri Sivonen (:hsivonen) 2014-03-03 23:23:11 PST
(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.
Comment 67 Florian Bender 2014-03-23 08:45:56 PDT
SOWs are removed in Bug 825392.
Comment 68 Sebastian Zartner [:sebo] 2014-07-01 03:16:37 PDT
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
Comment 69 William Chen [:wchen] 2014-07-01 18:40:01 PDT
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.
Comment 70 Henri Sivonen (:hsivonen) 2014-07-07 02:15:35 PDT
(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.
Comment 71 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2014-07-07 06:16:34 PDT
I could agree with that, especially if other browsers do badly with styling too.
Comment 72 Daniel Holbert [:dholbert] 2014-07-07 14:02:57 PDT
(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.
Comment 73 Henri Sivonen (:hsivonen) 2014-07-08 02:45:22 PDT
(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.
Comment 74 Henry Story 2014-07-08 22:33:01 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.