Closed Bug 987067 Opened 6 years ago Closed 6 years ago

On XP, mousehover is not registered over the whole tab-area when using a new profile

Categories

(Firefox :: Theme, defect)

x86
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox28 --- unaffected
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- verified

People

(Reporter: elbart, Assigned: Gijs)

References

Details

(Keywords: regression, Whiteboard: [Australis:P3-])

Attachments

(2 files, 1 obsolete file)

Attached image xp_drag_hack.png
STR:
Under XP, create a new profile and open it. The menubar is enabled by default.
Open another tab, and move the mouse over the now inactive tab.

ER:
The mousehover should be registered over the whole tab-area.

AR:
The mousehover is not being registered over the top 5 pixelrows of the tab-shape.

Regressed by:
bug 887397

Necessary workaround:
Disabling and reenabling the menubar once deactivates the magical tab-shape for good.

According to bug 887397, the magical tab-shape shouldn't be in effect when the menubar is enabled.
Matt:

https://hg.mozilla.org/releases/mozilla-aurora/rev/e72e46d55abc#l2.19

+ ...  #toolbar-menubar:-moz-any([autohide="true"][inactive], :not([autohide])) + #TabsToolbar...

Should that just be only #toolbar-menubar[autohide="true"][inactive] ?
Flags: needinfo?(MattN+bmo)
Whiteboard: [Australis:P3-]
(In reply to :Gijs Kruitbosch from comment #1)
> Matt:
> 
> https://hg.mozilla.org/releases/mozilla-aurora/rev/e72e46d55abc#l2.19
> 
> + ...  #toolbar-menubar:-moz-any([autohide="true"][inactive],
> :not([autohide])) + #TabsToolbar...
> 
> Should that just be only #toolbar-menubar[autohide="true"][inactive] ?

Nope, that would break OS X restored windows. I would consider this a P4 since it's only for XP users who have never toggled the menu bar. The bad assumption was that autohide was always present on Windows.

It may be as easy as setting autohide=false by default for Windows in the XUL but we'd have to see how that interacts with persistence and win6BrowserOverlay.xul.

