552944 - No relationship between tabs and associated property page in new tabbrowser construct

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This regressed after landing of bug 347930.
Previously, as can seen in the patch to that bug, the pagetab list and tabs were within the same pane as the property pages for each opened tab. Screen readers had a way of finding the associated tab that way.
Now, because they are no longer within the same container, we have to set so-called accessible relations on the tab list and tabs to each individual property page within the browser pane.

1. if xul:tabs has @tabbrowse attribute and it is  set to something, it means that this tablist controls the tabbrowser element and the property pages contained within it.

2. The reverse relation is created on the xul:tabbrowser element using the @tabcontainer attribute. This would translate to "controlled-by" accessible relation.

Then, we need to evangelize to screen reader vendors that if they need to find out the active tab, they have to use the relation to find out where the corresponding tablist is.

Comment 1

[Dão Gottwald [:dao]]

There never was an relationship exposed, AFAIK, since the tabs and the panels always were in different containers that didn't necessarily share the same order.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> There never was an relationship exposed, AFAIK, since the tabs and the panels
> always were in different containers that didn't necessarily share the same
> order.

I think it was. We used hierarchy and linkedPanel to expose accessible relations. So we expected accessible objects for tabs container and tabpanels container are contained by accessible of certain role (see http://mxr.mozilla.org/mozilla-central/source/accessible/src/xul/nsXULTabAccessible.cpp#182). Since tabbrowser was excluded from this hierarchy we've got broken.

Comment 3

[Dão Gottwald [:dao]]

Can't linkedPanel be used regardless of the hierarchy, since it's an id?

Also note that <tabbox> has a 'tabs' property and <tabs> has a 'tabbox' property. If needed, using this might be better than using the <tabbrowser>'s 'tabcontainer' attribute and the <tabs>'s 'tabbrowser' attribute, in terms of being a more generic solution.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Can't linkedPanel be used regardless of the hierarchy, since it's an id?

We do.

> Also note that <tabbox> has a 'tabs' property and <tabs> has a 'tabbox'
> property. If needed, using this might be better than using the <tabbrowser>'s
> 'tabcontainer' attribute and the <tabs>'s 'tabbrowser' attribute, in terms of
> being a more generic solution.

I would avoid to use XBL properties where it's possible since relation calculation might be a bottle neck in means of performance. We used accessible tree since it's quicker. To follow this  we need to get tabs container accessible by tabpanels container accessible and vise versa.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

1) expose interface for XUL:tabs and XUL:tabpanels to get associated tab and tabpanels
2) expose accessible for XUL:tabpanels instead of XUL:tabbox to expose accessible hierarchy in unique way

Comment 6

[Marco Zehe (:MarcoZ)]

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

Can we test the name for the tabpanel (the one it gets from the labelledby relation) in the accTree test as well?

Comment 7

[alexander :surkov (:asurkov)]

try server build - https://build.mozilla.org/tryserver-builds/[e-mail]-try-bfb4d6b7e7a5/

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (From update of attachment 437316 [details] [diff] [review])
> Can we test the name for the tabpanel (the one it gets from the labelledby
> relation) in the accTree test as well?

Technically I agree name and labelledby relation are tightly related but name and relation computation logic doesn't share the same code. Therefore I don't think tabpanel grabs the name from labelledby relation. Do we really need accessible name on tabpanel?

Comment 9

[Marco Zehe (:MarcoZ)]

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

r=me for the tests and the manual testing result of the patch. I take back my earlier question about the accessible name derived from the labelled_by relation. If ATs choose to speak the name from this relation, then it's their choice. I think we should keep things as they are now.

Comment 10

[[e-mail]]

