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

RESOLVED FIXED

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: iangilman, Assigned: iangilman)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Bug 597220 is for the Panorama-specific aspect.
hideTab() actually refuses hiding pinned tabs, doesn't it?
(Assignee)

Comment 3

8 years ago
Presumably when we fix bug 597220, we'll know how we got into that state.
(Assignee)

Comment 4

8 years ago
(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.
(Assignee)

Comment 5

8 years ago
Created attachment 479543 [details] [diff] [review]
patch v1

This should do it, I think; just needs a test.
Attachment #479543 - Flags: feedback?(dao)

Updated

8 years ago
Attachment #479543 - Flags: feedback?(dao) → review+
(Assignee)

Updated

8 years ago
Blocks: 585689
blocking2.0: --- → final+
blocking2.0: final+ → betaN+
(Assignee)

Updated

8 years ago
Blocks: 597043
No longer blocks: 585689
Assignee: nobody → ian
Status: NEW → ASSIGNED
(Assignee)

Comment 6

8 years ago
Created attachment 480976 [details] [diff] [review]
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?
Attachment #479543 - Attachment is obsolete: true
Attachment #480976 - Flags: review?(dao)
It may make sense to make 'hidden' read-only, 'pinned' already is.

Updated

8 years ago
Attachment #480976 - Flags: review?(dao) → review+
(Assignee)

Comment 8

8 years ago
(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.
(Assignee)

Comment 10

8 years ago
Created attachment 481696 [details] [diff] [review]
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?
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
(Assignee)

Updated

8 years ago
Blocks: 601014

Updated

8 years ago
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.
(Assignee)

Comment 12

8 years ago
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
Last Resolved: 8 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

Updated

6 years ago
Depends on: 759711

Updated

6 years ago
No longer depends on: 759711
You need to log in before you can comment on or make changes to this bug.