Last Comment Bug 583514 - All HTML Elements should have click() and accesskey
: All HTML Elements should have click() and accesskey
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla5
Assigned To: David Zbarsky (:dzbarsky)
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
: 82951 148585 (view as bug list)
Depends on: 1125813 650629 666604 666674 666955 666985 667632 668223
Blocks: 583533
  Show dependency treegraph
 
Reported: 2010-07-31 15:03 PDT by David Zbarsky (:dzbarsky)
Modified: 2015-02-05 16:00 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Part 1: Move focus, blur. click, accesskey to nsIDOMHTMLElement (55.44 KB, patch)
2010-07-31 15:04 PDT, David Zbarsky (:dzbarsky)
bugs: review-
Details | Diff | Review
Part 2: Elements with an accesskey are commands (2.80 KB, patch)
2010-08-01 15:14 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Part 2: Elements with an accesskey are commands (4.11 KB, patch)
2010-08-01 15:17 PDT, David Zbarsky (:dzbarsky)
bugs: review-
Details | Diff | Review
testcase (527 bytes, text/html)
2010-08-04 16:33 PDT, David Zbarsky (:dzbarsky)
no flags Details
Move focus, blur. click, accesskey to nsIDOMHTMLElement v2 (56.81 KB, patch)
2010-08-04 19:32 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Move focus, blur. click, accesskey to nsIDOMHTMLElement v2 (57.95 KB, patch)
2010-08-04 19:33 PDT, David Zbarsky (:dzbarsky)
bugs: review-
Details | Diff | Review
Patch v2.1 (62.65 KB, patch)
2010-08-12 18:52 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Patch v2.2 (69.59 KB, patch)
2010-08-13 11:20 PDT, David Zbarsky (:dzbarsky)
bugs: review-
Details | Diff | Review
Patch v2.3 (79.90 KB, patch)
2010-08-23 10:16 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Patch v2.3 (80.74 KB, patch)
2010-08-23 10:49 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Patch with updates tests (82.02 KB, patch)
2010-10-16 08:13 PDT, David Zbarsky (:dzbarsky)
bugs: review+
Details | Diff | Review
Refreshed patch r=smaug (82.04 KB, patch)
2010-11-09 13:58 PST, David Zbarsky (:dzbarsky)
roc: approval2.0-
Details | Diff | Review
Patch r=smaug (125.30 KB, patch)
2011-03-26 10:56 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Unbitrotted path r=smaug (125.57 KB, patch)
2011-04-01 15:03 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Unbitrotted path r=smaug (125.65 KB, patch)
2011-04-06 13:53 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Review
Merged to tip (123.76 KB, patch)
2011-04-11 08:36 PDT, :Ms2ger
no flags Details | Diff | Review

Description David Zbarsky (:dzbarsky) 2010-07-31 15:03:10 PDT

    
Comment 1 David Zbarsky (:dzbarsky) 2010-07-31 15:04:59 PDT
Created attachment 461828 [details] [diff] [review]
Part 1: Move focus, blur. click, accesskey to nsIDOMHTMLElement

All elements can have accesskeys (if they have an accesskey, they are a command).  The action of the command is basically to call click on the element, so all elements need click.  I added blur and focus because I was moving stuff around at the same time.
Comment 2 David Zbarsky (:dzbarsky) 2010-07-31 15:07:54 PDT
Looks like I missed the IID on Label and some whitespace is weird.
Comment 3 David Zbarsky (:dzbarsky) 2010-07-31 19:15:58 PDT
In order to correctly implement the label for "Using the accesskey attribute to define a command on other elements", we must do the following:

"The Label of the command depends on the element. If the element is a labeled control, the textContent of the first label element in tree order whose labeled control is the element in question is the Label (in DOM terms, this is the string given by element.labels[0].textContent). Otherwise, the Label is the textContent of the element itself."
Comment 4 David Zbarsky (:dzbarsky) 2010-08-01 15:14:58 PDT
Created attachment 461942 [details] [diff] [review]
Part 2: Elements with an accesskey are commands

Are we going to do something like "User agents may expose the commands whose Hidden State facet is false (visible), e.g. in the user agent's menu bar. User agents are encouraged to do this especially for commands that have Access Keys, as a way to advertise those keys to the user."?
Comment 5 David Zbarsky (:dzbarsky) 2010-08-01 15:17:27 PDT
Created attachment 461943 [details] [diff] [review]
Part 2: Elements with an accesskey are commands