(In reply to comment #3)
> Can't linkedPanel be used regardless of the hierarchy, since it's an id?
Not if it's in anonymous content, although the current code will find it via document.getAnonymousElementByAttribute if it's in the same binding.

Comment 11

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

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

r=me for the /accessible/ part - sorry for the delay (I may look through the whole thing again later)

Comment 12

[Neil Deakin]

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

>diff --git a/dom/interfaces/xul/nsIDOMXULTabsElement.idl b/dom/interfaces/xul/nsIDOMXULTabsElement.idl
>+#include "domstubs.idl"
>+
>+[scriptable, uuid(9fbac05a-fb27-470d-8e5f-028b2dc54ad0)]
>+interface nsIDOMXULTabsElement : nsISupports
>+{
>+  /**
>+   * Retrun tabpanel element associated with the given tab element.
>+   */
>+  nsIDOMNode getLinkedTabpanel(in nsIDOMNode aTabElement);
>+};
>+
>+[scriptable, uuid(59b58db6-f4a8-45b7-b108-aec4949ce247)]
>+interface nsIDOMXULTabpanelsElement : nsISupports
>+{
>+  /**
>+   * Retrun tab element associated with the given tabpanel element.
>+   */
>+  nsIDOMNode getLinkedTab(in nsIDOMNode aTabpanelElement);
>+};

I wonder if it would be better to just have one interface, with a getLinkedElement method.


>+      <method name="getLinkedTabpanel">
...
>+          // otherwise linked tabpanel element has the same index as the given
>+          // tab element.
>+          let tabElmIdx = -1;
>+          for (let tab = aTabElm; tab != null; tab = tab.previousSibling)
>+            tabElmIdx++;

This could be:
  tabElmIdx = aTabElm.control.getIndexOfItem(aTabElm);


>+
>+          let tabpanelElms = tabpanelsElm.childNodes;
>+          for (let idx = 0; idx < tabpanelElms.length; idx++) {
>+            if (idx == tabElmIdx)
>+              return tabpanelElms[idx];

Isn't this just tabpanelsElm.childNodes[tabElmIdx] ?


>+      <method name="getLinkedTab">
...
>+          let tabpanelIdx = -1;
>+          for (let panel = aTabPanelElm; panel != null;
>+               panel = panel.previousSibling)
>+            ++tabpanelIdx;

Also could be written as:

tabpanelIdx = Array.indexOf(aTabPanelElm.parentNode.childNodes, aTabPanelElm)

and done after the retrieval of the tabbox below (to handle unparented tabpanels)

>+
>+          let tabboxElm = this.parentNode;
>+          if (!tabboxElm)
>+            return null;
>+
>+          let tabsElm = tabboxElm.tabs;
>+          if (!tabsElm)
>+            return null;

The <tabpanels> element isn't required to be a direct child of <tabbox> and this will fail if it isn't.
Better would to add a 'tabbox' property, similar to what <tabs> already has.


>+
>+          // Search for tab element of the same index or having 'linkedpanel'
>+          // attribute equalled to the id.

equalled -> equal


>+          let tabElms = tabsElm.childNodes;
>+          let tabElmFromIndex = tabElms[tabpanelIdx];
>+          if (!tabpanelId)
>+            return tabElmFromIndex;

I'd reverse this conditon and put the for loop inside to avoid two similar returns.


>+          for (let idx = 0; idx < tabElms.length; idx++) {
>+            var tabElm = tabElms[idx];
>+            if (tabElm.linkedPanel == tabpanelId)
>+              return tabElm;
>+          }
>+
>+          return tabElmFromIndex;
>+        ]]>
>+        </body>
>+      </method>

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 14

[Neil Deakin]

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


>+      <!-- nsIDOMXULRelatedElement -->
...
>+          let tabpanelIdx = Array.indexOf(this.childNodes,  aTabPanelElm);
>+          if (tabpanelIdx == -1)
>+            return null;
>+
>+          let tabpanelId = aTabPanelElm.getAttribute("id");

Move this line to below where tabpanelId is used.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)

> >+          let tabpanelId = aTabPanelElm.getAttribute("id");
> 
> Move this line to below where tabpanelId is used.

fixed locally

Comment 16

[[e-mail]]

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

>+  /** Used for XUL tabpanels element, a container for tabpanel elements */
Nit: tabpanels don't have to contain "tabpanel" elements. Maybe say "a container for tab panels" ?

> 	nsIDOMXULPopupElement.idl          \
>+  nsIDOMXULRelatedElement.idl \
> 	nsIDOMXULSelectCntrlEl.idl       \
Nit: odd indentation

>+   * and visa versa.
vice versa.

>+            // XXX: if XUL tab element is anonymous element then suppose
>+            // linked XUL tappanel element is hosted within the same XBL
Nit: tabpanel

>+            // binding and search it by ID attribute inside an anonymous content
>+            // of the binding. This is not robust assumption since tab
>+            // elements may live outside a tabbox element so that for example
>+            // tab elements can be explicit content but tabpanel elements.
Nit: incomplete sentence.

