349643 - implement accessible objects for xforms messages

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

XForms has two types of messages: ephemeral and non ephemeral. Non ephemeral messages is just window opened by window.openDialog() method. Ephemeral messages behaves oneself like tooltips. Ephemeral messages implementation for xhtml is html:div, that div has absolute position and when message become to be visible then div changes the it's display style. Ephemeral messages implementation for xul is based on xul:tooltip (currently, still on xul:popup).

What events should I send when message changes state on visible/hidden? Do regular opened windows/xul:tooltip do it already?

Comment 1

[aaronr]

(In reply to comment #0)
> XForms has two types of messages: ephemeral and non ephemeral. Non ephemeral
> messages is just window opened by window.openDialog() method. Ephemeral
> messages behaves oneself like tooltips. Ephemeral messages implementation for
> xhtml is html:div, that div has absolute position and when message become to be
> visible then div changes the it's display style. Ephemeral messages
> implementation for xul is based on xul:tooltip (currently, still on xul:popup).
> 
> What events should I send when message changes state on visible/hidden? Do
> regular opened windows/xul:tooltip do it already?
> 


We also have inline alerts which fit neither of these descriptions.  They are shown upon xforms-invalid and disappear upon xforms-valid, I believe.

Comment 2

[Aaron Leventhal]

Perfect. If those nodes can be marked as ROLE_ALERT they'll be spoken when they appear. Might be able to do that with xhtml:role="wairole:alert". Or you can create via nsIAccessibleProvider. An AlertActive event can be fired on the node, if it's not getting the EVENT_ALERT when it appears.

Comment 3

[Aaron Leventhal]

I don't think we do a goodgreat  handling XUL tooltips right now, but it's not so critical because we use the tooltiptext for the accessible description via GetDescription(). 

Let's create accessible objects for tooltips eventually, but when we do we need to handle all tooltips at the same time. There are DHTML tooltips as well.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

The patch doesn't work since xforms message never become visible (excepting visible inline alert) and accessible objects can't be created for non visible elements. That's a problem.

XForms non ephemeral messages works corresponding following schema:
1) If xforms message have @src attribte then we open new window and load URL specified by @src attribute.
2) If xforms message is bound to instance node then we open new window with document that contains data of instance node.
3) If xforms message is inline then we open new window with that inline content.

XHTML XForms ephemeral messages shows html:div with message content in absolute position.
XUL XForms ephemeral messages shows popup with message content.

Comment 5

[Aaron Leventhal]

Test cases please :)
I want to see all these different types of XForms messages.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: message testcase

(In reply to comment #5)
> Test cases please :)
> I want to see all these different types of XForms messages.
> 

Testcase shows three types of messages: modal, modeless and ephemeral. Aler, help and hint messages use one of these types.

Comment 7

[Aaron Leventhal]

I see now. I was expecting something like DHTML alerts:
http://www.mozilla.org/access/dhtml/alert
Is there anything like that?

