Drawing a tab group should be transparent or in background view

VERIFIED FIXED in Firefox 4.0

Status

Firefox Graveyard
Panorama
P2
enhancement
VERIFIED FIXED
7 years ago
a year ago

People

(Reporter: tchung, Assigned: mitcho)

Tracking

Trunk
Firefox 4.0
Dependency tree / graph
Bug Flags:
in-litmus +

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Since tabcandy supports drawing the tab group size, it would be great if we made the box transparent.   That way if i have a group of individual tabs on tabcandy, and want to draw a rectangle over those tabs, i can still see the tabs im trying to group within the area.

Currently, it overlays with a solid color and covers up the tabs i want to group

Repro:
1) Install Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100819 Minefield/4.0b5pre
2) Create a bunch of individual tabs on tabcandy, and move them fairly close to each other. (but not on top of each other)
3) Now draw a rectangle around the the tabs to create a new group, so the tabs get populated within that group
4) Verify the box opaque, and covers up the tabs behind it.   It would be better if drawing the rectangle is either behind the tabs, or transparent

Expected:
- transparent tab group creation

Actual:
- covers up tabs
I think transparent selection is usually reserved for marquee selection of objects (i.e., folders, files primarily, or a common grouping of similar objects), but not utilized for drag creation.
(Assignee)

Comment 2

7 years ago
I agree with Tony, though... the marquee selection comparison (as Aaron mentions) may actually be appropriate here, though, as the selection actually then scoops up all tabs which it selected over.

I think this is a worthwhile enhancement.
Severity: normal → enhancement
OS: Mac OS X → All
Priority: -- → P4
Hardware: x86 → All

Updated

7 years ago
blocking2.0: --- → ?
Priority: P4 → P2
Target Milestone: --- → Firefox 4.0
(Assignee)

Updated

7 years ago
Assignee: nobody → mitcho
A nice enhancement, indeed, but not a blocker.
blocking2.0: ? → -
(Assignee)

Comment 4

7 years ago
Created attachment 477563 [details]
Screenshot on Mac

This is with the same color, but opacity 0.4. I'm sending it to try now to get a windows build I can run by you as well.
Attachment #477563 - Flags: ui-review?(aza)

Comment 5

7 years ago
Comment on attachment 477563 [details]
Screenshot on Mac

That looks great!

You can probably also change the z-index so that the group appears behind the tabs (which was how it was originally).
Attachment #477563 - Flags: ui-review?(aza) → ui-review+
Blocks: 597043
(Assignee)

Comment 6

7 years ago
Created attachment 482071 [details] [diff] [review]
Patch v2, incorporating Aza's suggestion to ensure that the dragRegions are always below the orphan tabs
Attachment #482071 - Flags: review?(dolske)
(Assignee)

Comment 7

7 years ago
Created attachment 482072 [details]
Updated screenshot
Attachment #477563 - Attachment is obsolete: true
Comment on attachment 482071 [details] [diff] [review]
Patch v2, incorporating Aza's suggestion to ensure that the dragRegions are always below the orphan tabs

I'll assume this looks reasonable when the drag region extends over/under an existing group?
Attachment #482071 - Flags: review?(dolske) → review+
(Assignee)

Comment 9

7 years ago
Comment on attachment 482071 [details] [diff] [review]
Patch v2, incorporating Aza's suggestion to ensure that the dragRegions are always below the orphan tabs

