1037990 - AccessKeyLabel should be available for detached nodes

Status: RESOLVED FIXED

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

Description

[Timo Tijhof]

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

Comment 1

[Timo Tijhof]

Test case: http://jsfiddle.net/R3fNY/

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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..."

Comment 4

[Kazami]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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()

Comment 7

[Kazami]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 9

[Kazami]

Thanks for your help.
I would try to understand what does GetAccessKeyLabelPrefix do and try to fix it.

Comment 10

[Akshendra Pratap Singh]

Is someone really working on this? If not then I would like to.

Comment 11

[Josh Matthews [:jdm]]

Go for it.

Comment 12

[Akshendra Pratap Singh]

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.

Comment 13

[Akshendra Pratap Singh]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

> 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 15

[Akshendra Pratap Singh]

Attachment: Made GetAcessKeyLabelPrefix static and remove the use of nsPresContext

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Akshendra Pratap Singh]

What kind of test?

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

[Akshendra Pratap Singh]

Do xmlDoc elements have the accessKey attribute?

Comment 20

[Boris Zbarsky [:bzbarsky]]

They do if they're in the HTML namespace <http://www.w3.org/1999/xhtml>.

Comment 21

[Akshendra Pratap Singh]

> 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?

Comment 22

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 23

[Akshendra Pratap Singh]

While trying to run the test, I am getting this error,
>TypeError: window.document.body is null
Any suggestions?

Comment 24

[Akshendra Pratap Singh]

Oh got it. Was silly to ask?

Comment 25

[Akshendra Pratap Singh]

Attachment: Fixed the code formatting issues

Comment 26

[Akshendra Pratap Singh]

Attachment: Mochitest test

Comment 27

[Akshendra Pratap Singh]

Attachment: Fixed the code formatting issues

Comment 28

[Boris Zbarsky [: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.

Comment 29

[Akshendra Pratap Singh]

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?

Comment 30

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 31

[Akshendra Pratap Singh]

Attachment: 1037990.patch

Comment 32

[Boris Zbarsky [: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!

Comment 33

[Akshendra Pratap Singh]

Attachment: 1037990.patch

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8505495 [details] [diff] [review]
1037990.patch

r=me

Comment 35

[Akshendra Pratap Singh]

Where?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Where which?

Comment 37

[Akshendra Pratap Singh]

Comment #34
> r=me

Comment 38

[Boris Zbarsky [:bzbarsky]]

That just means the patch has review.

I'll push it to try in a bit.

Comment 39

[Boris Zbarsky [:bzbarsky]]

https://tbpl.mozilla.org/?tree=Try&rev=b1985fc2c854

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f361ad6e908

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f361ad6e908

Comment 42

[Boris Zbarsky [:bzbarsky]]

Akshendra Pratap Singh, thank you for fixing this!

Comment 43

[Akshendra Pratap Singh]

So are we done here?

Comment 44

[Boris Zbarsky [:bzbarsky]]

Yep.