qref
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-08-03 05:11:33 PDT
Comment on attachment 461828 [details] [diff] [review]
Part 1: Move focus, blur. click, accesskey to nsIDOMHTMLElement

>+nsresult nsGenericHTMLElement::Click()
>+{
>+  if (mHandlingClick)
>+    return NS_OK;
>+
>+  mHandlingClick = PR_TRUE;
>+  // Strong in case the event kills it
>+  nsCOMPtr<nsIDocument> doc = GetCurrentDoc();
>+  if (!doc) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIPresShell> shell = doc->GetShell();
>+  nsRefPtr<nsPresContext> context = nsnull;
>+  if (shell) {
>+    context = shell->GetPresContext();
>+  }
>+
>+  if (!context) {
>+    doc->FlushPendingNotifications(Flush_Frames);
>+    shell = doc->GetShell();
>+    if (shell) {
>+      context = shell->GetPresContext();
>+    }
>+  }
>+
>+  if (context) {
>+    // Click() is never called from native code, but it may be
>+    // called from chrome JS. Mark this event trusted if Click()
>+    // is called from chrome code.
>+    nsMouseEvent event(nsContentUtils::IsCallerChrome(),
>+                       NS_MOUSE_CLICK, nsnull, nsMouseEvent::eReal);
>+    event.inputSource = nsIDOMNSMouseEvent::MOZ_SOURCE_UNKNOWN;
>+    nsEventStatus status = nsEventStatus_eIgnore;
>+
>+    nsEventDispatcher::Dispatch(static_cast<nsIContent*>(this), context,
>+                                &event, nsnull, &status);
>+  }       
>+
>+  mHandlingClick = PR_FALSE;
>+  return NS_OK;
>+}
I think we don't need to ensure that prescontext is non-null.
And one thing to test in other browsers is that whether they dispatch click event when the element
isn't in document. HTML5 draft needs to be clarified in that case.
So, for now use GetCurrentDoc() to get prescontext, but don't flush.
And don't null check the prescontext.
And could you please check what other browsers do with click when the element isn't in document.



>@@ -762,16 +766,17 @@ protected:
>                                     nsGkAtoms::contenteditable, values,
>                                     eIgnoreCase);
> 
>     return value > 0 ? eTrue : (value == 0 ? eFalse : eInherit);
>   }
> 
>   // Used by A, AREA, LINK, and STYLE.
>   already_AddRefed<nsIURI> GetHrefURIForAnchors() const;
>+  PRBool mHandlingClick;

This is not good. We shouldn't make basically all the html elements 4 or 8 bytes larger.
Please add a new node flag for this.

>+  // Forward nsIDOMHTMLElement -- cannot use macro because we override Focus
So you do this few times.
Perhaps some macro which forwards all the other calls except ::Focus?

