597218 - It shouldn't be possible for app tabs to be hidden

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Ian Gilman [:iangilman]]

Working on another bug (bug 595943), I discovered that it's possible for xul:tabs to have both .pinned and .hidden true. They're still visible in this state (evidently pinned takes precedence), but it's possible they behave badly in other ways. 

So I guess there's really two issues here: Panorama should never put tabs in that state, and also it probably shouldn't even be possible to do so. This bug is for the latter; I'll file another bug for the former.

Comment 1

[Ian Gilman [:iangilman]]

Bug 597220 is for the Panorama-specific aspect.

Comment 2

[Dão Gottwald [:dao]]

hideTab() actually refuses hiding pinned tabs, doesn't it?

Comment 3

[Ian Gilman [:iangilman]]

Presumably when we fix bug 597220, we'll know how we got into that state.

Comment 4

[Ian Gilman [:iangilman]]

(In reply to comment #2)
> hideTab() actually refuses hiding pinned tabs, doesn't it?

It does, but pinTab doesn't check to see if the tab is hidden. I propose pinning a tab should unhide it.

Comment 5

[Ian Gilman [:iangilman]]

Attachment: patch v1

This should do it, I think; just needs a test.

Comment 6

[Ian Gilman [:iangilman]]

Attachment: patch v2

Now with test. 

Of course people can still set pinned and hidden directly (without calling pinTab and hideTab) and get themselves in this state. Would it make sense to change those properties into getters/setters?

Comment 7

[Dão Gottwald [:dao]]

It may make sense to make 'hidden' read-only, 'pinned' already is.

Comment 8

[Ian Gilman [:iangilman]]

(In reply to comment #7)
> It may make sense to make 'hidden' read-only, 'pinned' already is.

Shall we do that in this bug?

Looks like hidden isn't an official property of tabbrowser-tab. Would making it read-only be just adding: 

      <property name="hidden" readonly="true">
        <getter>
          return this.getAttribute("hidden") == "true";
        </getter>
      </property>

... to tabbrowser-tab's implementation block, or is there more that would need to be done?

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> (In reply to comment #7)
> > It may make sense to make 'hidden' read-only, 'pinned' already is.
> 
> Shall we do that in this bug?

Your choice...

> ... to tabbrowser-tab's implementation block, or is there more that would need
> to be done?

I think that should work.
tabbrowser.xml and nsSessionStore.js currently set the hidden property and would need to stop doing that.

Comment 10

[Ian Gilman [:iangilman]]

Attachment: patch v3

This patch should make it read-only, and fix all the places it was being set directly.

Do we need to update documentation to indicate that it's read-only? If so, how do we flag that?

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> Do we need to update documentation to indicate that it's read-only? If so, how
> do we flag that?

You would add the dev-doc-needed keyword. However, 'hidden' is a general-purpose property which people weren't supposed to use for tabs in the first place, so I'm not sure there's something to update.

Comment 12

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/75b7652a6e2b

Adding dev-doc-needed; the docs folks can determine whether there's anything to be done on their end.

Comment 13

[Eric Shepherd [:sheppy]]

Nope, nothing to do for docs; docs never suggested tabs could be hidden.