209885 - Support <tab linkedpanel="panelId"/>

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[Alex Vincent [:WeirdAl]]

We should support a panel attribute for the <xul:tab/> element, where the panel
designated an appropriate <xul:tabpanel/> element.  The idea is multiple tabs
should be able to reference a single tabpanel, and use event listeners to apply
style sheets, etc. to that tabpanel.

This RFE started as an e-mail I sent to jag, rginda, and glazman, and has picked
up significant interest in 24 hours.  Hence, this bug.

I truly believe the fix for this will be trivial to implement.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: testcase

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

This patch implements a linkedPanel attribute and corresponding property for
the <xul:tab/> element.

Comment 3

[Alex Vincent [:WeirdAl]]

I decided to call it "linkedPanel" instead of "panel" to avoid confusion with
the <xul:tabpanel/> element.

Comment 4

[Alex Vincent [:WeirdAl]]

jag: timeless wants your sr+/- on this before he gives r+/-.

I'm having second thoughts about something in the patch, the linkedPanel setter.
 Do we really want a setter?  If so, what if the panel doesn't have an id?

Comment 5

[[e-mail]]

Alternatively, have the linked panel be a field, rather than a property.   
Initialize it in the constructor (compare _eventNode).

Comment 6

[jag (Peter Annema)]

Is there any code that goes back from selected panel to selected tab and now
won't know which tab to select?

Comment 7

[Alex Vincent [:WeirdAl]]

jag: I hope not, since that pretty much implies infinite recursion (tab sets
tabpanel, which sets tab, which sets tabpanel...)  More specifically, there is
no binding for <xul:tabpanel/> elements.  I think we're okay on that.

neil: good idea, but I would be more comfortable setting <xul:tab/>'s
_linkedPanel property from the <xul:tabbox/>'s constructor.  Reason:  we cannot
tell whether

<xul:tabbox>
  <xul:tabs>
    <xul:tab/>
  </xul:tabs>
  <xul:tabpanels>
    <xul:tabpanel/>
  </xul:tabpanels>
</xul:tabbox>

or

<xul:tabbox>
 <xul:tabpanels>
    <xul:tabpanel/>
  </xul:tabpanels>
  <xul:tabs>
    <xul:tab/>
  </xul:tabs>
</xul:tabbox>

I've found the order of bindings' constructors firing sometimes matters.  Opinion?

Comment 8

[Alex Vincent [:WeirdAl]]

Comment on attachment 126080 [details]
patch, v1

patch v2 coming up in 15 seconds.

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2

Comment 10

[[e-mail]]

Comment on attachment 126875 [details] [diff] [review]
patch, v2

>+        for (var i = 0; i < this._tabpanels.childNodes.length; i++) {
We like to save the childNodes result in a temporary variable (it's a computed
property).

>+          var panel = this._tabpanels.childNodes.item(i);
What's wrong with [i]?

>+          if (panel.id) {
>+            panelsById[panel.id] = panel;
>+          }
Hmm... what's brace style around here?

>+          if (linkedPanelId) {
>+            if (typeof panelsById[linkedPanelId] == "object") {
You can use if (linkPanelID in panelsById) here.

>+                  if (linkedPanel == null) {
>-                  tabpanels.selectedIndex = val;
>+                    tabpanels.selectedIndex = val;
>+                  } else {
>+                    tabpanels.selectedPanel = linkedPanel;
>+                  }
Reverse the clauses and use if (linkedPanel) instead?

Comment 11

[Alex Vincent [:WeirdAl]]

Re Neil's comment 10:  Okay, I've fixed all the sections you refer to.  caillon
explained to me brace style refers to consistency for braces throughout the
document.  In tabbox.xml, one-line statements belonging to conditional and
looping statements don't use braces, so I've got that straightened out.

Other commentary?  The changes are quite minor; if you want the revised patch I
can post it.

jag: I've confirmed setting a tabpanel as selected does not affect the tabs.

Comment 12

[Alex Vincent [:WeirdAl]]

Comment on attachment 126875 [details] [diff] [review]
patch, v2

reassigning r? based on jag's advice.

Comment 13

[jag (Peter Annema)]

Comment on attachment 126875 [details] [diff] [review]
patch, v2

I think the xul styleguide says all attributes should be completely lowercase,
so "linkedpanel".

This code doesn't deal well with tabs / panels dynamically being added/removed.
If you want to deal with that, I think it'd be okay to do
document.getElementById(tabs[val].getAttribute("selectedpanel")) to get the
panel to show (and if that returns null, do tabpanels.selectedIndex = val)

Comment 14

[[e-mail]]

Not forgetting to check for hasAttribute first, of course :-)

Comment 15

[jag (Peter Annema)]

Neil: of course, null checks are needed in the above suggestion :-)

Comment 16

[jag (Peter Annema)]

Comment on attachment 126875 [details] [diff] [review]
patch, v2

Marking -. ajvincent and I discussed a new approach on irc and he'll come up
with a patch soonish.

Comment 17

[Alex Vincent [:WeirdAl]]

Attachment: revised testcase

Now using lowercase linkedpanel attribute.

Comment 18

[Alex Vincent [:WeirdAl]]

Attachment: patch, v3

answering jag's concerns as voiced on irc.

Comment 19

[[e-mail]]

Comment on attachment 127701 [details] [diff] [review]
patch, v3

>+                  var linkedPanelId = tabs[val].linkedPanel
Dropped a ;
>+                  if (linkedPanelId)
>+                    var linkedPanel = document.getElementById(linkedPanelId);
>+                  else
>+                    linkedPanel = null;
"Badly" scoped var - although JS uses function scope humans use block scope,
easiest fix should be to use ?: operator instead of if/else.

Comment 20

[Alex Vincent [:WeirdAl]]

neil: ok, I'll fix that.  Anything else I should change to get an r+/-?

Comment 21

[[e-mail]]

No, I think that's all, thanks.

Comment 22

[jag (Peter Annema)]

Comment on attachment 127701 [details] [diff] [review]
patch, v3

sr=jag with Neil's comments addressed

Comment 23

[Alex Vincent [:WeirdAl]]

Attachment: final patch

patch conforming to neil r+, jag sr+

Comment 24

[Alex Vincent [:WeirdAl]]

patch checked in by jag.