339287 - support accesskey attribute

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[alexander :surkov (:asurkov)]

At least two controls doesn't support @accesskey attribute. Thease are <input appearance="full" type="xsd:date"/> and <output appearance="full" type="xsd:date"/>.

Comment 1

[alexander :surkov (:asurkov)]

I think we should get rid all inheritances of accesskey attribute by underlying widgets. Almost always this approach works but I guess it's wrong since document where xforms elements are hosted has two and the same accesskeys. One accesskey attribue is on xforms elmenent, other accesskey attribute is on underlying control.

It looks more right to register directly accesskeys via nsEventStateManager. Here I can see two ways:
1) XForms controls behave like it do analogues controls from host document. This looks reasonable and nsEventStateManger should works fine since it knows about xul and xhtml. But some xforms controls hasn't underlying controls of host document (like select appearance='full') and nsEventStateManager will unable to focus xforms controls.
2) XForms controls behave in the same manner independent of sort of host document. Imo that looks not bad too. But here we should let to know to nsEventStateManger how to focus xforms controls.

What do you, guys, think about it?

Comment 2

[alexander :surkov (:asurkov)]

Attachment: xhtml testcase

The controls those accesskey isn't work for:

input[type="xsd:date"][appearance="full"] - control hasn't unique underlying widget
trigger[appearance="minimal"] - due to bug 356660
select/select1[appearance="full"] - control hasn't unique underlying widget
range - I think it's our bad :).

Comment 3

[alexander :surkov (:asurkov)]

Talked with smaug on irc, we figured out nice way (imo) how to change nsEventStateMangager so that xforms can use it. The idea is to move part of nsEventStateManager::handleAccesskey() where focusing of element is happen to nsIContent implementations. So that nsGenericElement/nsXULElement/nsIXTFElementWrapper should implement that method.

The question about approach how we will handle accesskeys for xforms is steal open.

1) Try to redirect accesskey handling to native widget.
2) Call methods of nsIXFormsUIWidget, for example, focus().

Comment 4

[Aaron Leventhal]

Please note that bug 143065 modifies HandleAccessKey quite a bit, and it's basically done just waiting for final touches. Might want to help push that through before messing around there more.

Comment 5

[aaronr]

(In reply to comment #1)
> I think we should get rid all inheritances of accesskey attribute by underlying
> widgets. Almost always this approach works but I guess it's wrong since
> document where xforms elements are hosted has two and the same accesskeys. One
> accesskey attribue is on xforms elmenent, other accesskey attribute is on
> underlying control.
> 
> It looks more right to register directly accesskeys via nsEventStateManager.
> Here I can see two ways:
> 1) XForms controls behave like it do analogues controls from host document.
> This looks reasonable and nsEventStateManger should works fine since it knows
> about xul and xhtml. But some xforms controls hasn't underlying controls of
> host document (like select appearance='full') and nsEventStateManager will
> unable to focus xforms controls.
> 2) XForms controls behave in the same manner independent of sort of host
> document. Imo that looks not bad too. But here we should let to know to
> nsEventStateManger how to focus xforms controls.
> 
> What do you, guys, think about it?
> 


my problem with changing this would be that this would be trunk only.  We'd still have to support the old way on 1.8.1 and 1.8.0.  If we can keep the xforms changes as small and contained as possible, I have no problem with it.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> my problem with changing this would be that this would be trunk only.  We'd
> still have to support the old way on 1.8.1 and 1.8.0.  If we can keep the
> xforms changes as small and contained as possible, I have no problem with it.
> 

Right, it's only for trunk. I think the changes will be the next. In XBL part it is removing only xbl:inherits="accesskey" lines. And plus some special code in XTF part (probably additional method handleAccesskey() of nsXFormsStubElement). To be more precisely I should wait for bug 143065 and bug 356660.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: try

1) added performAccesskey for nsIContent
2) nsGenericHTMLElement/nsHTMLLabelElement/nsXULElement/nsXTFElementWrapper implement it
3) nsHTMLLabelElement uses the logic proposed in patch4 of bug 222297.
4) nsXTFElementWrapper redirects call to nsXTFElement.
5) xforms xtf element implements the method

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 249267 [details] [diff] [review]
try

wrong patch

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 10

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

Comment on attachment 249268 [details] [diff] [review]
patch


