Don't list app tabs in the all tabs menu

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks: 2 bugs, {ux-minimalism})

Trunk
Firefox 12
ux-minimalism
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
The all tabs menu was originally added along with tab overflow. App tabs aren't part of this and are always easily accessible by design.
(Assignee)

Comment 1

5 years ago
Created attachment 585690 [details] [diff] [review]
patch

noteworthy changes:

* replaced the per-menuitem command event handlers with a single command event handler

* removed the TabOpen handler, since we don't support opening tabs while the popup is open. We also don't handle TabPinned, TabUnpinned, TabMove, TabHide and TabShow.

* removed the aEvent.isTrusted check since we cannot get content events here
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #585690 - Flags: ui-review?(limi)
Attachment #585690 - Flags: review?(mak77)
(Assignee)

Comment 2

5 years ago
Created attachment 585692 [details] [diff] [review]
patch v2

* removed the nasty tab property hack in browser-syncui.js :/
Attachment #585690 - Attachment is obsolete: true
Attachment #585690 - Flags: ui-review?(limi)
Attachment #585690 - Flags: review?(mak77)
Attachment #585692 - Flags: ui-review?(limi)
Attachment #585692 - Flags: review?(mak77)
some questions:
1. can we receive TabClose commands while the popup is open? how does that differ from TabOpen?
2. Rather than relying on external callers to use _removeOnPopupHidden, maybe could be inverted, and add an attribute to static popup content in browser.xul.
3. in _updateTabsVisibilityStatus there is a special check for "tabs from other computers", but since now they don't have anymore a fake .tab property, the first check will already filter them out, so this additional check is useless.
(Assignee)

Comment 4

5 years ago
> 1. can we receive TabClose commands while the popup is open?

It would be hard for a user to actively close a tab, but a page could close itself or an add-on could do strange things.

> how does that differ from TabOpen?

It's easier to support, and the failure case is worse.

> 2. Rather than relying on external callers to use _removeOnPopupHidden,
> maybe could be inverted, and add an attribute to static popup content in
> browser.xul.

I don't really care which way we do this, except that I'd need to find a reasonable attribute name. Suggestions?

> 3. in _updateTabsVisibilityStatus there is a special check for "tabs from
> other computers", but since now they don't have anymore a fake .tab
> property, the first check will already filter them out, so this additional
> check is useless.

Will fix.
(In reply to Dão Gottwald [:dao] from comment #4)
> > how does that differ from TabOpen?
> 
> It's easier to support, and the failure case is worse.

Worse in the sense that we'd have a menuitem pointing to a nonexistent tab? While not adding one nothing bad happens, it is just missing?

> > 2. Rather than relying on external callers to use _removeOnPopupHidden,
> > maybe could be inverted, and add an attribute to static popup content in
> > browser.xul.
> 
> I don't really care which way we do this, except that I'd need to find a
> reasonable attribute name. Suggestions?

Heh, a "static" or "persistent" attribute name sucks. "persistonpopuphidden" is clearer but a bit verbose.
Another alternative to not add sucky named attributes, may be to add a constructor that adds a _static property to existing items.
(Assignee)

Comment 6

5 years ago
(In reply to Marco Bonardo [:mak] from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > > how does that differ from TabOpen?
> > 
> > It's easier to support, and the failure case is worse.
> 
> Worse in the sense that we'd have a menuitem pointing to a nonexistent tab?
> While not adding one nothing bad happens, it is just missing?

Yep.

> > > 2. Rather than relying on external callers to use _removeOnPopupHidden,
> > > maybe could be inverted, and add an attribute to static popup content in
> > > browser.xul.
> > 
> > I don't really care which way we do this, except that I'd need to find a
> > reasonable attribute name. Suggestions?
> 
> Heh, a "static" or "persistent" attribute name sucks. "persistonpopuphidden"
> is clearer but a bit verbose.
> Another alternative to not add sucky named attributes, may be to add a
> constructor that adds a _static property to existing items.

This seems more fragile, as external code would need to be careful about when to inject custom items. At that point, I'd just keep _removeOnPopuphidden.
(In reply to Dão Gottwald [:dao] from comment #6)
> This seems more fragile, as external code would need to be careful about
> when to inject custom items. At that point, I'd just keep
> _removeOnPopuphidden.

The most common behavior is to add items on capture popupshowing, but may be I'm too optimist! So, if we need defense against adding items before the constructor, the only ways are the attribute or _removeOnPopuphidden. I was preferring the attribute because the most common operation is to add content that has to be removed, so having to set the property on each item is more error-prone.
What about "staticcontent"?
(Assignee)

Comment 8

5 years ago
(In reply to Marco Bonardo [:mak] from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > This seems more fragile, as external code would need to be careful about
> > when to inject custom items. At that point, I'd just keep
> > _removeOnPopuphidden.
> 
> The most common behavior is to add items on capture popupshowing, but may be
> I'm too optimist!

We only have one external data point. Realistically, people may want to add items that should or shouldn't be removed at arbitrary points in time.

> So, if we need defense against adding items before the
> constructor, the only ways are the attribute or _removeOnPopuphidden. I was
> preferring the attribute because the most common operation is to add content
> that has to be removed, so having to set the property on each item is more
> error-prone.

It's not hugely more common, unless you count the tab menu items added in the loop as single instances...

> What about "staticcontent"?

Still doesn't sounds specific enough to me. The advantage of using a property is that we can use the underscore to indicate it's something private rather than a general XUL feature.
Comment on attachment 585692 [details] [diff] [review]
patch v2

Review of attachment 585692 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dão Gottwald [:dao] from comment #8)
> The advantage of using a
> property is that we can use the underscore to indicate it's something
> private rather than a general XUL feature.

yep, that's why I thought of the constructor approach adding a property to the less frequent case of static entries.

Ok, please just fix point 3, and eventually add a comment about the fact we support TabClose but not TabOpen to avoid leaving zombie menuitems around.

Eventually we'll evaluate the property change in future, if we get an healthier idea.
Attachment #585692 - Flags: review?(mak77) → review+
(Assignee)

Updated

5 years ago
Attachment #585692 - Flags: ui-review?(limi) → ui-review?(ux-review)
(Assignee)

Updated

5 years ago
Attachment #585692 - Flags: ui-review?(ux-review)
(Assignee)

Updated

5 years ago
Attachment #585692 - Flags: ui-review?(ux-review)
Attachment #585692 - Flags: ui-review?(ux-review) → ui-review+
(Assignee)

Comment 10

5 years ago
I used the first patch, since "Tabs from other computers" is gone.

> 3. in _updateTabsVisibilityStatus there is a special check for "tabs from
> other computers", but since now they don't have anymore a fake .tab
> property, the first check will already filter them out, so this additional
> check is useless.

Fixed.

http://hg.mozilla.org/integration/mozilla-inbound/rev/95a78ca23d9b
Target Milestone: --- → Firefox 12
https://hg.mozilla.org/mozilla-central/rev/95a78ca23d9b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 776669
You need to log in before you can comment on or make changes to this bug.