The default bug view has changed. See this FAQ.

support accesskey attribute

RESOLVED FIXED

Status

()

Core
Event Handling
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

(Assignee)

Description

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

Updated

11 years ago
Assignee: xforms → surkov.alexander
(Assignee)

Comment 1

11 years ago
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?
Status: NEW → ASSIGNED
(Assignee)

Comment 2

11 years ago
Created attachment 242271 [details]
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 :).
(Assignee)

Comment 3

11 years ago
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

11 years ago
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

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

Comment 6

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

Comment 7

10 years ago
Created attachment 249267 [details] [diff] [review]
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
Attachment #249267 - Flags: review?(mats.palmgren)
(Assignee)

Comment 8

10 years ago
Comment on attachment 249267 [details] [diff] [review]
try

wrong patch
Attachment #249267 - Flags: review?(mats.palmgren)
(Assignee)

Comment 9

10 years ago
Created attachment 249268 [details] [diff] [review]
patch
Attachment #249267 - Attachment is obsolete: true
Attachment #249268 - Flags: review?(mats.palmgren)
(Assignee)

Updated

10 years ago
Attachment #249268 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

10 years ago
Assignee: surkov.alexander → events
Status: ASSIGNED → NEW
Component: XForms → Event Handling
QA Contact: spride → ian
(Assignee)

Updated

10 years ago
Assignee: events → surkov.alexander
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| ?
Attachment #249268 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Updated

10 years ago
Attachment #249268 - Attachment is obsolete: true
Attachment #249268 - Flags: review?(mats.palmgren)
(Assignee)

Comment 11

10 years ago
Created attachment 250358 [details] [diff] [review]
patch2
Attachment #250358 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 12

10 years ago
(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?
Status: NEW → ASSIGNED
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?
(Assignee)

Comment 14

10 years ago
(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 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.
(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.
(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 on attachment 250358 [details] [diff] [review]
patch2

r- until comments addressed
Attachment #250358 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 19

10 years ago
Created attachment 251169 [details] [diff] [review]
patch3
Attachment #250358 - Attachment is obsolete: true
(Assignee)

Comment 20

10 years ago
Created attachment 251171 [details] [diff] [review]
patch4

fixed accesskey handling for select1/select
label of range with accesskey shows accesskey
Attachment #251169 - Attachment is obsolete: true
Attachment #251171 - Flags: review?(Olli.Pettay)
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.
Attachment #251171 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 22

10 years ago
Created attachment 251657 [details] [diff] [review]
patch5
Attachment #251171 - Attachment is obsolete: true
Attachment #251657 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 23

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

Comment 25

10 years ago
Comment on attachment 251657 [details] [diff] [review]
patch5

Mats, I'd like to fix smaug's nits after your comments.
Attachment #251657 - Flags: superreview?(mats.palmgren)
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.
Attachment #251657 - Flags: superreview?(mats.palmgren) → superreview-
Created attachment 253564 [details]
Testcase #2, HTML label event issue
(Assignee)

Comment 28

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

Comment 29

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

Comment 30

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

Comment 31

10 years ago
Created attachment 253602 [details] [diff] [review]
patch6
Attachment #251657 - Attachment is obsolete: true
Attachment #253602 - Flags: superreview?(mats.palmgren)
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.
Attachment #253602 - Flags: superreview?(mats.palmgren) → superreview+
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)
(Assignee)

Comment 34

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

Comment 35

10 years ago
Created attachment 253742 [details] [diff] [review]
patch7
Attachment #253602 - Attachment is obsolete: true
(Assignee)

Comment 36

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

Comment 37

10 years ago
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
Attachment #253742 - Flags: superreview?(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
Attachment #253742 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 39

10 years ago
Created attachment 254885 [details] [diff] [review]
patch8

needs to be checked in
Attachment #253742 - Attachment is obsolete: true
(Assignee)

Comment 40

10 years ago
checked in by smaug, no 'xf-to-branch' whiteboard status, patch is for trunk only.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Blocks: 370487

Updated

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