Closed Bug 1037990 Opened 10 years ago Closed 10 years ago

AccessKeyLabel should be available for detached nodes

Categories

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

32 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: krinkle, Assigned: akshendra521994, Mentored)

References

Details

(Whiteboard: [lang=c++] [platform-rel-Wikipedia])

Attachments

(1 file, 5 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.153 Safari/537.36

Steps to reproduce:

1. Create element with accesskey attribute
2. Read node.accessKeyLabel


Actual results:

Empty string


Expected results:

The keyboard shortcut for the element. Just like it does when the node is attached.

See also:
* Specification: http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#dom-accesskeylabel
* Downstream bug: https://bugzilla.wikimedia.org/show_bug.cgi?id=67946

Original implementation:
* Gecko https://bugzilla.mozilla.org/show_bug.cgi?id=583533
* WebKit https://bugs.webkit.org/show_bug.cgi?id=72715
* Blink https://code.google.com/p/chromium/issues/detail?id=393466
Test case: http://jsfiddle.net/R3fNY/
This depends on the access key label prefix from the ESM.  But all it _really_ cares about is whether the node is in a content or chrome docshell.

So could we make this a static method that takes the node's owner document and then examines that document's container or something?  And if there is no container, just ignore the prefix bit, I'd say.  Olli, make sense?
Component: Untriaged → DOM
Flags: needinfo?(bugs)
Product: Firefox → Core
Mentor: bzbarsky
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [lang=c++]
Makes sense, though, I think in case of there is no container, we could just return empty string.
That is ok per the spec, since all the access key assigning talks about "UA may..."
Flags: needinfo?(bugs)
Hello, I am a newcomer in Firefox.
I want to try this bug. 
Now I try to study the code of DOM, is it right?
Any help is appreciated.
Kazami, the implementation of this method is in content/html/content/src/nsGenericHTMLElement.cpp.  It currently depends on there being an nsPresContext to work correctly, but doesn't actually really use any state from it otherthan the container.  So what we should do is move nsEventStateManager::GetAccessKeyLabelPrefix to being a static method, have it take the element's GetOwnerDoc() as an argument, and get the container from the nsIDocument instead of from the prescontext.
So the code here http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.cpp?rev=a0778b3a1c04#292 shouldn't depend on prescontext.
GetAccessKeyLabelPrefix() could be a static method in nsEventStateManager, and the method
should then take an Element* as param, from which you can get docshell (its container) using
element->OwnerDoc()->GetDocShell()
Thanks for your help and comments.
After reading the codes, I still don't know why using Element* as a param. 
If I use Element, the static function would be
static nsEventStateManager::GetAccessKeyLabelPrefix(Element *);
Therefore, I should take nsString &aLabel be a param for GetAccessKeyLabelPrefix.
It would be
GetAccessKeyLabelPrefix(aLabel);
Finally, in GetAccessKeyLabelPrefix, get the container by element->OwnerDoc()->GetDocShell().
Do I misunderstand ?
In Element.h(http://mxr.mozilla.org/mozilla-central/source/content/base/public/Element.h), I do not find OwnerDoc().

Your helps are highly appreciated.
> After reading the codes, I still don't know why using Element* as a param. 

So you can get to the owner document.  As you noted, you need both the Element* and the nsString& outparam.

> In Element.h I do not find OwnerDoc().

It's in nsINode; Element inherits from nsINode.
Thanks for your help.
I would try to understand what does GetAccessKeyLabelPrefix do and try to fix it.
Is someone really working on this? If not then I would like to.
Go for it.
Need a clarification, what you actually want is that, 

node.accessKey = 'e';
log('detached: ' + val(node.accessKeyLabel)); // should print 'e'
document.body.appendChild(node);
log('attached: ' + val(node.accessKeyLabel)); // should print 'Alt + Shift + e'

or both should print the same.
Because in comment #3, Boris says
" And if there is no container, just ignore the prefix bit, I'd say."
> or both should print the same.

Both should print the same always.

> " And if there is no container, just ignore the prefix bit, I'd say."

Right.  So if "document" is window.document for some window, then you would get 'Alt + Shift + e' (or Mac equivalent) both before and after the appendChild call.  But if "document" is an XHR responseXML or |new Document| return value or some other document that's not actually rendered anywhere then you would get 'e' both before and after.
Comment on attachment 8502634 [details] [diff] [review]
Made GetAcessKeyLabelPrefix static and remove the use of nsPresContext

Thanks for the patch!  This is a great start.

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
> nsGenericHTMLElement::GetAccessKeyLabel(nsString& aLabel)

Please fix the indentation in this function; you need to outdent the code by two spaces, right?

>+       EventStateManager::GetAccessKeyLabelPrefix(this,aLabel)) {

Space after ',', please.

>+  

Drop the empty line.

>+++ b/dom/events/EventStateManager.cpp
>+EventStateManager::GetAccessKeyLabelPrefix(Element* element, nsAString& aPrefix)

"aElement", not "element".

>+++ b/dom/events/EventStateManager.h
>+#include "../../content/base/public/Element.h"

Please use a forward-declaration instead ("class Element;" right between the "class DataTransfer" and "class TabParent" bits).

>+  static bool GetAccessKeyLabelPrefix(mozilla::dom::Element* element, nsAString& aPrefix);

EventStateManager is already in the "mozilla" namespace, so just "dom::Element*".

Please add a test as well?

r=me with those issues fixed, but I'd like to see the resulting diff.
Attachment #8502634 - Flags: review?(bzbarsky) → review+
What kind of test?
A mochitest testing the steps from comment 0, both in and out of document, and for both displayed and undisplayed documents.

See https://developer.mozilla.org/en-US/docs/Mochitest#Writing_tests and https://developer.mozilla.org/en-US/docs/Mochitest#Running_select_tests for more info.
Do xmlDoc elements have the accessKey attribute?
They do if they're in the HTML namespace <http://www.w3.org/1999/xhtml>.
> var parser=new DOMParser();
> var xmlDoc=parser.parseFromString('<xml xmlns:html="http://www.w3.org/1999/xhtml"></xml>',"text/xml"); 
> var nn = xmlDoc.createElement('html:a');
> nn.href = 'http://google.com'
> nn.accessKey = 'f';
> log('detached: ' + val(nn.accessKeyLabel)); // this prints "undefined"

What is the problem with the code?
> What is the problem with the code?

You want to use createElementNS and pass in the right namespace.  Calling createElement on the document won't look up namespaces based on prefix (and if it did, looking that up on the _document_ is not so useful; the prefix is bound to a namespace on some descendant element, not on the document).
While trying to run the test, I am getting this error,
>TypeError: window.document.body is null
Any suggestions?
Oh got it. Was silly to ask?
Attached patch Fixed the code formatting issues (obsolete) — Splinter Review
Attachment #8502634 - Attachment is obsolete: true
Attachment #8503548 - Flags: review?(bzbarsky)
Attached file Mochitest test (obsolete) —
Attached patch Fixed the code formatting issues (obsolete) — Splinter Review
Attachment #8503548 - Attachment is obsolete: true
Attachment #8503548 - Flags: review?(bzbarsky)
Attachment #8503552 - Flags: review?(bzbarsky)
Comment on attachment 8503552 [details] [diff] [review]
Fixed the code formatting issues

Sorry for the lag here...

> nsGenericHTMLElement::GetAccessKeyLabel(nsString& aLabel)

This still has the indentation problem, no?

>+       EventStateManager::GetAccessKeyLabelPrefix(this,aLabel)) {

And this still has the missing space.  Did you maybe forget to qrefresh?

>+  if(modifierMask != -1) {

Space before '('.  Also, please invert this test and early-return if the mask is -1, so you don't end up having to reindent all the mask checks.

This patch is missing the actual test file...  It's also missing a useful commit message.
Attachment #8503552 - Flags: review?(bzbarsky) → review-
I will fox the commit message and the spaces.
But there is a problem if I remove this check
> if(modifierMask != -1)
without this whenever the container is something like xmlDoc or /new Document/, I get the value of detached node to be like
> ctrl+meta+win+alt+shift+'e'

And about the test, what test it should be, I have attached a mochitest. Should the test be a part of the patch?
> But there is a problem if I remove this check

I didn't say to remove it.  I said to invert it.  So like this:

  if (modifierMask == -1) {
    return;
  }

and then the rest of the code.

> Should the test be a part of the patch?

Yes.  hg add or git add the file, depending on which version control system you're using.
Attached patch 1037990.patch (obsolete) — Splinter Review
Attachment #8503550 - Attachment is obsolete: true
Attachment #8503552 - Attachment is obsolete: true
Attachment #8505452 - Flags: review?(bzbarsky)
Comment on attachment 8505452 [details] [diff] [review]
1037990.patch

>+  if(modifierMask == -1) {

Still needs space after "if".

>+	function val(value) {

This seems unnecessary.  You only pass strings, so this is a complicated no-op, no?  Just use the accessKeyLabel values directly in your test.

The test is full of tabs.  Please replace the tabs with two spaces.

r=me with those fixes.  Thank you!
Attachment #8505452 - Flags: review?(bzbarsky) → review+
Attached patch 1037990.patchSplinter Review
Attachment #8505452 - Attachment is obsolete: true
Attachment #8505495 - Flags: review?(bzbarsky)
Comment on attachment 8505495 [details] [diff] [review]
1037990.patch

r=me
Attachment #8505495 - Flags: review?(bzbarsky) → review+
Where?
Where which?
Comment #34
> r=me
That just means the patch has review.

I'll push it to try in a bit.
https://hg.mozilla.org/mozilla-central/rev/2f361ad6e908
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Akshendra Pratap Singh, thank you for fixing this!
So are we done here?
QA Whiteboard: [good first verify]
Component: DOM → DOM: Core & HTML
Whiteboard: [lang=c++] → [lang=c++] [platform-rel-Wikipedia]
You need to log in before you can comment on or make changes to this bug.