>-          // Propagate trusted state to the new event.
>-          nsEventStatus status = nsEventStatus_eIgnore;
>-          nsMouseEvent event(NS_IS_TRUSTED_EVENT(aEvent), NS_MOUSE_CLICK,
>-                             nsnull, nsMouseEvent::eReal);
>-
>-          nsAutoPopupStatePusher popupStatePusher(NS_IS_TRUSTED_EVENT(aEvent) ?
>-                                                  openAllowed : openAbused);
>-

Here NS_IS_TRUSTED_EVENT(aEvent) is used, but you change that to



>+nsGenericHTMLElement::PerformAccesskey(PRBool aKeyCausesActivation)
....
>+    // Click on it if the users prefs indicate to do so.
>+    nsEventStatus status = nsEventStatus_eIgnore;
>+    nsMouseEvent event(nsContentUtils::IsCallerChrome(), NS_MOUSE_CLICK,
>+                       nsnull, nsMouseEvent::eReal);
>+
>+    nsAutoPopupStatePusher popupStatePusher(nsContentUtils::IsCallerChrome() ?
>+                                            openAllowed : openAbused);

nsContentUtils::IsCallerChrome(). Why that change.

+
+    nsEventDispatcher::Dispatch(content, presContext, &event, nsnull, &status);

You don't actually need the |status|. (I know it is currently there in ESM.)