Otherwise we can fork that one selector inside the ifdef above it and add an else clause.
Assignee: nobody → MattN+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(MattN+bmo)
Whiteboard: [Australis:P3-] → [Australis:P4]
(In reply to Matthew N. [:MattN] from comment #2)
> Nope, that would break OS X restored windows.

Using #toolbar-menubar:not([autohide]) to target OS X makes little sense. Seems like we should just move this out of shared/ and use selectors that make sense on each platform.

> I would consider this a P4
> since it's only for XP users who have never toggled the menu bar.

I don't understand. Windows XP has a massive user base (larger than Linux, OS X, Windows Vista and 8 combined) and I think it's safe to assume that most users will never have toggled the menu bar.
Whiteboard: [Australis:P4] → [Australis:P3-]
This WFM on mac, testing on Windows/Linux now.
Assignee: MattN+bmo → gijskruitbosch+bugs
Of course, Linux doesn't actually need this hunk, as it never draws in the titlebar... Dao, Matt, can either of you review this when you have a moment?
Attachment #8406247 - Flags: review?(dao)
Attachment #8406247 - Flags: review?(MattN+bmo)
Attachment #8406241 - Attachment is obsolete: true
Comment on attachment 8406247 [details] [diff] [review]
use different selector on Windows/Linux than on OS X for background tab clipping for pointer events,

> :not([sizemode="maximized"]):not([inFullscreen])

[sizemode=normal]?
Attachment #8406247 - Flags: review?(dao) → review+
Comment on attachment 8406247 [details] [diff] [review]
use different selector on Windows/Linux than on OS X for background tab clipping for pointer events,

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

::: browser/themes/shared/tabs.inc.css
@@ -233,5 @@
> -   the titlebar. We don't need this in fullscreen since window dragging is not an issue there. */
> -%ifdef XP_MACOSX
> -#main-window[tabsintitlebar][sizemode="maximized"] .tab-background-middle:not([selected=true]),
> -%endif
> -#main-window[tabsintitlebar]:not([sizemode="maximized"]):not([inFullscreen]) #toolbar-menubar:-moz-any([autohide="true"][inactive], :not([autohide])) + #TabsToolbar .tab-background-middle:not([selected=true]) {

I was thinking of fixing this with an ifdef to avoid forking since Linux will get tabsintitlebar eventually. I guess this is fine.
Attachment #8406247 - Flags: review?(MattN+bmo) → review+
(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 8406247 [details] [diff] [review]
> use different selector on Windows/Linux than on OS X for background tab
> clipping for pointer events,
> 
> > :not([sizemode="maximized"]):not([inFullscreen])
> 
> [sizemode=normal]?

Done.

remote:   https://hg.mozilla.org/integration/fx-team/rev/87a8e8c24bf9
Whiteboard: [Australis:P3-] → [Australis:P3-][fixed-in-fx-team]
Comment on attachment 8406247 [details] [diff] [review]
use different selector on Windows/Linux than on OS X for background tab clipping for pointer events,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis / bug 887397
User impact if declined: users on Windows XP might see clipping even if they never got rid of the menubar, ever
Testing completed (on m-c, etc.): local, will be on m-c soon
Risk to taking this patch (and alternatives if risky): low. Small CSS-only patch, two reviews, and basically just a split-by-OS of what used to be a single definition, with a small update specific to Windows to cater for the "XP, never toggled the menubar off" case.
String or IDL/UUID changes made by this patch: none
Attachment #8406247 - Flags: approval-mozilla-beta?
Attachment #8406247 - Flags: approval-mozilla-aurora?
Comment on attachment 8406247 [details] [diff] [review]
use different selector on Windows/Linux than on OS X for background tab clipping for pointer events,

I am sorry but after beta 8, we only accept critical bug fixes...
Attachment #8406247 - Flags: approval-mozilla-beta?
Attachment #8406247 - Flags: approval-mozilla-beta-
Attachment #8406247 - Flags: approval-mozilla-aurora?
Attachment #8406247 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/87a8e8c24bf9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3-][fixed-in-fx-team] → [Australis:P3-]
Target Milestone: --- → Firefox 31
Depends on: 1015122
No longer depends on: 1015122
Flags: in-testsuite?
Keywords: verifyme
Flags: in-testsuite? → in-testsuite-
Flags: in-qa-testsuite?
Reproduced on Nightly 31.0a1 2014-03-24, verified as fixed using Firefox 31 beta 6, Win XP.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Gijs, looking at the bug summary this should only be about XP, but the patch actually touches other OS. Would you mind to clear up the confusion? Also I would like to see a comment on why the in-testsuite request has been denied.
Flags: in-testsuite- → in-testsuite?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Henrik Skupin (:whimboo) from comment #14)
> Gijs, looking at the bug summary this should only be about XP, but the patch
> actually touches other OS. Would you mind to clear up the confusion?

I don't understand what you're confused about. The patch touched other files, but the fix should only affect XP users, because we have different menubar defaults there than we do on other versions of Windows, and because the existing CSS was moved on OS X (but is otherwise identical) and would never have applied on Linux.

> Also I
> would like to see a comment on why the in-testsuite request has been denied.

Because this would require having a test framework which lets you simulate an initial install + profile (which I do not believe we have) - or manually toggling attributes on the toolbar and checking the CSS result would be correct, which would be error-prone, not achieve what we want, and just make updating the CSS a pain) - and would only do a useful test on Windows XP, and because it's such an edgecase that it didn't seem useful to me to invest time to write such a test framework etc. etc.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: in-testsuite?
Flags: in-testsuite-
Ok, so this is XP only then. Given that there is no official support from MS anymore, and our support might also end at some time, I don't think that this is any valuable time my team could spend on creating a Mozmill test.
Flags: in-qa-testsuite? → in-qa-testsuite-
You need to log in before you can comment on or make changes to this bug.