589206 - Drawing a tab group should be transparent or in background view

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, enhancement, P2)

Description

[Tony Chung [:tchung]]

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

Comment 1

[Aaron Train [:aaronmt]]

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.

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

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.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

A nice enhancement, indeed, but not a blocker.

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: 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.

Comment 5

[Aza Raskin [:aza]]

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).

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2, incorporating Aza's suggestion to ensure that the dragRegions are always below the orphan tabs

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Updated screenshot

Comment 8

[Justin Dolske [:Dolske]]

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?

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

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:

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

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...

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: 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?

Comment 12

[Aza Raskin [:aza]]

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

Nice work, Mitcho!

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

Could we get approval on this? It's been almost a month with approval? .

Comment 14

[Aza Raskin [:aza]]

Isn't that because approval is assigned to you, Mitcho? Shouldn't it be Beltzner?

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

(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.

Comment 16

[Kevin Hanes]

Moving to b9

Comment 17

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

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.

Comment 19

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/7c40cabc296e

Comment 20

[Tracy Walker [:tracy]]

Verified in recent nightly minefield build

Comment 21

[Tracy Walker [:tracy]]

test case added: https://litmus.mozilla.org/show_test.cgi?id=15105