>diff --git a/browser/base/content/tabview/ui.js b/browser/base/content/tabview/ui.js
>--- a/browser/base/content/tabview/ui.js
>+++ b/browser/base/content/tabview/ui.js
>@@ -763,37 +763,36 @@ let UI = {
>     const minSize = 60;
>     const minMinSize = 15;
> 
>     let lastActiveGroupItem = GroupItems.getActiveGroupItem();
>     GroupItems.setActiveGroupItem(null);
> 
>     var startPos = { x: e.clientX, y: e.clientY };
>     var phantom = iQ("<div>")
>-      .addClass("groupItem phantom activeGroupItem")
>+      .addClass("groupItem phantom activeGroupItem dragRegion")
>       .css({
>         position: "absolute",
>-        opacity: .7,
>         zIndex: -1,
>         cursor: "default"
>       })
>       .appendTo("body");
> 
>     var item = { // a faux-Item
>       container: phantom,
>       isAFauxItem: true,
>       bounds: {},
>       getBounds: function FauxItem_getBounds() {
>         return this.container.bounds();
>       },
>       setBounds: function FauxItem_setBounds(bounds) {
>         this.container.css(bounds);
>       },
>       setZ: function FauxItem_setZ(z) {
>-        this.container.css("z-index", z);
>+        // don't set a z-index because we want to force it to be low.
>       },
>       setOpacity: function FauxItem_setOpacity(opacity) {
>         this.container.css("opacity", opacity);
>       },
>       // we don't need to pushAway the phantom item at the end, because
>       // when we create a new GroupItem, it'll do the actual pushAway.
>       pushAway: function () {},
>     };
>@@ -846,16 +845,17 @@ let UI = {
>           phantom.remove();
>         }
>       });
>       GroupItems.setActiveGroupItem(lastActiveGroupItem);
>     }
> 
>     function finalize(e) {
>       iQ(window).unbind("mousemove", updateSize);
>+      item.container.removeClass("dragRegion");
>       dragOutInfo.stop();
>       if (phantom.css("opacity") != 1)
>         collapse();
>       else {
>         var bounds = item.getBounds();
> 
>         // Add all of the orphaned tabs that are contained inside the new groupItem
>         // to that groupItem.
>diff --git a/browser/themes/gnomestripe/browser/tabview/tabview.css b/browser/themes/gnomestripe/browser/tabview/tabview.css
>--- a/browser/themes/gnomestripe/browser/tabview/tabview.css
>+++ b/browser/themes/gnomestripe/browser/tabview/tabview.css
>@@ -163,16 +163,20 @@ body {
>   box-shadow:
>     rgba(0,0,0,0.6) 1px 1px 5.5px;
> }
> 
> .phantom {
>   border: 1px solid rgba(190,190,190,1);
> }
> 
>+.dragRegion {
>+  background: rgba(248,248,248,0.4);
>+}
>+
> .overlay {
>   background-color: rgba(0,0,0,.7) !important;
>   box-shadow: 3px 3px 5.5px rgba(0,0,0,.5);
>   border-radius: 0.4em;
>   /*
>   border: 1px solid rgba(230,230,230,1);
>   background-color: rgba(248,248,248,1);
>   box-shadow:
>diff --git a/browser/themes/pinstripe/browser/tabview/tabview.css b/browser/themes/pinstripe/browser/tabview/tabview.css
>--- a/browser/themes/pinstripe/browser/tabview/tabview.css
>+++ b/browser/themes/pinstripe/browser/tabview/tabview.css
>@@ -170,16 +170,20 @@ body {
>   box-shadow:
>     rgba(0,0,0,0.6) 1px 1px 5.5px;
> }
> 
> .phantom {
>   border: 1px solid rgba(255, 255, 255, 0.5);
> }
> 
>+.dragRegion {
>+  background: rgba(235, 235, 235, 0.4);
>+}
>+
> .overlay {
>   background-color: rgba(0,0,0,.7) !important;
>   box-shadow: 3px 3px 5.5px rgba(0,0,0,.5);
>   border-radius: 0.4em;
>   /*
>   border: 1px solid rgba(230,230,230,1);
>   background-color: rgba(248,248,248,1);
>   box-shadow:
>diff --git a/browser/themes/winstripe/browser/tabview/tabview.css b/browser/themes/winstripe/browser/tabview/tabview.css
>--- a/browser/themes/winstripe/browser/tabview/tabview.css
>+++ b/browser/themes/winstripe/browser/tabview/tabview.css
>@@ -176,16 +176,20 @@ body {
> .groupItem.activeGroupItem {
>   box-shadow:
>     rgba(0,0,0,0.6) 1px 1px 5.5px;
> }
> 
> .phantom {
> }
> 
>+.dragRegion {
>+  background: rgba(224, 234, 245, 0.4);
>+}
>+
> .overlay {
>   background-color: rgba(0,0,0,.7) !important;
>   box-shadow: 3px 3px 5.5px rgba(0,0,0,.5);
>   border-radius: 0.4em;
>   /*
>   border: 1px solid rgba(230,230,230,1);
>   background-color: rgba(248,248,248,1);
>   box-shadow:
Attachment #482071 - Flags: approval2.0?
Comment on attachment 482071 [details] [diff] [review]
Patch v2, incorporating Aza's suggestion to ensure that the dragRegions are always below the orphan tabs

Removing my hasty approval?. Need to check something with Aza first, as pointed out by Dolske...
Attachment #482071 - Flags: approval2.0?
Created attachment 485593 [details]
Screenshot of phantom dragRegion *under* other groups, as well as under orphan tabs

Requesting ui-review from Aza. Aza, this issue was brought up by Dolske in review: what should be the spec for these dragged-out regions, visually, with respect to other groups?
Attachment #485593 - Flags: ui-review?(aza)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED

Comment 12

7 years ago
Comment on attachment 485593 [details]
Screenshot of phantom dragRegion *under* other groups, as well as under orphan tabs

Nice work, Mitcho!
Attachment #485593 - Flags: ui-review?(aza) → ui-review+
(Assignee)

Updated

7 years ago
Attachment #482071 - Flags: approval2.0?
Could we get approval on this? It's been almost a month with approval? .

Comment 14

7 years ago
Isn't that because approval is assigned to you, Mitcho? Shouldn't it be Beltzner?
(In reply to comment #14)
> Isn't that because approval is assigned to you, Mitcho? Shouldn't it be
> Beltzner?

I think that's just the bugzilla notation for my having requested approval.
Attachment #482071 - Flags: approval2.0? → approval2.0+

Comment 16

7 years ago
Moving to b9
Blocks: 598154

Updated

7 years ago
No longer blocks: 597043
Created attachment 497189 [details] [diff] [review]
Patch for checkin
Attachment #482071 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Flags: in-litmus?
Keywords: checkin-needed
This is a visual fix so there's no mochitest for it. I just flagged in-litmus:?. Please let me know how best to request some kind of manual test or verification for this patch once landed.
http://hg.mozilla.org/mozilla-central/rev/7c40cabc296e
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 619937
Verified in recent nightly minefield build
Status: RESOLVED → VERIFIED
test case added: https://litmus.mozilla.org/show_test.cgi?id=15105
Flags: in-litmus? → in-litmus+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.