All HTML Elements should have click() and accesskey

RESOLVED FIXED in mozilla5

Status

()

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

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

(Depends on: 1 bug, {dev-doc-complete, html5})

Trunk
mozilla5
dev-doc-complete, html5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

(URL)

Attachments

(3 attachments, 13 obsolete attachments)

527 bytes, text/html
Details
125.65 KB, patch
Details | Diff | Splinter Review
123.76 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

7 years ago
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.
Attachment #461828 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 2

7 years ago
Looks like I missed the IID on Label and some whitespace is weird.
(Assignee)

Updated

7 years ago
Depends on: 583533
(Assignee)

Comment 3

7 years ago
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."
Depends on: 556743
(Assignee)

Comment 4

7 years ago
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."?
Attachment #461942 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 5

7 years ago
Created attachment 461943 [details] [diff] [review]
Part 2: Elements with an accesskey are commands

qref
Attachment #461942 - Attachment is obsolete: true
Attachment #461943 - Flags: review?(Olli.Pettay)
Attachment #461942 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
Attachment #461943 - Attachment is patch: true
Attachment #461943 - Attachment mime type: application/octet-stream → text/plain
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?
Attachment #461828 - Flags: review?(Olli.Pettay) → review-
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
Attachment #461943 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 8

7 years ago
> 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 ;)
(Assignee)

Comment 9

7 years ago
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?
(Assignee)

Comment 10

7 years ago
Created attachment 462980 [details]
testcase
(Assignee)

Comment 11

7 years ago
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().
(Assignee)

Comment 12

7 years ago
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.
Attachment #461828 - Attachment is obsolete: true
Attachment #463038 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 13

7 years ago
Created attachment 463040 [details] [diff] [review]
Move focus, blur. click, accesskey to nsIDOMHTMLElement v2

this time for reals
Attachment #461943 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #463040 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
Attachment #463038 - Attachment is obsolete: true
Attachment #463038 - Flags: review?(Olli.Pettay)
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.
Attachment #463040 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 15

7 years ago
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)
Attachment #463040 - Attachment is obsolete: true
Attachment #465515 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 16

7 years ago
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.
Attachment #465515 - Attachment is obsolete: true
Attachment #465747 - Flags: review?(Olli.Pettay)
Attachment #465515 - Flags: review?(Olli.Pettay)
(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?
I'll review this tomorrow (need to be careful with testcases too, in this case, since we're changing focus handling).
(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 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.
Attachment #465747 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 21

7 years ago
(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?
(Assignee)

Comment 22

7 years ago
(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.
(Assignee)

Comment 23

7 years ago
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>
> 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.
(Assignee)

Comment 25

7 years ago
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.
Attachment #465747 - Attachment is obsolete: true
Attachment #468343 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 26

7 years ago
Created attachment 468359 [details] [diff] [review]
Patch v2.3
Attachment #468343 - Attachment is obsolete: true
Attachment #468359 - Flags: review?(Olli.Pettay)
Attachment #468343 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
Blocks: 583533
No longer depends on: 583533
(Assignee)

Updated

7 years ago
Summary: Implement HTML5 Command API → All HTML Elements should have click() and accesskey
(Assignee)

Comment 27

7 years ago
Changing title to reflect goal of bug
Sorry for the delay.
Have you tested accesskey + contentEditable?
Comment on attachment 468359 [details] [diff] [review]
Patch v2.3

Clearing review until I get answer to my question.
Attachment #468359 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 30

7 years ago
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.
Attachment #468359 - Attachment is obsolete: true
Attachment #483730 - Flags: review?(Olli.Pettay)
Duplicate of this bug: 148585

Updated

7 years ago
Keywords: html5

Comment 32

7 years ago
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>)".)
blocking2.0: --- → ?
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.
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.
(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

7 years ago
(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.
(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?
(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
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 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.
Attachment #483730 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 41

7 years ago
Created attachment 489288 [details] [diff] [review]
Refreshed patch r=smaug
Attachment #483730 - Attachment is obsolete: true
blocking2.0: ? → -
Attachment #489288 - Flags: approval2.0+
Note, the patch makes interface changes.
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.
Attachment #489288 - Flags: approval2.0+ → approval2.0-
You forgot to change the uuid for nsIDOMHTMLLabelElement. Also, I think you need to rev the uuid for all subclasses of nsIDOMHTMLElement.
(Assignee)

Comment 45

6 years ago
Created attachment 522126 [details] [diff] [review]
Patch r=smaug

Refreshed to current trunk and updated the IID's.
Attachment #489288 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
No longer depends on: 556743
The patch doesn't apply cleanly to trunk any more.
Keywords: checkin-needed
(Assignee)

Comment 47

6 years ago
Created attachment 523696 [details] [diff] [review]
Unbitrotted path r=smaug
Attachment #522126 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Keywords: dev-doc-needed
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
I backed it out from cedar because of the orange: http://hg.mozilla.org/projects/cedar/rev/bc3d761754da
Keywords: checkin-needed
(Assignee)

Comment 50

6 years ago
Created attachment 524255 [details] [diff] [review]
Unbitrotted path r=smaug

Gah, forgot to disable the reftest-wait class.
This one passes try server.
Attachment #523696 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
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>
Keywords: checkin-needed
Created attachment 525078 [details] [diff] [review]
Merged to tip

Pushed to try as rev be8f9b90b013
Thanks for your patch!

http://hg.mozilla.org/mozilla-central/rev/74e91dcb802d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
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.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 650629

Updated

6 years ago
Duplicate of this bug: 82951

Comment 56

6 years ago
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.
http://www.tbg.nu assumes that click() trigger's <a>.
Depends on: 666604

Updated

6 years ago
Depends on: 666985

Comment 58

6 years ago
This bug has also broken the navigation on http://sympatico.ca as shown in bug 666769.

Updated

6 years ago
Depends on: 667632, 666955, 666674

Updated

6 years ago
Depends on: 668223

Updated

3 years ago
Depends on: 1125813
You need to log in before you can comment on or make changes to this bug.