Closed Bug 447580 Opened 16 years ago Closed 15 years ago

New Control+Tab switcher does not fire an accessible event when switching between pages, only initial alert gets fired for ATs

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: MarcoZ, Assigned: dao)

References

Details

(Keywords: access, verified1.9.2)

Attachments

(7 obsolete files)

The below excerpt from a message from Jamie explains this best: When you first press ctrl+tab, an alert event (EVENT_SYSTEM_ALERT) is fired on a grouping. This grouping has two children: a label, which includes the name of the page, and a diagram, which contains no useful information for a screen reader. When an alert event is fired, NVDA automatically speaks all descendent objects. This is why you hear both the label and the diagram. Note that no focus event is fired. Subsequent presses of tab (while still holding control) fire no further events. (I tested this by placing beeps in the callback which receives all winEvents for NVDA.) In short, we need to determine if the widget needs to do anything further, or if our a11y code needs to react to the name change in a certain way. CC'ing all parties involved for comments.
Flags: blocking1.9.1?
The diagram is purely visual and not expected to provide any other information. Only the label is relevant. I expected that the user is notified of the value change automatically.
There are only 2 places where nsIAccessibleEvent::EVENT_NAME_CHANGE is fired: Once if a XUL treeView is invalidated, or if the alt or title attribute of any node are changing. Since the @value attribute of a xul:label element is translated into the accessible name for the label's accessible, we'd need to fire the same event if the value of a xul label changed. We don't want a general EVENT_NAME_CHANGE on the change of any value, because everytime the contents of a textbox changes, for example, that is a change in the @value attribute that would otherwise cause a namechange that is totally unnecessary.
First of all the label doesn't have an accessible in this case, or at least I didn't assign one. Secondly I think the value change of an editable element (e.g. a textbox) is semantically quite different from the value change of a non-editable element (e.g. a label). So unless you think the way I use XUL is invalid, this really is a shortcoming in the a11y code. Even if we add some magic on the frontend to trigger a name change event, that problem would persist for random XUL apps, extensions etc.
The accessible is created for the xul:label automatically by a11y code. Just the name change isn't being picked up. Moving this to "Disability Access APIs". Also there is a second problem. In order for ATs to decide when to speak name changes on labels, it would be good if the panel itself had an accessible name as well. Currently it does not have one. Dão, any idea on that?
Component: Widget → Disability Access APIs
QA Contact: general → accessibility-apis
Attached patch Alternative 1 (obsolete) (deleted) — Splinter Review
This patch does 2 things: 1. It fires an EVENT_NAME_CHANGED if the value is changed for a label. 2. It gives the Tab Switcher panel a fake accessible name. With this, the Tab Switcher is uniquely identifiable, and a name change event could be tracked. However Jamie said tha t a focus event on the label whenever its value changes would be preferable. Aaron added that one could fire a DOMMenuItemActive event on the label, which a11y would translate into a focus event. Will try that next.
(In reply to comment #4) > The accessible is created for the xul:label automatically by a11y code. Just > the name change isn't being picked up. Moving this to "Disability Access APIs". Sorry, I mixed up the terminology. What I meant is that the label element doesn't label any other element, i.e. it doesn't have a control attribute. I don't know if that should matter for what the a11y code does with the label. > Also there is a second problem. In order for ATs to decide when to speak name > changes on labels, it would be good if the panel itself had an accessible name > as well. Currently it does not have one. Dão, any idea on that? What's the situation like with Alt+Tab on Linux and Windows? Are these panels labeled? (In reply to comment #5) > However Jamie said that a focus event on the label whenever its value changes > would be preferable. Aaron added that one could fire a DOMMenuItemActive event > on the label, which a11y would translate into a focus event. Will try that > next. Kind of makes sense since it acts a lot like a menu from which you select an item. However, it's also special treatment for accessibility that shouldn't be necessary. In an ideal world the panel would be accessible as it is now.
(In reply to comment #6) > (In reply to comment #4) > > The accessible is created for the xul:label automatically by a11y code. Just > > the name change isn't being picked up. Moving this to "Disability Access APIs". > Sorry, I mixed up the terminology. What I meant is that the label element > doesn't label any other element, i.e. it doesn't have a control attribute. I > don't know if that should matter for what the a11y code does with the label. No, it doesn't. This only becomes relevant when calculating the accessible name for a different control, such as a textbox. > > Also there is a second problem. In order for ATs to decide when to speak name > > changes on labels, it would be good if the panel itself had an accessible name > > as well. Currently it does not have one. Dão, any idea on that? > What's the situation like with Alt+Tab on Linux and Windows? Are these panels > labeled? They have different controls. They use list items, iirc, or some other control type/sub item of a control that is used to having changing values or selections. A label is usually something rather static that doesn't change very often, so ATs usually don't expect them to have anything special. So, to make them special, we need to do some extra work to get these focus changes. Patch coming up.
Attached patch Patch (obsolete) (deleted) — Splinter Review
Fire a DOMMenuItemActive event on the label once it got its new value. A11y code translates this into a regular focus event if fired on anything other than a real menu item. With this, I get a focus event after each name change for the label which should allow screen readers to pick up the change. Question: This is now in the "scroll" function. There is a second part in the "arrange" function that looks fairly similar. If I put the code in there as well, I get two events. Should the code be in "scroll" or in "arrange"? Is one guaranteed to *always* fire?
Assignee: nobody → marco.zehe
Attachment #330913 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #330949 - Flags: review?(dao)
Component: Disability Access APIs → Tabbed Browser
Flags: blocking1.9.1?
Product: Core → Firefox
Assignee: marco.zehe → nobody
Status: ASSIGNED → NEW
QA Contact: accessibility-apis → tabbed.browser
Status: NEW → ASSIGNED
Assignee: nobody → marco.zehe
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
The one in "arrange" is guaranteed, but happens with a delay if browser.ctrlTab.smoothScroll is enabled (which it is by default). The one in "scroll" is immediate but happens only if smoothScroll is enabled. I can come up with a patch that covers all cases.
Attached patch better patch (obsolete) (deleted) — Splinter Review
Attachment #330949 - Attachment is obsolete: true
Attachment #330984 - Flags: review?(marco.zehe)
Attachment #330949 - Flags: review?(dao)
Comment on attachment 330984 [details] [diff] [review] better patch r=me, thanks!
Attachment #330984 - Flags: review?(marco.zehe) → review+
Attachment #330984 - Flags: review?(gavin.sharp)
(In reply to comment #6) > What's the situation like with Alt+Tab on Linux and Windows? Are these panels > labeled? In Windows, the task list is a list control containing list items for each task on the system. Each time you press tab while holding alt, a focus event is sent for the task list item which is now active. > > However Jamie said that a focus event on the label whenever its value changes > > would be preferable. Actually, I realised this is not quite correct. Because there is only one label for which the value (accessible name) changes, subsequent focus events fired on the same label will have no effect, as most screen readers ignore duplicate focus events. We need a focus event on the label when ctrl+tab is first pressed and then name change events on the label for each subsequent press of tab (each time the name changes). > > Aaron added that one could fire a DOMMenuItemActive event > > on the label, which a11y would translate into a focus event. > Kind of makes sense since it acts a lot like a menu from which you select an > item. However, it's also special treatment for accessibility that shouldn't be > necessary. In an ideal world the panel would be accessible as it is now. In order for the panel to be accessible as it is now (Assuming name change events were being fired for each change of the label) , screen readers would have to speak name change events for *any* object. This is a very bad idea, as this means that any object which changes its name, even one which has no relevance whatsoever to the user's current interest, will have its name spoken. Marco's original idea was to name the switcher panel so that it could be uniquely identified, thus allowing all name changes beneath this panel to be spoken. However, this requires a special case within ATs, and as you point out, special treatment is usually undesirable. (This unique name also causes i18n problems.) In order to avoid speaking irrelevant name changes, NVDA (and probably other ATs) only act upon name changes for the current focus. The reason is that the user is almost certainly interested in name changes for the current focus, as the focus indicates one point of the user's current interest. I would argue that having the switcher label gain focus is quite valid, even outside of accessibility. Focus indicates that the user is interacting with a particular element, which could certainly be said of the switcher while one is engaged in switching tabs. Using the more specific definition of focus (i.e. the widget which currently receives keyboard input) makes this argument somewhat less correct, as the switcher does not process keyboard input as such. Even so, I would argue that it does indirectly handle keyboard input due to its response to the tab key, and no other widget can handle keyboard input while the switcher is engaged. Given these arguments, this is not really special treatment for accessibility.
Attachment #330984 - Flags: review?(gavin.sharp)
Attached patch focus the label (obsolete) (deleted) — Splinter Review
Assignee: marco.zehe → dao
Attachment #330984 - Attachment is obsolete: true
Attachment #331319 - Flags: review?(marco.zehe)
Attached patch Modified version of my Take 1 (obsolete) (deleted) — Splinter Review
Additional patch which makes this feature work with NVDA.
Attachment #331400 - Flags: review?(aaronleventhal)
Comment on attachment 331319 [details] [diff] [review] focus the label That, in combination with the part that introduces a NameChange event to a11y code for label vlaue changes, makes this feature work just nicely with NVDA. JAWS and Window-Eyes should be modifiable for this to work with them as well.
Attachment #331319 - Flags: review?(marco.zehe) → review+
Attached patch Only fire event if the label is also focused (obsolete) (deleted) — Splinter Review
Attachment #331400 - Attachment is obsolete: true
Attachment #331401 - Flags: review?(aaronleventhal)
Attachment #331400 - Flags: review?(aaronleventhal)
I'm proposing bug 448660 to turn off the new Ctrl+tab feature on Windows. However, we still need this for other platforms -- the screen reader flag does not work on those platforms. Gnome won't tell us if an AT is present.
Flags: blocking-firefox3.1?
So, can we get Attachment 331319 [details] [diff] and Attachment 331401 [details] [diff], which both make up the complete fix for this bug, reviewed?
Attachment #331319 - Flags: review?(mano)
Blocks: 395980
Assignee: dao → marco.zehe
Status: ASSIGNED → NEW
Comment on attachment 331401 [details] [diff] [review] Only fire event if the label is also focused ><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">diff --git a/accessible/src/base/nsDocAccessible.cpp b/accessible/src/base/nsDocAccessible.cpp >--- a/accessible/src/base/nsDocAccessible.cpp >+++ b/accessible/src/base/nsDocAccessible.cpp >@@ -1093,8 +1093,12 @@ > return; > } > >+ nsCOMPtr<nsIAtom> tag = aContent->Tag(); > if (aAttribute == nsAccessibilityAtoms::alt || >- aAttribute == nsAccessibilityAtoms::title) { >+ aAttribute == nsAccessibilityAtoms::title || >+ (aAttribute == nsAccessibilityAtoms::value && // see bug 447580 >+ tag == nsAccessibilityAtoms::label && // xul:label's value changed >+ targetNode == gLastFocusedNode)) { // but only if it is also focused What is this all about? You want to fire name change event when tab is switched, right? If so then I believe it's better to handle this inside tabs bindings. Do you? > FireDelayedToolkitEvent(nsIAccessibleEvent::EVENT_NAME_CHANGE, > targetNode); > return;
Alex, the goal of this bug fix, which consists of two parts, is, to speak the active tab's caption when it changes. The panel that does this new visual tab switching looks like this: xul:panel - xul:label - xul:SVB element The SVG element does the animated thumbnail preview, and the label changes its caption with the title of the new tab whenever the thumbnail changes. So what we need is a) make sure the label gets focus, which is what Dao's patch does, and b) fire a name change event on a focused label in addition to the ones we already fire a name change event for. These two patches make NVDA properly speak the new tab switching thingie.
(In reply to comment #20) > So what we need is a) make sure the label gets focus, which is what Dao's patch > does, and b) fire a name change event on a focused label in addition to the > ones we already fire a name change event for. I'm just not sure we want to fire name change event for every focused label element and I'm not happy to add specific code into accessibility without strong reason so I think probably it's better to fire name change event from browser or toolkit code?
I would be happy to do so. If you have an idea how we can fire that event from browser code, let me know how it's done, or attach a patch and I'll try it.
If we want to have this ability for every tabbox then we should fix tabbox.xml I guess.
(In reply to comment #21) > I'm just not sure we want to fire name change event for every focused label > element Labels are usually not focusable. (In reply to comment #23) > If we want to have this ability for every tabbox then we should fix tabbox.xml > I guess. This has nothing to do with tabboxes, see comment 20. We aren't switching tabs here.
(In reply to comment #24) > (In reply to comment #21) > > I'm just not sure we want to fire name change event for every focused label > > element > > Labels are usually not focusable. Sure, and actually that's the main reason I don't like to bring this into accessibility because it's always pain to read the code introduced for special cases and special applications like browser. Since it's not common case then I really would love to fire name change event in browser code or something like. Dao, you has a patch where you focus label element, do you want to fire name change event there? If so we could reintroduce NameChange event I removed in bug 308564.
What's the difference between NameChange and ValueChange?
(In reply to comment #26) > What's the difference between NameChange and ValueChange? > these DOM events are used to fire different accessibility events, screen readers uses them differently too.
So what happens if we dispatch ValueChange?
(In reply to comment #28) > So what happens if we dispatch ValueChange? > that's better to ask Marco but I think screen reader won't announce the change.
Attached patch dispatch ValueChange event (obsolete) (deleted) — Splinter Review
Attachment #333211 - Flags: review?(marco.zehe)
(In reply to comment #29) > (In reply to comment #28) > > So what happens if we dispatch ValueChange? > that's better to ask Marco but I think screen reader won't announce the change. Correct. A label's value attribute or inner text becomes the accessible name for the label's accessible. The name is the primary source of information. So what needs to happen is this: A change in a label's value attribute must be translated into an accessible name change for the label's accessible. AccessibleValue, on the other hand, is not used so often. The only place where it is being used by older MSAA clients is textboxes: The AccessibleName is the textbox's label like "File name:", the AccessibleValue is the textbox's content, for example C:\Test.txt. For label accessibles, an accessible value does not exist. Therefore firing a ValueChange is simply pointless.
Comment on attachment 333211 [details] [diff] [review] dispatch ValueChange event See comment #31.
Attachment #333211 - Flags: review?(marco.zehe) → review-
Should we go back to DOMMenuItemActive?
No, a DOMMenuItemActive event translates into a Focus event. Unless you really want a11y to always fire a focus event to indicate that the name changed. See Comment#5 for a try on that.
I don't understand how comment 5 rules that solution out. You also reviewed attachment 330984 [details] [diff] [review] after that comment.
Well, one thing is the initial focus. Another is to pick up the change in names. A focused element that simply changes names needs either another focus event or a name change event for the screen reader to pick up the name.
I'm not sure what this means for the various patches that we have. Simple question: Would attachment 330984 [details] [diff] [review] without any other patch have fixed this bug?
(In reply to comment #37) > I'm not sure what this means for the various patches that we have. Simple > question: Would attachment 330984 [details] [diff] [review] without any other patch have fixed this bug? No, it would not. It would emit a focus event when the tab switcher first gained focus. But as you hit TAB again and again, the change in the label's caption would not be picked up.
(In reply to comment #36) > A focused element that simply changes names needs either another focus > event or a name change event for the screen reader to pick up the name. It needs a name change event. A focus event will not work for NVDA, because duplicate focus events are simply ignored. I suspect the same is true for other screen readers. Correct me if my understanding is incorrect, but it seems to me that this event translation is an accessibility issue. A XUL label specifies its text in the value, but this gets translated to accessible name. Therefore, shouldn't a value change event get translated to an accessible name change event?
If we need name change event then I would vote to reintroduce DOM "NameChange" event, though probably I would call it "a11yNameChanged" or something like. How does it sound?
Alex, would this be specifically tailored to labels, or could any control fire this even whenever their captioning property changed? For example if the @label attribute of the xul:checkbox element changed, the same event would need to be fired.
Comment on attachment 331319 [details] [diff] [review] focus the label Given bug 450477, I don't think we want to change focus.
Attachment #331319 - Flags: review?(mano)
Then we'll have a big problem on our hands with this one, which would mean unconditionally turning the feature off for all screen reader users or they'll all be screwed. We need a focus event on the label if we want to consider it the active item. And then we need the name changed event on the *focused* label to detect the continuous switching of tabs.
But do we need a real focus event or can we fake it? I'm thinking of stuff like: (In reply to comment #5) > Aaron added that one could fire a DOMMenuItemActive event > on the label, which a11y would translate into a focus event.
Also, would we need focus for the NameChange event that Alexander proposed? A removed comment in attachment 294717 [details] [diff] [review] reads: "capture NameChange events (fired whenever name changes, immediately after, whether focus moves or not)".
A faked focus event would work I believe. Jamie, can you speak to that more? If we fake the focus event strictly through a11y only, that should be enough for NVDA to then consider the item focused, and then watch for nameChange events. Correct?
(In reply to comment #46) > A faked focus event would work I believe. Jamie, can you speak to that more? If > we fake the focus event strictly through a11y only, that should be enough for > NVDA to then consider the item focused, and then watch for nameChange events. Normally, we check for the focused state on the object and ignore a focus event if the focused state is not set. (This is a sanity check; it helps avoid situations where focus events might be coming too fast for us to process. We've also found it eliminates some pointless focus events.) A fake focus event would probably not cause the focused state to be set on the accessible object. However, we have already had to make some exceptions to this focused state check for Mozilla, so one more (i.e. labels) won't hurt and is easy for us to add.
I agree: if we do a focus event the focused state should be set as well. That should make it work with NVDA. That shouldn't be a problem. In fact we probably don't need namechange. Just do a focus event on each individual tab as it gets focused. Fire DOMMenuitemActive on each thumbnail object and just make sure it has a role=something -- you'll have to look at the list of ARIA roles and see if there is something appropriate. If not we can come up with one.
Maybe just role="option" would suffice.
(In reply to comment #48) > In fact we probably don't need namechange. Just do a focus event on each > individual tab as it gets focused. Fire DOMMenuitemActive on each thumbnail > object... As I understand it, there aren't individual objects for each tab. The label/thumbnail simply changes as you move between the tabs. This is inconsistent with the majority of task/window switcher implementations, which present a list of all tasks/documents and move the focus between them. This aside, a name change event is required in the current implementation, as duplicate focus events are simply ignored by most screen readers (see comment 12 and comment 39).
The panel has changed quite a bit. Can we re-evaluate what's missing and what could be done?
Depends on: 463799
Depends on: 465076
Assignee: marco.zehe → dao
Attachment #331319 - Attachment is obsolete: true
Attachment #333211 - Attachment is obsolete: true
Attachment #331401 - Attachment is obsolete: true
Attachment #331401 - Flags: review?(aaronleventhal)
This is quite different indeed. :) First, this is how the tab switcher appears via accessibility APIs: * The parent object is a "grouping", which contains: * A "Search Tabs" editable field. * A label object for each tab. When the tab switcher appears, it fires an "alert" accessible event on the main grouping object, probably because it is a popup. When an alert event is fired, screen readers try to read the content of the alert. Thus, all of the tab label objects are read, which is highly undesirable. It would probably be best to somehow avoid firing this alert event. There is no way to determine which is the currently selected tab via accessibility APIs. If you press control+tab,f, the "Search Tabs" edit field gets focus as it should. One can then press tab to move to the tab selector. This sets focus to the grouping. Here is how I think it should ideally work: * The objects for each tab should have an accessible role of tab instead of label. Hearing "label" for these is semantically incorrect. I suspect this may not be possible, as it would require spoofing the accessible role somehow. (Can ARIA role be used somehow?) This is just a nice-to-have. * An alert event should not be fired on the parent grouping object. * Focus should be set to the object for the selected tab. This applies both to the normal tab switcher and tabfind UIs. This means that a focus event should be fired and the focused state should be set on the object. Again, this can be done with a real focus change or faked for accessibility. I still feel it should be a real focus change, but I believe there were problems with that which currently escape my mind.
(In reply to comment #52) The patch in bug 465076 fixes this.
No longer depends on: 463799
(In reply to comment #53) > The patch in bug 465076 fixes this. I tried this out. Everything is fantastic except that the alert event is still fired on the switcher itself, which causes the whole thing to be read by NVDA. Is there a way we can stop this event from being fired? It's not really an alert, although I understand that it is a popup of sorts which would probably trigger this. Alternatively, we may be able to find a way to handle alerts better in NVDA. Aaron/Marco, any ideas on how to differentiate an alert which should have its content read versus an alert which should not without resorting to nasty hacks? Everything else is great; the items have a role of button (which makes some sense as they aren't really tabs) instead of label and they gain focus as they should. Thanks!
Not blocking 3.1 as we've removed the Ctrl-Tab feature from this release.
Flags: blocking-firefox3.1? → blocking-firefox3.1-
Fixed by bug 465076. James, I don't know the answers to your questions in comment 54, please make sure to file new bugs on them.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Marco, can you verify that this bug is completely fixed for Firefox 3.6?
(In reply to comment #57) > Marco, can you verify that this bug is completely fixed for Firefox 3.6? Yes. This is definitely working with NVDA in Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2b3) Gecko/20091115 Firefox/3.6b3 (.NET CLR 3.5.30729)
Keywords: verified1.9.2
Thanks.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: