504252 - Expose STATE_HASPOPUP on XUL elements that have an @popup attribute

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Spun off bug 504163, inspired by the new SeaMonkey download window.

Requiring developers to put aria-haspopup on a XUL element that already has the @popup attribute set is redundant, our code should be smart enough to expose that state from the given information.

Comment 1

[Marco Zehe (:MarcoZ)]

Note to assignee: Once this has landed on a branch where SeaMonkey is built from, revert bug 504163.

Comment 2

[Marco Zehe (:MarcoZ)]

Attachment: Patch

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 389459 [details] [diff] [review]
Patch

Wrong patch, forgot to remove the aria-haspopup attribute from the testcase.

Comment 4

[Marco Zehe (:MarcoZ)]

Attachment: Correct patch

Comment 5

[alexander :surkov (:asurkov)]

How do they use STATE_HASPOPUP, how do they appear popup on the screen? Should we expose accessible actions for this?

Comment 6

[[e-mail]]

Comment on attachment 389462 [details] [diff] [review]
Correct patch

>+    if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::popup)) {
>+      nsAutoString name;
>+      content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::popup, name);
I'm not sure it's worth testing both HasAttr and GetAttr (there is a minuscule performance win since GetAttr has to truncate the passed-in string).

>+		test_states_editablebody.html \
> 		test_states_frames.html \
>+        test_states_popup.xul \
Nit: As with most Makefiles, file style is tab characters, not spaces.

Comment 7

[Marco Zehe (:MarcoZ)]

(In reply to comment #5)
> How do they use STATE_HASPOPUP, how do they appear popup on the screen? Should
> we expose accessible actions for this?

I believe if you left-click on it, or press Enter on it, a contextmenu style menu pops up. In the example from SeaMonkey, and also in the Thunderbird and SeaMonkey headers, both the APPLICATIONS key and pressing ENTER will open the context menu for each header ("From: " etc.)

As for exposing actions: A simple "click" should do the trick.

Comment 8

[Marco Zehe (:MarcoZ)]

(In reply to comment #6)
> I'm not sure it's worth testing both HasAttr and GetAttr (there is a minuscule
> performance win since GetAttr has to truncate the passed-in string).

Changed it locally to test whether GetAttr() returns true and if so, if the string is empty.

> >+		test_states_editablebody.html \
> > 		test_states_frames.html \
> >+        test_states_popup.xul \
> Nit: As with most Makefiles, file style is tab characters, not spaces.

Bah! Wonder when I'll stop falling into that stupid trap! Thanks! :)

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> I believe if you left-click on it, or press Enter on it, a contextmenu style
> menu pops up. In the example from SeaMonkey, and also in the Thunderbird and
> SeaMonkey headers, both the APPLICATIONS key and pressing ENTER will open the
> context menu for each header ("From: " etc.)
>
> As for exposing actions: A simple "click" should do the trick.

Do you mean we don't need to expose any actions or?

Comment 10

[Marco Zehe (:MarcoZ)]

Correct, I believe no extra actions are necessary.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> Correct, I believe no extra actions are necessary.

Why do we expose actions on menu items since they have state_haspopup also? What's the difference?

Comment 12

[Marco Zehe (:MarcoZ)]

Attachment: Patch2, with action added

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 395820 [details] [diff] [review]
Patch2, with action added


>+  // Check if a XUL element has the popup attribute (an attached context menu).

nit: not context, but popup menu in terms of XUL.

>+  if (content->IsNodeOfType(nsINode::eXUL)) {
>+    nsAutoString name;
>+    if (content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::popup, name))
>+      if (!name.IsEmpty())
>+        *aState |= nsIAccessibleStates::STATE_HASPOPUP;
>+  }

>+  // Return "click" action on elements that have an attached context menu.
>+  if (content->IsNodeOfType(nsINode::eXUL))
>+    if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::popup))
>+      return eClickAction;

This logic is different from GetStatesInternal(). Would we rather have utility method to check this? On another hand should we ensure referred popup element exists in the DOM?

>+		test_states_popup.xul \

probably it's worth to have general tests like test_states.xul. It's too luxuriously to have own test for @popup attribute I think.

>       <button type="menu" id="buttonmenu" label="button">
>         <menupopup>
>           <menuitem label="item1" id="buttonmenu_item"/>
>           <menuitem label="item1"/>
>         </menupopup>
>       </button>
>+      <label id="fileName" crop="center" value=""
>+             context="fileContext" popup="fileContext"
>+             style="-moz-user-focus: normal;"/>

nit: empty line please between button and label.
nit: @context attribute is not needed.
nit: @style="-moz" - I would prefer to use @tabindex here if it works ;)
nit: why @id="fileName"?
nit: why do we need @crop and empty value?

cancelling review

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #13)
> On another hand should we ensure referred popup element
> exists in the DOM?

I also considered this, but I lean towards not checking the DOM for the existence of a popup id.

re: util method, maybe ::HasDefinedAttribute?

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 395820 [details] [diff] [review]
Patch2, with action added

1 additional nit:

>+  // Check if a XUL element has the popup attribute (an attached context menu).
>+  if (content->IsNodeOfType(nsINode::eXUL)) {
>+    nsAutoString name;
>+    if (content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::popup, name))

My pref for variable name for the attribute value is "popupId" instead of "name".

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> (In reply to comment #13)
> > On another hand should we ensure referred popup element
> > exists in the DOM?
> 
> I also considered this, but I lean towards not checking the DOM for the
> existence of a popup id.

the question is sort of should we try to fix markup errors when we can?

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > On another hand should we ensure referred popup element
> > > exists in the DOM?
> > 
> > I also considered this, but I lean towards not checking the DOM for the
> > existence of a popup id.
> 
> the question is sort of should we try to fix markup errors when we can?

It is a good question. I lean towards not fixing.

I think we should fix only when we can be certain it is both an error, and it is performant to check. I can imagine some crazy authors who might add a popup attribute with some bogus id, and on a mouse-over they change it based on the coords of the mouse (not sure that would even work, but you get the idea).

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)

> It is a good question. I lean towards not fixing.
> 
> I think we should fix only when we can be certain it is both an error, and it
> is performant to check. I can imagine some crazy authors who might add a popup
> attribute with some bogus id, and on a mouse-over they change it based on the
> coords of the mouse (not sure that would even work, but you get the idea).

Actually getDocumentById() should be quick enough since it's hash table based iirc.

Comment 19

[Marco Zehe (:MarcoZ)]

Attachment: Patch3, with comments addressed

Hope I got them all!

Comment 20

[alexander :surkov (:asurkov)]

Comment on attachment 395842 [details] [diff] [review]
Patch3, with comments addressed


>+      <label id="labelWithPopup" value="file name"
>+             popup="fileContext"
>+             tabindex="0"/>

btw, you're example is great to show we expose states and click action when there is no poup ;)

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 395842 [details] [diff] [review]
Patch3, with comments addressed

nice 1; r=me

Comment 22

[Marco Zehe (:MarcoZ)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/a2ffba621d98

Comment 23

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090825 Minefield/3.7a1pre (.NET CLR 3.5.30729)