Closed Bug 598375 Opened 9 years ago Closed 9 years ago

Tab group displays over the top of the tab search button in panorama view.

Categories

(Firefox Graveyard :: Panorama, defect, P3, minor)

x86
All

Tracking

(blocking2.0 final+)

VERIFIED FIXED
Firefox 4.0b10
Tracking Status
blocking2.0 --- final+

People

(Reporter: jeff, Assigned: iangilman)

References

Details

(Whiteboard: [softblocker])

Attachments

(3 files)

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20100921 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20100921 Firefox/4.0b7pre

When resizing or moving a tab group, the tab group box will display on top of the panorama and search buttons. After the group is dropped, the panorama button will pop on top of the group box.

Reproducible: Always

Steps to Reproduce:
1. Drag a tab group to the top right of the window. (ss1.JPG)
2. Release the tab group. (ss2.JPG)
Actual Results:  
The tab group box should hides the panorama and tab search buttons.

Expected Results:  
The buttons should be always on top.
This is what bug 594950, although that was a specific instance of this problem. The search handle should remain on top just like the Panorama button when you drop a group over that area.
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
OS: Windows XP → All
Assignee: nobody → seanedunn
Priority: -- → P3
blocking2.0: ? → final+
Blocks: 598154
Note that this is related to bug 597269.
Assignee: seanedunn → ian
Attached patch patch v1Splinter Review
Note that this patch also gets us a little closer on bug 597269.
Attachment #499430 - Flags: feedback?(mitcho)
Blocks: 597269
Status: NEW → ASSIGNED
Would this patch also fix bug 594811? Shall we dupe it?
Comment on attachment 499430 [details] [diff] [review]
patch v1

Looks good! The behavior is fixed.

NB: I've only checked this out on Mac. Looks like you did actually put some thought into the design of the buttons for each platform? (I'm saying this based on the crazy box shadow CSS in winstripe's #actions.) Maybe an image with the button treatment for each platform would make it easy for feedback/ui-review from someone like Aza or Shorlander.

Another caveat: I did not check in rtl.

> #searchshade{

Are we putting spaces between the selector and the {? We seem to in some places but not others... hmm...

>-  z-index: 1000;
>+  z-index: 1000001;

Seems to work, but maybe we should create a wiki page with "z-index policy" just so we all know what divs have what z-index when...
Attachment #499430 - Flags: feedback?(mitcho) → feedback+
Duplicate of this bug: 594811
(In reply to comment #7)
> NB: I've only checked this out on Mac. Looks like you did actually put some
> thought into the design of the buttons for each platform? (I'm saying this
> based on the crazy box shadow CSS in winstripe's #actions.) Maybe an image with
> the button treatment for each platform would make it easy for
> feedback/ui-review from someone like Aza or Shorlander.

I'm not really set up to try it on Windows or Linux... I just used the existing styling on the various sheets. I figure we can tweak as needed once it lands. 

> Another caveat: I did not check in rtl.

Thanks for the reminder! I've now tried it, and it looks good. 

> > #searchshade{
> 
> Are we putting spaces between the selector and the {? We seem to in some places
> but not others... hmm...

Yeah, we should have a space so it matches our JavaScript. I'm not going to fix them all with this patch though, just the ones I touched.

> >-  z-index: 1000;
> >+  z-index: 1000001;
> 
> Seems to work, but maybe we should create a wiki page with "z-index policy"
> just so we all know what divs have what z-index when...

Good idea! Done:

https://wiki.mozilla.org/Firefox/Projects/TabCandy/Work#Z-Index_Map
Attachment #499430 - Flags: review?(dietrich)
Comment on attachment 499430 [details] [diff] [review]
patch v1

can you add a test? should be easy enough to load tab view, move the group, and check that the positions don't overlap.
(In reply to comment #10)
> Comment on attachment 499430 [details] [diff] [review]
> patch v1
> 
> can you add a test? should be easy enough to load tab view, move the group, and
> check that the positions don't overlap.

The positions do overlap... the change here is that the buttons are always above the groups. I don't know how I would test that, except to check their z-index, which seems kind of pointless.
Comment on attachment 499430 [details] [diff] [review]
patch v1

That sounds ok then. r=me. However, because nobody has tested this on anything but Mac, please run it on tryserver to make sure there aren't any regressions in the tabcandy suite for those OSes.
Attachment #499430 - Flags: review?(dietrich) → review+
bugspam (moving b9 to b10)
Blocks: 608028
bugspam (removing b9)
No longer blocks: 598154
(In reply to comment #12)
> Comment on attachment 499430 [details] [diff] [review]
> patch v1
> 
> That sounds ok then. r=me. However, because nobody has tested this on anything
> but Mac, please run it on tryserver to make sure there aren't any regressions
> in the tabcandy suite for those OSes.

Pushed: 

ian@iangilman.com – Mon Jan 10 17:45:44 2011 PST (compare: )List changeset URLs

    * b0244fbf0136
      Ian Gilman – try: -m none -u mochitests
    * dc5610b1c2a9
      Ian Gilman – Bug 598375 - Tab group displays over the top of the tab search button in panorama view.
    * 09c1835140c9
      Ian Gilman – Bug 623330 - Intermittent failure in browser/base/cont
The try builds are complete: 

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/ian@iangilman.com-b0244fbf0136/

Looks good to me on Windows. Dietrich, you're on linux right? Perhaps you can check it there? Otherwise, Tim is too.

For that matter, Faaborg, do you want to check it out as well?
having the buttons in a higher z-order is fine.
Whiteboard: [softblocker]
http://hg.mozilla.org/mozilla-central/rev/e62bdeb43768
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110114 Firefox/4.0b10pre

Would an automated test be possible? Otherwise we should have a Litmus test for it.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
Target Milestone: --- → Firefox 4.0b10
Version: unspecified → Trunk
Duplicate of this bug: 626342
(In reply to comment #19)
> Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre)
> Gecko/20110114 Firefox/4.0b10pre
> 
> Would an automated test be possible? Otherwise we should have a Litmus test for
> it.

I don't think we can test this meaningfully with mochitest (unless it's enough to just test z-indexes of the two objects).
(In reply to comment #21)
> > Would an automated test be possible? Otherwise we should have a Litmus test for
> > it.
> 
> I don't think we can test this meaningfully with mochitest (unless it's enough
> to just test z-indexes of the two objects).

Which kind of tests you would think of we need manual tests or Mozmill tests?
(In reply to comment #21)
> I don't think we can test this meaningfully with mochitest (unless it's enough
> to just test z-indexes of the two objects).

I think we could use EventUtils.synthesizeMouseExpectEvent() to fire over the search button and see if the right element gets the event?!
(In reply to comment #23)
> (In reply to comment #21)
> > I don't think we can test this meaningfully with mochitest (unless it's enough
> > to just test z-indexes of the two objects).
> 
> I think we could use EventUtils.synthesizeMouseExpectEvent() to fire over the
> search button and see if the right element gets the event?!

Good idea! I'll remove the manual test flags, and I've filed a new bug for the new test: bug 629257
Flags: in-testsuite?
Flags: in-litmus?
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.