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

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Elbart]

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

Comment 1

[:Gijs (he/him)]

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] ?

Comment 2

[Matthew N. [:MattN]]

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

Comment 3

[Dão Gottwald [:dao]]

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

Comment 4

[:Gijs (he/him)]

Attachment: use different selector on Windows/Linux than on OS X for background tab clipping for pointer events,

This WFM on mac, testing on Windows/Linux now.

Comment 5

[:Gijs (he/him)]

Attachment: use different selector on Windows/Linux than on OS X for background tab clipping for pointer events,

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?

Comment 6

[Dão Gottwald [:dao]]

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]?

Comment 7

[Matthew N. [:MattN]]

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.

Comment 8

[:Gijs (he/him)]

(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

Comment 9

[:Gijs (he/him)]

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

Comment 10

[Sylvestre Ledru [:Sylvestre]]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87a8e8c24bf9

Comment 12

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/55cae0a17861

Comment 13

[Petruta Horea [:phorea], Desktop QA]

Reproduced on Nightly 31.0a1 2014-03-24, verified as fixed using Firefox 31 beta 6, Win XP.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 15

[:Gijs (he/him)]

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

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.