Last Comment Bug 986920 - [Windows Classic] New tab button stealing focus from unselected tabs
: [Windows Classic] New tab button stealing focus from unselected tabs
Status: VERIFIED FIXED
[Australis:P3]
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: 31 Branch
: x86 Windows 7
-- normal (vote)
: Firefox 31
Assigned To: Mike Conley (:mconley)
:
: Dão Gottwald [:dao]
Mentors:
Depends on: 990099 989761
Blocks: australis-cust 978752
  Show dependency treegraph
 
Reported: 2014-03-23 06:01 PDT by Tss
Modified: 2014-04-08 07:48 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified


Attachments
Patch v1 (1.26 KB, patch)
2014-03-25 09:45 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch v2 (1.40 KB, patch)
2014-03-25 11:40 PDT, Mike Conley (:mconley)
gijskruitbosch+bugs: review+
sledru: approval‑mozilla‑aurora+
sledru: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Tss 2014-03-23 06:01:32 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140322030204

Steps to reproduce:

Using Windows Classic theme, when you hover over the rightmost unselected tab (next to the new tab button), new tab button takes focus when the cursor is still over the unselected tab's close button.

Bug 978752 - "Tab strip gradient is broken in customization mode on Windows XP in classic mode" introduced some z-index layering to tabs and new tab button, and I think that's the origin of the problem. I used

main-window[tabsintitlebar]:not([sizemode=fullscreen]) .tabbrowser-tab:not([selected=true]) {
    z-index: 2 !important;
    }

#main-window[tabsintitlebar]:not([sizemode=fullscreen]) .tabbrowser-tab[selected=true] {
    z-index: 3 !important;
    }

to set the tabs on higher layers, and it seems to fix the issue.
Comment 1 User image Mike Conley (:mconley) 2014-03-25 09:45:31 PDT
Created attachment 8396490 [details] [diff] [review]
Patch v1

Naive patch. Should I put some space between these z-index's for add-ons?
Comment 2 User image Mike Conley (:mconley) 2014-03-25 09:47:42 PDT
Comment on attachment 8396490 [details] [diff] [review]
Patch v1

Nope nope nope - this overlaps the selected tab.
Comment 3 User image Mike Conley (:mconley) 2014-03-25 11:40:20 PDT
Created attachment 8396602 [details] [diff] [review]
Patch v2

So, this appears to be better. Layering seems to be right. Kept my eye on things like the separators and the overflow arrow buttons, and didn't see anything hairy.

As before, I wonder if I should space out the indexes a little bit for add-ons to jam things in between. Open to suggestions there.
Comment 4 User image :Gijs 2014-03-26 08:12:28 PDT
Comment on attachment 8396602 [details] [diff] [review]
Patch v2

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

WFM.
Comment 5 User image Mike Conley (:mconley) 2014-03-26 08:49:13 PDT
Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/eefdd02dd206
Comment 6 User image Ryan VanderMeulen [:RyanVM] 2014-03-26 17:57:31 PDT
https://hg.mozilla.org/mozilla-central/rev/eefdd02dd206
Comment 7 User image :Gijs 2014-03-28 06:15:40 PDT
Comment on attachment 8396602 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis! But also, bug 978752
User impact if declined: weird overlap between tabs and new tab button
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): relatively low, just changes z-indices of the tabs
String or IDL/UUID changes made by this patch: none
Comment 8 User image Dão Gottwald [:dao] 2014-03-31 06:58:02 PDT
This code needs some documentation. It's not even obvious why .tabs-newtab-button has z-index:1 in the first place, let alone that this is what made you give .tabbrowser-tab a z-index too.
Comment 9 User image Mike de Boer [:mikedeboer] 2014-03-31 09:47:36 PDT
Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/9fc38ffaff75
Comment 10 User image Mike Conley (:mconley) 2014-03-31 09:49:50 PDT
(In reply to Dão Gottwald [:dao] from comment #8)
> This code needs some documentation. It's not even obvious why
> .tabs-newtab-button has z-index:1 in the first place, let alone that this is
> what made you give .tabbrowser-tab a z-index too.

Sure, I can add that documentation. Would you mind filing a bug and assigning it to me?
Comment 11 User image Mike de Boer [:mikedeboer] 2014-03-31 10:02:40 PDT
Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/e24101b952ad
Comment 12 User image Bogdan Maris, QA [:bogdan_maris] 2014-04-01 06:02:02 PDT
Reproduced the initial issue on nightly build from 2014-03-23, verified fixed on Windows XP 32bit, Windows 7 32bit/64bit using Firefox 29 beta 4, latest Aurora and latest Nightly.
Comment 13 User image mcdavis941 (sporadically reading bugmail) 2014-04-02 07:56:29 PDT
It looks like this should be the patch for beta (rather than e24101b952ad):

https://hg.mozilla.org/releases/mozilla-beta/rev/126e650100c6
Comment 14 User image Mike Conley (:mconley) 2014-04-02 07:58:31 PDT
Yes, that's correct - thanks mcdavis941.
Comment 15 User image Mike Conley (:mconley) 2014-04-04 09:53:58 PDT
Backed out of mozilla-central as https://hg.mozilla.org/integration/fx-team/rev/6a5529e2150d

We landed a better solution in Bug 989761. Back-outs for mozilla-aurora and mozilla-beta will follow once bug 989761 gets uplift approval.
Comment 16 User image Mike Conley (:mconley) 2014-04-04 09:54:28 PDT
Ugh - I should have said backed out of fx-team, not mozilla-central. Of course this will get backed out of mozilla-central after merge.
Comment 17 User image Mike Conley (:mconley) 2014-04-08 07:48:22 PDT
Backouts:

mozilla-central: https://hg.mozilla.org/mozilla-central/rev/6a5529e2150d

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/a9b6b1a08970

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/a90a4219b520


This problem was fixed with a different solution in bug 989761.

Note You need to log in before you can comment on or make changes to this bug.