(Is there a bug filed on this? It seems to me that the solution would be to use tabpanelsElm instead of aTabElm in the code below. But either way it should work with SeaMonkey's tabbed browser, right?)

>-              var tabpanels = this.tabbox.tabpanels;
>-              // This will cause an onselect event to fire for the tabpanel element.
>+              // This will cause an onselect event to fire for the tabpanel
>+              // element.
>+              let tabpanels = this.tabbox.tabpanels;
(Hardly worth making these changes, but see below.)

>+              tabpanels.selectedPanel = linkedPanel;
Since you're only using tabpanels once now, you could use this.tabbox.tabpanels directly instead of using a temporary.

>+          // Get id and index of the given tabpanel element.
>+          let tabpanelIdx = Array.indexOf(this.childNodes,  aTabPanelElm);
Nit: doubled space. Also, you only use the index right at the end, so I'm not sure why you bother calculating it so early.

>+          let tabpanelId = aTabPanelElm.getAttribute("id");
.id, and again you only need this just before you use it.

>+          // Search for tab element of the same index or having 'linkedpanel'
You don't search for the same index any more.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> (From update of attachment 438951 [details] [diff] [review])
> >+  /** Used for XUL tabpanels element, a container for tabpanel elements */
> Nit: tabpanels don't have to contain "tabpanel" elements. Maybe say "a
> container for tab panels" ?

If tabpanel element shouldn't be a child of tabpanels element then how tabs are going to work, how do tab and tabpanel find each other? Is linkedPanel used here? If so then should new interface be implemented by tabpanel and tab elements rather than tabpanels and tabs elements?

Comment 18

[[e-mail]]

(In reply to comment #17)
> If tabpanel element shouldn't be a child of tabpanels element then how tabs are
> going to work, how do tab and tabpanel find each other?
Language confusion here - any element is allowed as the child of a tabpanels element, so you can call it a "tabpanel" but not a "tabpanel element".

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> (In reply to comment #17)
> > If tabpanel element shouldn't be a child of tabpanels element then how tabs are
> > going to work, how do tab and tabpanel find each other?
> Language confusion here - any element is allowed as the child of a tabpanels
> element, so you can call it a "tabpanel" but not a "tabpanel element".

Oh, I see :). Thanks.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #16)

> >+            // XXX: if XUL tab element is anonymous element then suppose
> >+            // linked XUL tappanel element is hosted within the same XBL
> Nit: tabpanel
> 
> >+            // binding and search it by ID attribute inside an anonymous content
> >+            // of the binding. This is not robust assumption since tab
> >+            // elements may live outside a tabbox element so that for example
> >+            // tab elements can be explicit content but tabpanel elements.
> Nit: incomplete sentence.
> 
> (Is there a bug filed on this? It seems to me that the solution would be to use
> tabpanelsElm instead of aTabElm in the code below. But either way it should
> work with SeaMonkey's tabbed browser, right?)

I'll file a bug. However I don't know seamonkey's tabbed browser implementation details but I think it should work because the bug (where it was decided linkedPanelId can point either to anonid or id) was more theoretical than having real use case.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)

> > (Is there a bug filed on this? It seems to me that the solution would be to use
> > tabpanelsElm instead of aTabElm in the code below.

I filed bug 565858. Could you describe the solution more please?

Comment 23

[[e-mail]]

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

I still don't like the term "tabpanel element", since it's misleading.
Suggested alternative wording:

>+  /** Used for XUL tabpanel elements container */
/** Used for XUL tabpanels container element */

>+ * A container of tabpanel elements.
* A container of tab panels, xul:tabpanels element.

>+   * and used to get XUL tab element by XUL tabpanel element linked with it
* and used to get XUL tab element by linked tab panel

>+          // Get linked tabpanel element by 'linkedpanel' attribute on the given
// Get linked tab panel by 'linkedpanel' attribute on the given

>+          // tab element,
[Should be . not ,]

>+            // linked XUL tabpanel element is hosted within the same XBL
linked tab panel is hosted within the same XBL

>+            // tab elements can be explicit content but tabpanel elements can
tab elements can be explicit content but tab panels can

>+          // of the tabpanel element or the same index with the tabpanel
>+          // element.
// of the tab panel or the same index as the tab panel.

Comment 24

[alexander :surkov (:asurkov)]

landed on 1.9.3 (http://hg.mozilla.org/mozilla-central/rev/897f37baa800) with Neil's comment addressed.