The ephemeral messages (tooltips) can be exposed for now as the accessible descriptions.
The modal and modeless work pretty much already, although they should expose ROLE_ALERT instead of ROLE_DIALOG (not sure how it manages to get ROLE_DIALOG, but that's pretty good already). This is probably just a P3 in terms of priority.

Instead of sublcassing nsRootAccessible you would probably just change:
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsRootAccessible.cpp#162
Or, where the dialog is created in XBL, you could put xhtml:role="wairole:alert" at the root element.

Comment 8

[Aaron Leventhal]

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

After talking with Alexander on IRC, I understand that ephemeral msgs aren't tooltips, so I will look again.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: alert testcase

Comment 10

[Aaron Leventhal]

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

Don't use event_alert_high/medium/low because there is no equivalent on ATK, and they're not used by Windows ATs. In fact we shoehorn INVALID and REQUIRED into those states on Windows. In the future we may use this once we construct better APIs, but just remove it for now.

Don't fire any special events for the alerts that have their own window. The toolkits fire all the necessary events in that case.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> 
> Don't fire any special events for the alerts that have their own window. The
> toolkits fire all the necessary events in that case.
> 

But will be that messages readed automatically if they are just windows even if they have role="alert"?

Comment 12

[Aaron Leventhal]

They'll be read automatically if they're windows and have ROLE_ALERT. Those screen readers which don't do that have a bug.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> They'll be read automatically if they're windows and have ROLE_ALERT. Those
> screen readers which don't do that have a bug.
> 

I tried http://www.mozilla.org/access/dhtml/alert, when alert is show then EVENT_ALERT event is not fired. Should EVENT_ALERT be fired when element of alert role become to be visible?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> I tried http://www.mozilla.org/access/dhtml/alert, when alert is show then
> EVENT_ALERT event is not fired. Should EVENT_ALERT be fired when element of
> alert role become to be visible?
> 

The first reason is nsDocAccessible::mIsContentLoaded == PR_FALSE (see http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsDocAccessible.cpp#1250) if page is loaded into browser before than ally code is activated. mIsContentLoaded is changed in two places (nsDocAccessible::nsDocAccessible() http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsDocAccessible.cpp#116 and nsDocAccessible::FireDocLoadEvents http://lxr.mozilla.org/mozilla/source/accessible/src/base/nsDocAccessible.cpp#682). Right now I have not idea how to fix it.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> I tried http://www.mozilla.org/access/dhtml/alert, when alert is show then
> EVENT_ALERT event is not fired. Should EVENT_ALERT be fired when element of
> alert role become to be visible?
> 

The second reason is nsDocAccessible::InvalidateCacheSubtree() isn't called when I show alert by modifying display/visibility styles.

Comment 16

[Aaron Leventhal]

Yeah, do a reload and then try. The mIsContentLoaded gets set after the first load where a11y is initialized. This is not normally a problem since the AT is usually loaded before Firefox comes up, but it should be fixed.

Also, do you have an updated tree with the fix from bug 354745?

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)

> Also, do you have an updated tree with the fix from bug 354745?
> 

Yes, nsIAccessibilityService::InvalidateSubtreeFor() method isn't called does layout code should call it when display/visibility are changed?

Comment 18

[Aaron Leventhal]

> Yes, nsIAccessibilityService::InvalidateSubtreeFor() method isn't called does
> layout code should call it when display/visibility are changed?

Yes, if you search for that method you'll see it's used in presshell, framemanager and cssframeconstructor.

Comment 19

[alexander :surkov (:asurkov)]

Attachment: patch2

The patch uses xhtml2:role approach. I skiped support of ephemeral messages in xul since they don't longer work (bug 357604).

Comment 20

[Aaron Leventhal]

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

The role attribute is not part of the XUL or SVG namespace. So why the change to GetRoleAttribute()?

Don't use xhtml2 anymore for the role attribute, use the xhtml1.x namespace. It's been moved there.

What does the change to loadMessageURL do? Is there more of a core fix required there?

Most of this looks like something that Aaron Reed needs to review. The only mozilla/accessible change is something that looks to me like it should go away.

Comment 21

[alexander :surkov (:asurkov)]

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

(In reply to comment #20)
> (From update of attachment 243118 [details] [diff] [review] [edit])
> The role attribute is not part of the XUL or SVG namespace. So why the change
> to GetRoleAttribute()?

I like to use dhtml roles for custom namespace elements like for XUL. Should XUL have special mechanism to provide role support?

> What does the change to loadMessageURL do? Is there more of a core fix required
> there?

Previously I used xul:browser that was used loadMessageURL, but I failed to use xhtml:role with them therefore I replaced it on xul:iframe.

I didn't change ns though, I will do it after your comments.

Comment 22

[Aaron Leventhal]

> I like to use dhtml roles for custom namespace elements like for XUL. Should
> XUL have special mechanism to provide role support?

This should be enough:


     ...
     <xul:foo 
      xmlns:xhtml=""http://www.w3.org/1999/xhtml"
      xmlns:wairole="http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#"
      xhtml:role="wairole:bar">

Comment 23

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 24

[aaronr]

Attachment: testcase - inline alert

another testcase to try.  Please verify that this works.

Comment 25

[aaronr]

Comment on attachment 243474 [details] [diff] [review]
patch3

I don't have a problem with the code.  But what about hints and inline alerts?  I don't see how they are handled here.  Don't they need some kind of role on their div's, too?  Or is that a followup bug?

Comment 26

[aaronr]

Comment on attachment 243474 [details] [diff] [review]
patch3

removing from my review queue.  Please re-request if you have a new patch or decide to open a new bug (or if I am just wrong and they are handled and I just don't see it :-)

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> (From update of attachment 243474 [details] [diff] [review] [edit])
> I don't have a problem with the code.  But what about hints and inline alerts? 
> I don't see how they are handled here.  Don't they need some kind of role on
> their div's, too?  Or is that a followup bug?
> 

ALERT_EVENT shouldn't be fired for xforms hints since they are mapped to description property of accessible object. But I'm not sure about inline alerts. AaronLev how we should handle inline alerts (please look at AaronR's testcase)?

Comment 28

[Aaron Leventhal]

Inline alert is almost exactly what we want to fire EVENT_ALERT for. It needs to be fired so the alert text is spoken when it is dynamically made visible.

Comment 29

[Aaron Leventhal]

So, the short answer is "Yes. Fire EVENT_ALERT for inline alerts"

Comment 30

[Aaron Leventhal]

BTW, if the inline alert has ROLE_ALERT, the accessibility magic should work to fire EVENT_ALERT automatically.

Comment 31

[alexander :surkov (:asurkov)]

Attachment: patch4

inline alerts supported both for xul and xhtml

Comment 32

[aaronr]

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

>Index: extensions/xforms/resources/content/xforms-xhtml.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms-xhtml.xml,v
>retrieving revision 1.15
>diff -u -8 -p -r1.15 xforms-xhtml.xml
>--- extensions/xforms/resources/content/xforms-xhtml.xml	21 Sep 2006 16:49:13 -0000	1.15
>+++ extensions/xforms/resources/content/xforms-xhtml.xml	29 Oct 2006 00:54:00 -0000
>@@ -49,17 +49,18 @@
> <!ENTITY % xformsDTD SYSTEM "chrome://xforms/locale/xforms.dtd">
>   %xformsDTD;
> ]>
> 
> <bindings id="xformsBindingsForXHTML"
>           xmlns="http://www.mozilla.org/xbl"
>           xmlns:html="http://www.w3.org/1999/xhtml"
>           xmlns:xbl="http://www.mozilla.org/xbl"
>-          xmlns:mozType="http://www.mozilla.org/projects/xforms/2005/type">
>+          xmlns:mozType="http://www.mozilla.org/projects/xforms/2005/type"
>+          xmlns:wairole="http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#">
> 
> 
>   <!-- OUTPUT: <DEFAULT> -->
>   <binding id="xformswidget-output"
>            extends="chrome://xforms/content/xforms.xml#xformswidget-output-base">
>     <content>
>       <children includes="label"/>
>       <!-- XXX initialize span with a space until repeat is xbl-ized.  Part
>@@ -427,33 +428,35 @@
>       </method>
>     </implementation>
>   </binding>
> 
> 
>   <!-- ALERT: <DEFAULT> -->
>   <binding id="xformswidget-alert"
>            extends="chrome://xforms/content/xforms.xml#xformswidget-alert-base">
>-    <content>
>+    <content html:role="wairole:alert">
>       <html:div anonid="container" class="-moz-xforms-message-container">

Are you sure that this will work?  I'd think that you'd want the role to be on the html:div, not on the xbl:content since the div is actually in the anonymous content.


>@@ -462,54 +465,54 @@
>             }
>           };
>         </body>
>       </method>
>     </implementation>
>   </binding>
> 
> 
>-  <!-- MESSAGE: EPHEMERAL, HINT: <DEFAULT>
>-    The widget reuses content of xformswidget-alert.
>-  -->
>-  <binding id="xformswidget-ephemeral-message"
>+    <!-- HINT: <DEFAULT> -->
>+  <binding id="xformswidget-hint"
>            extends="#xformswidget-alert">

nit: you are indenting the comment.  Other places in this file we don't, so we probably shouldn't here.  It should start in the same column as the binding element for which it is a comment.

>-
>-    <implementation implements="nsIXFormsEphemeralMessageUI">
>+     <implementation implements="nsIXFormsEphemeralMessageUI">

nit: you shouldn't add another indent here.  We should keep the indent at 2 spaces all throughout the file.

>Index: extensions/xforms/resources/content/xforms-xul.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms-xul.xml,v
>retrieving revision 1.9
>diff -u -8 -p -r1.9 xforms-xul.xml
>--- extensions/xforms/resources/content/xforms-xul.xml	30 Aug 2006 15:01:43 -0000	1.9
>+++ extensions/xforms/resources/content/xforms-xul.xml	29 Oct 2006 00:54:00 -0000
>@@ -350,17 +350,20 @@
>       </method>
>     </implementation>
>   </binding>
> 
> 
>   <!-- ALERT: <DEFAULT> -->
>   <binding id="xformswidget-alert"
>            extends="chrome://xforms/content/xforms.xml#xformswidget-alert-base">
>-    <content>
>+
>+    <content xmlns:xhtml="http://www.w3.org/1999/xhtml"
>+             xmlns:wairole="http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#"
>+             xhtml:role="wairole:alert">
>       <xul:deck anonid="contentswitcher" flex="1" selectedIndex="1">

same here...is it ok to have the xhtml:role attribute on the xbl:content or would it be better to have it on the xul:deck?

r-'ing just to make sure that the xhtml:role is supposed to go where this patch has it or not.  Otherwise just nits.

Comment 33

[alexander :surkov (:asurkov)]

Attachment: patch5

(In reply to comment #32)

> >+    <content html:role="wairole:alert">
> >       <html:div anonid="container" class="-moz-xforms-message-container">
> 
> Are you sure that this will work?  I'd think that you'd want the role to be on
> the html:div, not on the xbl:content since the div is actually in the anonymous
> content.

> >+    <content xmlns:xhtml="http://www.w3.org/1999/xhtml"
> >+             xmlns:wairole="http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#"
> >+             xhtml:role="wairole:alert">
> >       <xul:deck anonid="contentswitcher" flex="1" selectedIndex="1">
> 
> same here...is it ok to have the xhtml:role attribute on the xbl:content or
> would it be better to have it on the xul:deck?

@role attributes on content is used only for inline alerts. When alert becomes visible then accessibility code will not look inside of element that is visibile. So I should set @role attribute on xforms:alert directly.

I changed only nits :)

Comment 34

[Aaron Leventhal]

But have you tested to ensure it fires EVENT_ALERT?

If we can use XBL to set attributes on a the original non-anonymous element I'd be really surprised.

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #34)
> But have you tested to ensure it fires EVENT_ALERT?

Yes. It fires.

> If we can use XBL to set attributes on a the original non-anonymous element I'd
> be really surprised.
> 

That's XBL can do. Fine feature. Also the feature is used by toolkit too (see for example, http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/menulist.xml#17)

Comment 36

[Aaron Leventhal]

Wow, so will the attribute be present in the normal DOM, or do we have a concept of anonymous attributes on non-anonymous elements?

Anyway, that's great. Really great, because it means we can use the accessibility role and property attributes more easily for creating new widgets.

Comment 37

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

(In reply to comment #36)
> Wow, so will the attribute be present in the normal DOM, or do we have a
> concept of anonymous attributes on non-anonymous elements?
> 

No, we don't have concept of anonymous attributes, but that sounds interesting :)

Comment 38

[alexander :surkov (:asurkov)]

(In reply to comment #36)
> Wow, so will the attribute be present in the normal DOM, or do we have a
> concept of anonymous attributes on non-anonymous elements?

attribute is present in the normal DOM.

> Anyway, that's great. Really great, because it means we can use the
> accessibility role and property attributes more easily for creating new
> widgets.
> 

Agree, very nice feature :)

Comment 39

[alexander :surkov (:asurkov)]

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

So, if aaron had only nits, then I guess I can ask you to review don't wating aaron's one.

Comment 40

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

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

>Index: extensions/xforms/resources/content/xforms-message.xul
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms-message.xul,v
>retrieving revision 1.2
>diff -u -8 -p -r1.2 xforms-message.xul
>--- extensions/xforms/resources/content/xforms-message.xul	19 Sep 2006 07:54:58 -0000	1.2
>+++ extensions/xforms/resources/content/xforms-message.xul	31 Oct 2006 02:30:29 -0000
>@@ -35,27 +35,35 @@
>    - the terms of any one of the MPL, the GPL or the LGPL.
>    -
>    - ***** END LICENSE BLOCK ***** -->
> 
> <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> 
> <dialog id="XFormsMessageDialog"
>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

No need for this.

> 
>-  <browser flex="1" id="browser"/>
>+  <xul:iframe xmlns:xhtml="http://www.w3.org/1999/xhtml"
>+              xmlns:wairole="http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#"
>+              id="frame" flex="1"
>+              style="display: none;"
>+              xhtml:role="wairole:alert"/>


<iframe .... should be enough.

Comment 41

[alexander :surkov (:asurkov)]

Attachment: patch6

Comment 42

[aaronr]

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

Of course all of this assumes that the user didn't have a html:role on the control to begin with.

We need answer from accessibility guys whether we can blindly overwrite html:role like this.

Comment 43

[Aaron Leventhal]

Already checked in by aaronr.