>-[scriptable, uuid(a6cf9085-15b3-11d2-932e-00805f8add32)]
>+[scriptable, uuid(1d36fdfb-9b24-4fcb-a6b0-9c53ca8ebc84)]
> interface nsIDOMHTMLElement : nsIDOMElement
> {
>            attribute DOMString        id;
>            attribute DOMString        title;
>            attribute DOMString        lang;
>            attribute DOMString        dir;
>            attribute DOMString        className;
>+
>+           attribute DOMString        accessKey;
>+  void blur();
>+  void focus();
>+  void click();
Align properly with the rest of the interface


This all needs mochitests:
- What happens when focus/blur is called on for example <div> element?
- How are the accesskeys handled?
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-08-03 05:22:28 PDT
Comment on attachment 461943 [details] [diff] [review]
Part 2: Elements with an accesskey are commands

>+NS_IMETHODIMP 
>+nsGenericHTMLElement::GetIcon(nsAString& aIcon)
>+{
>+  aIcon.AssignLiteral("");
SetDOMStringToNull

>+NS_IMETHODIMP 
>+nsGenericHTMLElement::GetLabel(nsAString& aLabel)
>+{
>+  GetTextContent(aLabel); // XXX A little incorrect, but we need element.labels to fix this
>+  return NS_OK;           // See Bug 556743
Could you perhaps ensure that 556743 gets fixed before this one?



>+nsGenericHTMLElement::GetCommandType(nsAString& aCommandType)
>+{
>+  aCommandType.AssignLiteral("command");
>+  return NS_OK;
>+}
So why is this right?
"If the element does not define a command, it must return null."


>diff --git a/dom/interfaces/html/nsIDOMNSHTMLElement.idl b/dom/interfaces/html/nsIDOMNSHTMLElement.idl
>--- a/dom/interfaces/html/nsIDOMNSHTMLElement.idl
>+++ b/dom/interfaces/html/nsIDOMNSHTMLElement.idl
>@@ -54,9 +54,16 @@ interface nsIDOMNSHTMLElement : nsISuppo
Update uuid
Comment 8 David Zbarsky (:dzbarsky) 2010-08-04 14:26:50 PDT
> This all needs mochitests:

I'm not sure how to write mochitests for this, but I'll look around and see if I can find examples

> - What happens when focus/blur is called on for example <div> element?

According to the spec, 
"The focus() method, when invoked, must run the following algorithm:

    If the element is marked as locked for focus, then abort these steps.

    If the element is not focusable, then abort these steps."
 
Focusable elements are defined at http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#focusable.
Basically, it should just do nothing.  Will make sure this happens, but how can we test this?

> - How are the accesskeys handled?

When the accesskey is activated, it fires the click event. (Call Click())  
I just need to make sure this actually happens ;)
Comment 9 David Zbarsky (:dzbarsky) 2010-08-04 16:32:15 PDT
So I ran the attached test.

According to my understand of the spec, it should say:
Div clicked input clicked input focused

It currently says:
Div clicked input focused

Before the patch is says:
Input focused ------------ Do we not call onclick on inputs?

Chrome does not support calling .click() and .focus() on arbitrary elements, so I commented those lines out.  It says:
input clicked div clicked input focused
Looks like they fire onclick at ancestors of the element, including the input itself.

What's the right thing to do here?
Comment 10 David Zbarsky (:dzbarsky) 2010-08-04 16:33:01 PDT
Created attachment 462980 [details]
testcase
Comment 11 David Zbarsky (:dzbarsky) 2010-08-04 17:21:28 PDT
So we had a bug where the input element Click() method would not fire onclick if the input type did not match one of the 6 types it checked against.  I fixed that, now the output of the test case is:

div clicked input clicked div clicked input focused

This should be correct, because the input onclick() bubbles to the div, which also gets onclick().
Comment 12 David Zbarsky (:dzbarsky) 2010-08-04 19:32:13 PDT
Created attachment 463038 [details] [diff] [review]
Move focus, blur. click, accesskey to nsIDOMHTMLElement v2

Addressing comments and fixing the click() issues.  I added the testcase as a reftest.  Blur and Focus don't need tests because they were already on nsGenericHTMLElement, I just moved them from nsIDOMNSHTMLElement to nsIDOMHTMLElement.

Still need to figure out a way to test accesskeys.
Comment 13 David Zbarsky (:dzbarsky) 2010-08-04 19:33:27 PDT
Created attachment 463040 [details] [diff] [review]
Move focus, blur. click, accesskey to nsIDOMHTMLElement v2

this time for reals
Comment 14 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-08-11 09:17:14 PDT
Comment on attachment 463040 [details] [diff] [review]
Move focus, blur. click, accesskey to nsIDOMHTMLElement v2

># HG changeset patch
># Parent d09b3c13383a2836d532f4061bccd4545bbafc49
># User David Zbarsky <dzbarsky@gmail.com>
>Move focus, blur, click, and accesskey to nsIDOMHTMLElement.idl per HTML5
>
>diff --git a/content/base/public/nsINode.h b/content/base/public/nsINode.h
>--- a/content/base/public/nsINode.h
>+++ b/content/base/public/nsINode.h
>@@ -174,16 +174,19 @@ enum {
>   NODE_IS_ELEMENT              = 0x00200000U,
>   
>   // Set if the node has the accesskey attribute set.
>   NODE_HAS_ACCESSKEY           = 0x00400000U,
> 
>   // Set if the node has the accesskey attribute set.
>   NODE_HAS_NAME                = 0x00800000U,
> 
>+  // Set if the node is currently handling the click
>+  NODE_HANDLING_CLICK          = 0x01000000U,
>+
>   // Two bits for the script-type ID.  Not enough to represent all
>   // nsIProgrammingLanguage values, but we don't care.  In practice,
>   // we can represent the ones we want, and we can fail the others at
>   // runtime.
>   NODE_SCRIPT_TYPE_OFFSET =               24,
When you add a new flag, you need to increase NODE_SCRIPT_TYPE_OFFSET
> nsresult
> nsGenericHTMLElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>                               nsIAtom* aPrefix, const nsAString& aValue,
>                               PRBool aNotify)
> {
>   PRBool contentEditable = aNameSpaceID == kNameSpaceID_None &&
>                            aName == nsGkAtoms::contenteditable;
>+  PRBool accessKey = aNameSpaceID == kNameSpaceID_None &&
>+                     aName == nsGkAtoms::accesskey;
Could you first check aName, and only after that the namespace.
namespace is pretty much always kNameSpaceID_None.
>+nsresult nsGenericHTMLElement::Click()
>+{
>+  if (HasFlag(NODE_HANDLING_CLICK))
>+    return NS_OK;
>+
>+  SetFlags(NODE_HANDLING_CLICK);
>+  // Strong in case the event kills it
>+  nsCOMPtr<nsIDocument> doc = GetCurrentDoc();
>+  if (!doc) {
You should unset the flag here.
Or just move the !doc check to be before setting the flag.


> nsHTMLInputElement::Click()
> {
>   nsresult rv = NS_OK;
> 
>-  if (GET_BOOLBIT(mBitField, BF_HANDLING_CLICK)) // Fixes crash as in bug 41599
>-      return rv;                      // --heikki@netscape.com
>+  if (HasFlag(NODE_HANDLING_CLICK)) // Fixes crash as in bug 41599
>+    return NS_OK;                   // --heikki@netscape.com               
>+
>+  SetFlags(NODE_HANDLING_CLICK);
Set the flag after all the checks which may return early.

Tests!

For accesskeys you can dispatch key events using nsIDOMWindowUtils.
Comment 15 David Zbarsky (:dzbarsky) 2010-08-12 18:52:34 PDT
Created attachment 465515 [details] [diff] [review]
Patch v2.1

We didn't have enough flags to add one more, so I ended up changing the NODE_IS_ELEMENT flag to an mIsElement bool (per bz's suggestion)
Comment 16 David Zbarsky (:dzbarsky) 2010-08-13 11:20:45 PDT
Created attachment 465747 [details] [diff] [review]
Patch v2.2

Fixed a test that relied on previous (incorrect) behavior.  Fixed my reftest.  This now passes tryserver.  I disabled the check for <object> because it is failing - object receives focus, which is the behavior we probably want to preserve.
Comment 17 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-08-16 12:56:39 PDT
(In reply to comment #16)
> I disabled the check for <object> because it is
> failing - object receives focus, which is the behavior we probably want to
> preserve.
Have you sent any email about this to whatwg mailing list?
Comment 18 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-08-16 13:08:53 PDT
I'll review this tomorrow (need to be careful with testcases too, in this case, since we're changing focus handling).
Comment 19 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-08-19 03:58:00 PDT
(In reply to comment #16)
> Created attachment 465747 [details] [diff] [review]
> Patch v2.2
> 
> Fixed a test that relied on previous (incorrect) behavior.
So could you explain this? Are you sure the current behavior is wrong.
HTML5 draft has bugs very often.
Comment 20 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-08-19 04:29:51 PDT
Comment on attachment 465747 [details] [diff] [review]
Patch v2.2

>+++ b/content/events/test/test_bug409604.html
>@@ -17,22 +17,19 @@ https://bugzilla.mozilla.org/show_bug.cg
> </div>
> <pre id="test">
> <script class="testbody" type="text/javascript">
> 
>   /** Test for Bug 409604 **/
> 
>   var modifier = Components.interfaces.nsIDOMNSEvent.ALT_MASK |
>                  Components.interfaces.nsIDOMNSEvent.SHIFT_MASK;
>-  var expectedFocus = "d,g,h,k,l,m,n";
>+  var expectedFocus = "d,g,h,k,l,m,n,p";
>   // XXX the "map" test is causing trouble, see bug 433089
>-  // var expectedClick = "a,b,c,e,f,i,j";
>-  var expectedClick = "a,c,e,f,i,j";
So why this change?

>@@ -58,25 +55,24 @@ https://bugzilla.mozilla.org/show_bug.cg
>       {tag: "hr"},
>       {tag: "i", content: "text"},
>       {tag: "img", attribs: {src: "any.png", alt: "image"}},
>       {tag: "ins", content: "text"},
>       {tag: "kbd", content: "text"},
>       {tag: "li", content: "text", parent: "ol"},
>       {tag: "li", content: "text", parent: "ul"},
>       {tag: "noscript", content: "text"},
>-      {tag: "object", content: "text"},
>+      //{tag: "object", content: "text"},
I don't understand this?
Are we breaking some case? We probably don't want to do that.

>+      <tr>
>+        <td>select</td>
>+        <td>
>+            <select onclick="handleClick(event.target);" onfocus="handleFocus(event.target)" accesskey="p">test button</button>
>+        </td>
>+      </tr>

This needs more tests. How should accesskey work with <div> element for example?


>diff --git a/content/html/content/reftests/583514-1-ref.html b/content/html/content/reftests/583514-1-ref.html
>new file mode 100644
>--- /dev/null
>+++ b/content/html/content/reftests/583514-1-ref.html
>@@ -0,0 +1,3 @@
>+<body>
>+<input>
>+<div>Results: div clicked body clicked input clicked div clicked body clicked input focused
Please add some test for focusable <div> (You need to set some CSS property for that, IIRC)


>+nsresult nsGenericHTMLElement::Click()
>+{
>+  if (HasFlag(NODE_HANDLING_CLICK))
>+    return NS_OK;
>+
>+  // Strong in case the event kills it
>+  nsCOMPtr<nsIDocument> doc = GetCurrentDoc();
>+  if (!doc) {
>+    return NS_OK;
>+  }
So did you test what other browsers do if the element isn't in document?


>+<pre id="test">
>+<script type="application/javascript">
>+
>+    /** Test for Bug 583514 **/
>+
>+    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+
>+    var Cc = Components.classes;
>+    var Ci = Components.interfaces;
>+
>+    var modifier = Cc["@mozilla.org/preferences-service;1"].
>+    getService(Ci.nsIPrefBranch).
>+    getIntPref("ui.key.contentAccess");
>+
>+    var divfocus = false;
>+    var divclicked = false;
>+    var inputfocus = false;
>+    var inputclicked = false;
>+
>+    var utils = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
>+    //utils.sendNativeKeyEvent(0, 65, modifier, "a", "a");
Remove this.
Comment 21 David Zbarsky (:dzbarsky) 2010-08-22 15:49:09 PDT
(In reply to comment #17)
> Have you sent any email about this to whatwg mailing list?
The spec says "User agents should make the following elements focusable, unless platform conventions dictate otherwise", so sounds like we can make <object> focusable

(In reply to comment #20)
> So why this change?

This is because every element should fire onclick if its accesskey is activated, not just the ones that are in the expected click array.

> >@@ -58,25 +55,24 @@ https://bugzilla.mozilla.org/show_bug.cg
> >       {tag: "hr"},
> >       {tag: "i", content: "text"},
> >       {tag: "img", attribs: {src: "any.png", alt: "image"}},
> >       {tag: "ins", content: "text"},
> >       {tag: "kbd", content: "text"},
> >       {tag: "li", content: "text", parent: "ol"},
> >       {tag: "li", content: "text", parent: "ul"},
> >       {tag: "noscript", content: "text"},
> >-      {tag: "object", content: "text"},
> >+      //{tag: "object", content: "text"},
> I don't understand this?
> Are we breaking some case? We probably don't want to do that.

I've forgotten why I did this, going to take another look.

> >+      <tr>
> >+        <td>select</td>
> >+        <td>
> >+            <select onclick="handleClick(event.target);" onfocus="handleFocus(event.target)" accesskey="p">test button</button>
> >+        </td>
> >+      </tr>
> 
> This needs more tests. How should accesskey work with <div> element for
> example?

I think that div is one of the elements in the list that end up getting tested.

> Please add some test for focusable <div> (You need to set some CSS property for

Is it enough to set style="height: 30px; overflow: auto;" and add a lot of text?
When I call .focus() on the div after that, scrolling the mouse scrolls the div, even if the mouse is not over the div.  Either way, this patch shouldn't change the behavior of focus, only that of click and accesskey.

> So did you test what other browsers do if the element isn't in document?

The following test alerts in Chrome, Opera, and Safari, but not IE.
var elem = document.createElement("input");
elem.setAttribute("onclick","javascript:alert('works for elements not in doc');");
elem.click();

I'm guessing we want to allow click even if element isn't in document?
Comment 22 David Zbarsky (:dzbarsky) 2010-08-22 16:01:31 PDT
(In reply to comment #19)
> So could you explain this? Are you sure the current behavior is wrong.
> HTML5 draft has bugs very often.

The HTML 4.01 spec says the accesskeys are only allowed on A, AREA, BUTTON, INPUT, LABEL, and LEGEND, and TEXTAREA.
HTML5 spec says accesskeys are allowed on all elements, and if activated, they call click() and focus().  This is a deliberate change; I'm not sure what you mean by a bug in the draft, but this was a design decision.
Comment 23 David Zbarsky (:dzbarsky) 2010-08-22 16:50:32 PDT
So IE doesn't alert in this case either:

<body id="body">
<script>
var elem = document.createElement("input");
elem.setAttribute("onclick","javascript:alert('hi');");
elem.setAttribute("id", "a");
elem.setAttribute("type", "button");
document.getElementById("body").appendChild(elem);
elem.click();
</script>
Comment 24 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-08-23 01:16:18 PDT
> Is it enough to set style="height: 30px; overflow: auto;" and add a lot of
> text
> When I call .focus() on the div after that, scrolling the mouse scrolls the
> div, even if the mouse is not over the div.
Ok. Do we have a test for that.

>  Either way, this patch shouldn't
> change the behavior of focus, only that of click and accesskey.
But accesskey may focus some an element.
Does -moz-user-focus (ignore|normal|none) CSS property change the behavior?
 



> I'm guessing we want to allow click even if element isn't in document?
Yes.
Comment 25 David Zbarsky (:dzbarsky) 2010-08-23 10:16:31 PDT
Created attachment 468343 [details] [diff] [review]
Patch v2.3

-moz-user-focus only seems to work for xul, and is already tested in dom/tests/mochitest/chrome/window_focus.xul, including accesskeys.
Comment 26 David Zbarsky (:dzbarsky) 2010-08-23 10:49:42 PDT
Created attachment 468359 [details] [diff] [review]
Patch v2.3
Comment 27 David Zbarsky (:dzbarsky) 2010-08-27 09:20:31 PDT
Changing title to reflect goal of bug
Comment 28 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-09-06 07:23:49 PDT
Sorry for the delay.
Have you tested accesskey + contentEditable?
Comment 29 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-09-14 11:15:15 PDT
Comment on attachment 468359 [details] [diff] [review]
Patch v2.3

Clearing review until I get answer to my question.
Comment 30 David Zbarsky (:dzbarsky) 2010-10-16 08:13:10 PDT
Created attachment 483730 [details] [diff] [review]
Patch with updates tests

contenteditable and accesskey work together, I added a test for that.
Also added a test for clicking an element that is not in a document.
Comment 31 Jeff Walden [:Waldo] (remove +bmo to email) 2010-10-25 14:02:09 PDT
*** Bug 148585 has been marked as a duplicate of this bug. ***
Comment 32 Jim Blandy :jimb 2010-11-04 18:26:08 PDT
Nominating this for blocking 2.0, as this causes the following strict-mode compliance test to claim that our compliance is not complete:

http://kangax.github.com/es5-compat-table/strict-mode/

That page incorrectly claims we don't support strict mode code in event handlers, because the test is written to rely on buttons having a 'click' method. We've contacted the test author, but he hasn't gotten around to fixing it yet. I'm not sure how significant that site is, PR-wise, but if it is significant, then fixing this bug will help us get recognition we deserve.

(I'm referring to the test toward the bottom of the page: "NON-STANDARD: Event handler follows strict mode rules when its body starts with use strict directive (e.g.: <p onclick="'use strict'; ...">...</p>)".)
Comment 33 Juriy "kangax" Zaytsev 2010-11-04 18:44:49 PDT
Jim, sorry it completely slipped my mind (Jeff did in fact bring it up recently). I've been buried with work and haven't had a chance to look into replacing click with something else. The test is faulty, which is not good. I'll comment it out for now.

I would still love to see this ticket taken care of sooner rather than later.
Comment 34 Juriy "kangax" Zaytsev 2010-11-04 21:41:39 PDT
Fixed click issue — using form with "onreset" event handler attribute now. FWIW, there's still one case where FF "fails" comparing to WebKit on that test page. WebKit disallows FunctionDeclaration to be part of Statement when in strict mode (a very popular de-facto standard among almost all ES implementations, but — AUIU — is encouraged by TC39 to be avoided for future compatibility (as block-level function declarations seem to be making it to Harmony)).

But this, of course, isn't any kind of "failure" (since behavior is non-standard and is just an informal suggestion), which is why the test is in "non-standard" section.
Comment 35 Brendan Eich [:brendan] 2010-11-04 21:57:03 PDT
(In reply to comment #34)
> Fixed click issue — using form with "onreset" event handler attribute now.

Thanks!

> FWIW, there's still one case where FF "fails" comparing to WebKit on that test
> page. WebKit disallows FunctionDeclaration to be part of Statement when in
> strict mode (a very popular de-facto standard among almost all ES
> implementations, but — AUIU — is encouraged by TC39 to be avoided for future
> compatibility (as block-level function declarations seem to be making it to
> Harmony)).

This is a bug, it will be fixed: bug 609832.

> But this, of course, isn't any kind of "failure" (since behavior is
> non-standard and is just an informal suggestion), which is why the test is in
> "non-standard" section.

It is a failure, if the implementation does not in fact make functions as block children block-local, hoisted to top of block. We don't yet (bug 585536). Hence the es5strict-blocking bug 609832.

See also bug 497869.

/be
Comment 36 Jim Blandy :jimb 2010-11-04 22:21:37 PDT
(In reply to comment #33)
> Jim, sorry it completely slipped my mind (Jeff did in fact bring it up
> recently). I've been buried with work and haven't had a chance to look into
> replacing click with something else. The test is faulty, which is not good.
> I'll comment it out for now.

Awesome!  Thank you very much!

> I would still love to see this ticket taken care of sooner rather than later.

Indeed.
Comment 37 Juriy "kangax" Zaytsev 2010-11-04 23:05:25 PDT
(In reply to comment #35)
> (In reply to comment #34)
[...]
> > FWIW, there's still one case where FF "fails" comparing to WebKit on that test
> > page. WebKit disallows FunctionDeclaration to be part of Statement when in
> > strict mode (a very popular de-facto standard among almost all ES
> > implementations, but — AUIU — is encouraged by TC39 to be avoided for future
> > compatibility (as block-level function declarations seem to be making it to
> > Harmony)).
> 
> This is a bug, it will be fixed: bug 609832.
> 
> > But this, of course, isn't any kind of "failure" (since behavior is
> > non-standard and is just an informal suggestion), which is why the test is in
> > "non-standard" section.
> 
> It is a failure, if the implementation does not in fact make functions as block
> children block-local, hoisted to top of block. We don't yet (bug 585536). Hence
> the es5strict-blocking bug 609832.
> 
> See also bug 497869.

Hold on, but WebKit doesn't actually make them block-local. It disallows them (and does it per wiki suggestion, as far as I can tell — http://wiki.ecmascript.org/doku.php?id=conventions:no_non_standard_strict_decls).

Should wiki be changed to disallow func. decl in statements to be interpreted as anything other than block-local, runtime-hoisted (for harmony compat). Or am I missing something?
Comment 38 Brendan Eich [:brendan] 2010-11-04 23:21:55 PDT
(In reply to comment #37)
> > It is a failure, if the implementation does not in fact make functions as block
> > children block-local, hoisted to top of block. We don't yet (bug 585536). Hence
> > the es5strict-blocking bug 609832.
> > 
> > See also bug 497869.
> 
> Hold on, but WebKit doesn't actually make them block-local. It disallows them
> (and does it per wiki suggestion, as far as I can tell —
> http://wiki.ecmascript.org/doku.php?id=conventions:no_non_standard_strict_decls).

Yes, I'm saying we will disallow our non-future-proof function sub-statements too, just as JSC does.

*If* we made function sub-statements bind block-locals, then we might let them stand in ES5 strict mode. Opinions are mixed on TC39 but not strongly divided. We are very likely to make function in block hoist to top of block, binding a block-local. Engines that permit this probably are future proof and need not ban such an extension from strict mode, esp. if it is already widely used. That's all I was getting at, and (alas) it doesn't apply to us.

> Should wiki be changed to disallow func. decl in statements to be interpreted
> as anything other than block-local, runtime-hoisted (for harmony compat). Or am
> I missing something?

The wiki is not a normative spec. Those conventions have not been vetted by TC39. They are a work in progress.

/be
Comment 39 Brendan Eich [:brendan] 2010-11-04 23:23:42 PDT
For an opinion contrary to Mark's convention wiki page, from another TC39'er (and Googler), see

http://www.mail-archive.com/es-discuss@mozilla.org/msg05017.html

/be
Comment 40 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-11-09 10:07:50 PST
Comment on attachment 483730 [details] [diff] [review]
Patch with updates tests



>+  nsEventStatus status = nsEventStatus_eIgnore;
>+
>+  nsEventDispatcher::Dispatch(static_cast<nsIContent*>(this), context,
>+                              &event, nsnull, &status);
You don't need the two last parameters.
Comment 41 David Zbarsky (:dzbarsky) 2010-11-09 13:58:14 PST
Created attachment 489288 [details] [diff] [review]
Refreshed patch r=smaug
Comment 42 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2010-11-12 01:05:18 PST
Note, the patch makes interface changes.
Comment 43 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-12 11:11:11 PST
Comment on attachment 489288 [details] [diff] [review]
Refreshed patch r=smaug

Good point. So I guess we won't be taking this for FF4 then.
Comment 44 :Ms2ger 2011-02-27 08:57:17 PST
You forgot to change the uuid for nsIDOMHTMLLabelElement. Also, I think you need to rev the uuid for all subclasses of nsIDOMHTMLElement.
Comment 45 David Zbarsky (:dzbarsky) 2011-03-26 10:56:49 PDT
Created attachment 522126 [details] [diff] [review]
Patch r=smaug

Refreshed to current trunk and updated the IID's.
Comment 46 :Ehsan Akhgari (busy, don't ask for review please) 2011-03-31 22:21:38 PDT
The patch doesn't apply cleanly to trunk any more.
Comment 47 David Zbarsky (:dzbarsky) 2011-04-01 15:03:41 PDT
Created attachment 523696 [details] [diff] [review]
Unbitrotted path r=smaug
Comment 48 :Ms2ger 2011-04-02 02:35:05 PDT
Landed on cedar: http://hg.mozilla.org/projects/cedar/rev/88eb5b5bb5d2

Causing permaorange there:

REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/content/html/content/reftests/409604-1.html | load failed: timed out waiting for reftest-wait to be removed
Comment 49 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-02 04:56:48 PDT
I backed it out from cedar because of the orange: http://hg.mozilla.org/projects/cedar/rev/bc3d761754da
Comment 50 David Zbarsky (:dzbarsky) 2011-04-06 13:53:24 PDT
Created attachment 524255 [details] [diff] [review]
Unbitrotted path r=smaug

Gah, forgot to disable the reftest-wait class.
This one passes try server.
Comment 51 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-08 08:59:20 PDT
Sorry, this bounced.  I landed it on cedar <http://hg.mozilla.org/projects/cedar/rev/4910cd19bc9a> but it broke the builds <http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1302255081.1302255741.9278.gz> when I merged with mozilla-central which had the fix for bug 581177.  So I had to back it out :( <http://hg.mozilla.org/projects/cedar/rev/e838e2dc618e>
Comment 52 :Ms2ger 2011-04-11 08:36:32 PDT
Created attachment 525078 [details] [diff] [review]
Merged to tip

Pushed to try as rev be8f9b90b013
Comment 53 :Ms2ger 2011-04-11 11:20:22 PDT
Thanks for your patch!

http://hg.mozilla.org/mozilla-central/rev/74e91dcb802d
Comment 54 Eric Shepherd [:sheppy] 2011-04-12 13:45:20 PDT
Rewrote the HTMLElement interface page:

https://developer.mozilla.org/en/DOM/HTMLElement

Also updated:

https://developer.mozilla.org/en/DOM/element.click

Also added to Firefox 5 for developers.
Comment 55 :Ms2ger 2011-06-13 12:34:31 PDT
*** Bug 82951 has been marked as a duplicate of this bug. ***
Comment 56 saltradio 2011-06-23 04:03:35 PDT
This bug has broken the navigation on the website tbg.nu which uses document.getElementById('').click()

1. Go to http://www.tbg.nu
2. Open any topic with at least 2 messages
3. Click on the Prev or Next button

Expected results: Move to previous or next message
Actual results: Nothing

All other browsers (and previous Firefox versions) behave correctly.
Comment 57 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-06-23 08:56:14 PDT
http://www.tbg.nu assumes that click() trigger's <a>.
Comment 58 Thomas Ahlblom 2011-06-26 04:29:08 PDT
This bug has also broken the navigation on http://sympatico.ca as shown in bug 666769.

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