>+void
>+nsXULElement::PerformAccesskey(PRBool aKeyCausesActivation)
>+{
>+  nsCOMPtr<nsIContent> content(this);

Why do you need |content| ?

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #10)

> >+void
> >+nsXULElement::PerformAccesskey(PRBool aKeyCausesActivation)
> >+{
> >+  nsCOMPtr<nsIContent> content(this);
> 
> Why do you need |content| ?
> 

The content is either this or control that label points to. It is used in code below. Or do you mean something else?

Comment 13

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

Comment on attachment 250358 [details] [diff] [review]
patch2

> 
>+void
>+nsXULElement::PerformAccesskey(PRBool aKeyCausesActivation,
>+                               PRBool aIsTrustedEvent)
>+{
>+  nsCOMPtr<nsIContent> content(this);
>+
>+  if (Tag() == nsGkAtoms::label) {
>+    nsCOMPtr<nsIDOMElement> element;
>+
>+    nsAutoString control;
>+    GetAttr(kNameSpaceID_None, nsGkAtoms::control, control);
>+    if (!control.IsEmpty()) {
>+      nsCOMPtr<nsIDOMDocument> domDocument =
>+        do_QueryInterface(content->GetDocument());

GetDocument() is deprecated. Use GetCurrentDoc() ?

XForms part looks ok to me.

Question, do we want to hardcode it to XTF that the attribute name
for access key is really |accesskey| in every case? Is it possible that
some xml language uses some other attribute name?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> Question, do we want to hardcode it to XTF that the attribute name
> for access key is really |accesskey| in every case? Is it possible that
> some xml language uses some other attribute name?
> 

It's interesting idea though we haven't usecases for this I guess. But I think it's easy enough to support this. Do we need this?

Comment 15

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

Comment on attachment 250358 [details] [diff] [review]
patch2


>+void
>+nsXULElement::PerformAccesskey(PRBool aKeyCausesActivation,
>+                               PRBool aIsTrustedEvent)
>+{
>+  nsCOMPtr<nsIContent> content(this);
>+
>+  if (Tag() == nsGkAtoms::label) {
>+    nsCOMPtr<nsIDOMElement> element;
>+
>+    nsAutoString control;
>+    GetAttr(kNameSpaceID_None, nsGkAtoms::control, control);
>+    if (!control.IsEmpty()) {

Maybe
if (GetAttr(kNameSpaceID_None, nsGkAtoms::control, control))

>+      nsCOMPtr<nsIDOMDocument> domDocument =
>+        do_QueryInterface(content->GetDocument());
>+      if (domDocument)
>+        domDocument->GetElementById(control, getter_AddRefs(element));
>+    }
>+    // ... that here we'll either change |content| to the element
>+    // referenced by |element|, or clear it.
>+    content = do_QueryInterface(element);
>+  }

What if content is null? You're removing a null check which is there
currently in ESM.

I'd make it possible to define access key attribute name, just to be consistent with
class attribute name.

Comment 16

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

(In reply to comment #15)

> What if content is null? You're removing a null check which is there
> currently in ESM.
> 

nm, GetPrimaryFrameFor is null safe.

Comment 17

[Mats Palmgren (inactive)]

(In reply to comment #16)
> nm, GetPrimaryFrameFor is null safe.

Yes, but you shouldn't call it with null though, it will trigger a NS_WARNING.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsFrameManager.cpp&rev=1.241&root=/cvsroot&mark=344#340

Comment 18

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

Comment on attachment 250358 [details] [diff] [review]
patch2

r- until comments addressed

Comment 19

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch4

fixed accesskey handling for select1/select
label of range with accesskey shows accesskey

Comment 21

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

Comment on attachment 251171 [details] [diff] [review]
patch4


>+  /**
>+   * The method focues (or activates) element that accesskey is bound to. It is
>+   * called when accesskey was activated.

"Focuses",
"when accesskey is activated", maybe?


>+   *
>+   * @param aKeyCausesActivation - if true then HTML element should be activated

HTML element?

>@@ -424,16 +444,24 @@ nsXTFElementWrapper::UnsetAttr(PRInt32 a
>   else { // try wrapper
>     rv = nsXTFElementWrapperBase::UnsetAttr(aNameSpaceID, aAttr, aNotify);
>   }
> 
>   if (aNameSpaceID == kNameSpaceID_None &&
>       (mNotificationMask & nsIXTFElement::NOTIFY_ATTRIBUTE_REMOVED))
>     GetXTFElement()->AttributeRemoved(aAttr);
> 
>+  if (mNotificationMask & nsIXTFElement::NOTIFY_PERFORM_ACCESSKEY) {
>+    nsCOMPtr<nsIDOMAttr> accesskey;
>+    GetXTFElement()->GetAccesskeyNode(getter_AddRefs(accesskey));
>+    nsCOMPtr<nsIAttribute> attr(do_QueryInterface(accesskey));
>+    if (attr && attr->NodeInfo()->Equals(aAttr, aNameSpaceID))
>+      RegUnregAccessKey(PR_TRUE);
>+  }
>+

Shouldn't this be before the attribute is actually removed from element,
I mean before nsXTFElementWrapperBase::UnsetAttr call.

> 
>+void
>+nsXULElement::PerformAccesskey(PRBool aKeyCausesActivation,
>+                               PRBool aIsTrustedEvent)
>+{
>+  nsCOMPtr<nsIContent> content(this);
>+
>+  if (Tag() == nsGkAtoms::label) {
>+    nsCOMPtr<nsIDOMElement> element;
>+
>+    nsAutoString control;
>+    GetAttr(kNameSpaceID_None, nsGkAtoms::control, control);
>+    if (!control.IsEmpty()) {
>+      nsCOMPtr<nsIDOMDocument> domDocument =
>+        do_QueryInterface(content->GetCurrentDoc());
>+      if (domDocument)
>+        domDocument->GetElementById(control, getter_AddRefs(element));
>+    }
>+    // ... that here we'll either change |content| to the element
>+    // referenced by |element|, or clear it.

Why you kept this part of the comment but not "// If anything fails, this will be null ..." .
Not that the comment is too good in any case. Maybe you could remove the  "...that" comment.

In nsXULElement indentation is 4 spaces.

Comment 22

[alexander :surkov (:asurkov)]

Attachment: patch5

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> 
> In nsXULElement indentation is 4 spaces.
> 

I'd like to save 2 spaces because some methods of nsXULElement has 2 spaces and mozilla style assume 2 spaces too.

Comment 24

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

Comment on attachment 251657 [details] [diff] [review]
patch5


>+  /**
>+   * The method focuses (or activates) element that accesskey is bound to. It is
>+   * called when accesskey is activated.
>+   *
>+   * @param aKeyCausesActivation - if true then element should be activated
>+   * @param aIsTrustedEvent - if true then event that is cause of accesskey
>+   *                          execution is trusted.
>+   */


The comment is not actually quite right. Maybe something like
"This method is called when the accesskey bound to an element is activated."

Because nsXULElement is marked to use 4 spaces, better to stick with that.

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 251657 [details] [diff] [review]
patch5

Mats, I'd like to fix smaug's nits after your comments.

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 251657 [details] [diff] [review]
patch5

> Index: content/html/content/src/nsGenericHTMLElement.cpp
> +  // It's hard to say what HTML4 wants us to do in all cases.
> +  esm->ChangeFocusWith(NS_STATIC_CAST(nsIContent*, this),
> +                       nsIEventStateManager::eEventFocusedByKey);

Why NS_STATIC_CAST? can't you just use 'this' as is?
Also, the comment as it stands doesn't add any value IMO. We might
as well just remove it.

> +    nsEventStatus status = nsEventStatus_eIgnore;
> +    nsEventDispatcher::Dispatch(NS_STATIC_CAST(nsIContent*, this), ...

'status' is not used
NS_STATIC_CAST again

> Index: content/html/content/src/nsHTMLLabelElement.cpp
> +void
> +nsHTMLLabelElement::PerformAccesskey(PRBool aKeyCausesActivation,
> +                                     PRBool aIsTrustedEvent)

This makes me worried. See the testcase I will attach -
you have lost the "LABEL:click" event.
Would it be possible to keep the HTML label event handling as is?

There is a GetForContent() method to find the 'for'-target...

> Index: content/xul/content/src/nsXULElement.cpp
> +  const nsStyleVisibility* vis = frame->GetStyleVisibility();
> +  PRBool viewShown = frame->AreAncestorViewsVisible();
> +
> +  if (!viewShown || vis->mVisible == NS_STYLE_VISIBILITY_COLLAPSE ||
> +      vis->mVisible == NS_STYLE_VISIBILITY_HIDDEN)

I would prefer:
   if (vis->mVisible == NS_STYLE_VISIBILITY_COLLAPSE ||
       vis->mVisible == NS_STYLE_VISIBILITY_HIDDEN ||
       !frame->AreAncestorViewsVisible())

> Index: extensions/xforms/resources/content/input-xhtml.xml
>    <binding id="xformswidget-input-boolean"
> ...
> -      <html:input anonid="control" xbl:inherits="accesskey" type="checkbox"
> +      <html:input anonid="control" type="checkbox"
>                    class="xf-value"/>

Will now fit on one line.

Marking sr- for now, until the HTML label event issue is resolved.

Comment 27

[Mats Palmgren (inactive)]

Attachment: Testcase #2, HTML label event issue

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #26)

> > Index: content/html/content/src/nsHTMLLabelElement.cpp
> > +void
> > +nsHTMLLabelElement::PerformAccesskey(PRBool aKeyCausesActivation,
> > +                                     PRBool aIsTrustedEvent)
> 
> This makes me worried. See the testcase I will attach -
> you have lost the "LABEL:click" event.
> Would it be possible to keep the HTML label event handling as is?

The label logic you see I took from the patch4 of bug 222297. dbaron said there
"This is just yet another workaround for the real problem I described in comment
9.  I'd rather not spread label logic all over the tree if we don't have to. 
What I described in comment 9 shouldn't be very hard to fix."

Here I keep label logic inside label code.

I made this change because current code has trick to substitute for nsEventStateManager::mCurrentTargetContent during 'click' event dispatching. Probably it doesn't make sense now for current events implementation. But I'm not sure. I will be very appritiate if Olli will give some clarification. Since I move accesskey handling into label code then I can't access for mCurrentTargetContent from there by nice way. So, what the way is preferable?

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #26)
> (From update of attachment 251657 [details] [diff] [review])
> > Index: content/html/content/src/nsGenericHTMLElement.cpp
> > +  // It's hard to say what HTML4 wants us to do in all cases.
> > +  esm->ChangeFocusWith(NS_STATIC_CAST(nsIContent*, this),
> > +                       nsIEventStateManager::eEventFocusedByKey);
> 
> Why NS_STATIC_CAST? can't you just use 'this' as is?
> Also, the comment as it stands doesn't add any value IMO. We might
> as well just remove it.

STATIC_CAST is just explicit convertion that is often used in mozilla code.

"A pointer to member type can be explicitly converted into a different pointer to member type if both types are pointers to members of the same class. This form of explicit conversion may also take place if the pointer to member types are from separate classes, however one of the class types must be derived from the other."

Should I try to remove it? I think it may involve warnings or even errors though I didn't test it yet.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #26)
> (From update of attachment 251657 [details] [diff] [review])
> > Index: content/html/content/src/nsGenericHTMLElement.cpp
> > +  // It's hard to say what HTML4 wants us to do in all cases.
> > +  esm->ChangeFocusWith(NS_STATIC_CAST(nsIContent*, this),
> > +                       nsIEventStateManager::eEventFocusedByKey);
> 
> Why NS_STATIC_CAST? can't you just use 'this' as is?
> Also, the comment as it stands doesn't add any value IMO. We might
> as well just remove it.

I remove NS_STATIC_CAST in nsGenericHTMLElement but I can't do it for nsHTMLLabelElement because I got an error "ambiguous conversions from 'nsHTMLLabelElement *const ' to 'nsISupports *'"

> > +    nsEventStatus status = nsEventStatus_eIgnore;
> > +    nsEventDispatcher::Dispatch(NS_STATIC_CAST(nsIContent*, this), ...
> 
> 'status' is not used
> NS_STATIC_CAST again

Fixed.

> > Index: content/html/content/src/nsHTMLLabelElement.cpp
> > +void
> > +nsHTMLLabelElement::PerformAccesskey(PRBool aKeyCausesActivation,
> > +                                     PRBool aIsTrustedEvent)
> 
> This makes me worried. See the testcase I will attach -
> you have lost the "LABEL:click" event.
> Would it be possible to keep the HTML label event handling as is?

Fixed

Comment 31

[alexander :surkov (:asurkov)]

Attachment: patch6

Comment 32

[Mats Palmgren (inactive)]

Comment on attachment 253602 [details] [diff] [review]
patch6

Looks good so far.  I found one more issue while (briefly) testing some
XForms elements and saw that accesskey for <xf:label> did not work. 
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/xforms/nsXFormsLabelElement.cpp&rev=1.19&root=/cvsroot&mark=110,113-116#107
(the NOTIFY_PERFORM_ACCESSKEY that nsXFormsDelegateStub added is lost)
Could you look through the various XForms classes and add the bit where
needed please. With that sr=mats.

Comment 33

[Mats Palmgren (inactive)]

It might also be worth filing a bug on making a better interface for
subclasses to add/remove bits to the NotificationMask so the wrapper
can evolve without having to update every wrapped class...  e.g.
ModifyNotificationMask(PRUint32 bits, PRBool addOrRemove)

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #33)
> It might also be worth filing a bug on making a better interface for
> subclasses to add/remove bits to the NotificationMask so the wrapper
> can evolve without having to update every wrapped class...  e.g.
> ModifyNotificationMask(PRUint32 bits, PRBool addOrRemove)
> 

Good idea, I filed bug 369069 to address it.

Comment 35

[alexander :surkov (:asurkov)]

Attachment: patch7

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #32)
> (From update of attachment 253602 [details] [diff] [review])
> Looks good so far.  I found one more issue while (briefly) testing some
> XForms elements and saw that accesskey for <xf:label> did not work. 
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/xforms/nsXFormsLabelElement.cpp&rev=1.19&root=/cvsroot&mark=110,113-116#107
> (the NOTIFY_PERFORM_ACCESSKEY that nsXFormsDelegateStub added is lost)
> Could you look through the various XForms classes and add the bit where
> needed please. With that sr=mats.
> 

Thank you, good catch. Fixed. The other XForms UI controls looks working.

Comment 37

[alexander :surkov (:asurkov)]

Comment on attachment 253742 [details] [diff] [review]
patch7

As Olli proposed Im asking one more sr, since mats is not listed at http://www.mozilla.org/hacking/reviewers.html

Comment 38

[Johnny Stenback (:jst)]

Comment on attachment 253742 [details] [diff] [review]
patch7

- In nsXULElement::PerformAccesskey():

+    if (Tag() == nsGkAtoms::label) {
+        nsCOMPtr<nsIDOMElement> element;
+
+      nsAutoString control;
+      GetAttr(kNameSpaceID_None, nsGkAtoms::control, control);
...

Clean up the indentation here please (stick to 4-space indentation like the surrounding code does).

sr=jst

Comment 39

[alexander :surkov (:asurkov)]

Attachment: patch8

needs to be checked in

Comment 40

[alexander :surkov (:asurkov)]

checked in by smaug, no 'xf-to-branch' whiteboard status, patch is for trunk only.