Closed Bug 597218 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: iangilman, Assigned: iangilman)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Bug 597220 is for the Panorama-specific aspect.
hideTab() actually refuses hiding pinned tabs, doesn't it?
Presumably when we fix bug 597220, we'll know how we got into that state.
(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.
Attached patch patch v1 (obsolete) — Splinter Review
This should do it, I think; just needs a test.
Attachment #479543 - Flags: feedback?(dao)
Attachment #479543 - Flags: feedback?(dao) → review+
Blocks: 585689
blocking2.0: --- → final+
blocking2.0: final+ → betaN+
Blocks: 597043
No longer blocks: 585689
Assignee: nobody → ian
Status: NEW → ASSIGNED
Attached patch patch v2 (obsolete) — Splinter Review
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?
Attachment #479543 - Attachment is obsolete: true
Attachment #480976 - Flags: review?(dao)
It may make sense to make 'hidden' read-only, 'pinned' already is.
Attachment #480976 - Flags: review?(dao) → review+
(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?
(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.
Attached patch patch v3Splinter Review
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?
Attachment #480976 - Attachment is obsolete: true
Attachment #481696 - Flags: review?(dao)
Attachment #481696 - Attachment is patch: true
Attachment #481696 - Attachment mime type: application/octet-stream → text/plain
Blocks: 601014
Attachment #481696 - Flags: review?(dao) → review+
(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.
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: dev-doc-needed
Resolution: --- → FIXED
Nope, nothing to do for docs; docs never suggested tabs could be hidden.
Keywords: dev-doc-needed
Depends on: 759711
No longer depends on: 759711
You need to log in before you can comment on or make changes to this bug.