Closed Bug 1256731 Opened 8 years ago Closed 8 years ago

Investigate overlap between window controls and tabs in mixed-dpi 2+-screen situations

Categories

(Core :: Widget: Win32, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: Gijs, Assigned: jfkthame)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(4 files, 2 obsolete files)

See bug 1249496 comment 28 and attachment 8729423 [details] in particular.
Blocks: 890156
OK, after further testing, I'm convinced this is the correct thing to do; all these window-frame (non-client area) elements are not scaled according to per-monitor dpi by Windows, and therefore we should not scale their metrics here. This fixes the overlap issues. I don't much like the result if you have a single tab open, with the menubar visible, and maximize the window on a lo-dpi secondary monitor: we end up with an awful lot of wasted title-bar space. But that has to be expected, due to the silly Windows behavior of not scaling the buttons properly.
Attachment #8731256 - Flags: review?(VYV03354)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
The patch above fixes the overlap issues; but it also makes another (already-existing) issue more obvious: the TabsInTitlebar code needs to refresh its computations when the window moves between displays of different resolutions, because the fact that Windows *doesn't* scale the non-client area means that the effective size of the titlebar, buttons, etc. changes (in relation to elements like the menus and tab-bar, which we *do* scale for dpi).

A case where this is visible occurs if you move a window (with menubar visible) from a hi-dpi (200%) screen to a lo-dpi (100%) one. The menubar does not "stick" to the top of the titlebar, as intended; it appears slightly lower (and with the patch above, this becomes more obvious because the titlebar is expanded by the large buttons). Hiding and then re-showing the menubar (or maximizing and restoring the window) corrects its position, as does running TabsInTitlebar.updateAppearance(true) from the browser console.

So we need to trigger a TabsInTitlebar.updateAppearance automatically when such a resolution change happens.
Here's a simple patch that triggers the updateAppearance we need. No idea if this is the most appropriate mechanism to use in order to notify the front-end code like this; it's juts the first/simplest I could find.
Attachment #8731258 - Flags: review?(gijskruitbosch+bugs)
Attachment #8731256 - Flags: review?(VYV03354) → review+
With these two patches, the titlebar & associated controls are behaving much better. There's one remaining cosmetic issue I'm seeing: when the primary display is hi-dpi, and the window is maximized on a lo-dpi display (hence it has the "extra-large" titlebar buttons, etc), and the menubar is hidden, we get some overlap of the window controls into the browser toolbar area, because the titlebar buttons are taller than the height allowed for the tab strip. (See attached screenshot.)

I suspect this is probably a front-end issue, with the height of the button box being ignored when calculating the height for the tab strip. Gijs, WDYT?
Comment on attachment 8731258 [details] [diff] [review]
patch 2 - Notify front-end code when screen resolution changes on Windows, so that TabsInTitlebar code can refresh its computations

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

Does this change fire only for changes to the DPI scaling *after* the initial construction (in some DPI scaling X) of the window? Because otherwise this is likely to impact startup time/perf unless we make it ignore the initial notification in some way.

Otherwise, this is is pretty close, but I would expect the observer notification to pass the window that's changed DPI scaling. AIUI not all windows would necessarily use the same DPI scaling, so every notification is only relevant to the window that gets it.

Two ways to solve this. One would be passing the window (would need to be nsIFoo rather than nsWindow pointer) as the subject of the notification. This is nice for people who are interested when any window changes resolution because they can observe the notification and get told about any/every window - but there are currently 0 such consumers. :-) Instead, every toplevel chrome window would register an observer and we'd check that the notification applies to us before proceeding, which is slightly wasteful, but most people won't have oodles and oodles of windows - though it seems more plausible to have multiple windows if you have multiple screens to begin with...

It would be slightly more efficient if nsWindow fired a custom event instead. I don't know how easy that is though - all the places I could easily find that do that use DOM, and I don't know how straightforward it is to do that from widget code.

Either way, we should avoid every window running its calculation code whenever some other window changes DPI scaling.
Attachment #8731258 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #5)
> Comment on attachment 8731258 [details] [diff] [review]
> patch 2 - Notify front-end code when screen resolution changes on Windows,
> so that TabsInTitlebar code can refresh its computations
> 
> Review of attachment 8731258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does this change fire only for changes to the DPI scaling *after* the
> initial construction (in some DPI scaling X) of the window? Because
> otherwise this is likely to impact startup time/perf unless we make it
> ignore the initial notification in some way.

Yes, this is a response to a change; it doesn't happen on initial window construction. (And on a system without multiple, mixed-dpi screens, it simply never happens.)

(In practice, it will sometimes be fired during the process of opening a new window, because the toolkit code may construct the window at an initial default location, and then move it to its intended position; and that may shift it from one screen to another. But that's part of a more general inefficiency in how we do things. I don't know whether it's important enough to be worth trying to optimize, but even if so, I'd regard that as a separate bug.)

> Otherwise, this is is pretty close, but I would expect the observer
> notification to pass the window that's changed DPI scaling. AIUI not all
> windows would necessarily use the same DPI scaling, so every notification is
> only relevant to the window that gets it.

True; moving one window between screens shouldn't have any effect on other windows. So this would trigger a redundant recalculation for them.

> Two ways to solve this. One would be passing the window (would need to be
> nsIFoo rather than nsWindow pointer) as the subject of the notification.
> This is nice for people who are interested when any window changes
> resolution because they can observe the notification and get told about
> any/every window - but there are currently 0 such consumers. :-) Instead,
> every toplevel chrome window would register an observer and we'd check that
> the notification applies to us before proceeding, which is slightly
> wasteful, but most people won't have oodles and oodles of windows - though
> it seems more plausible to have multiple windows if you have multiple
> screens to begin with...

Though it also seems likely that people with multiple screens will usually have powerful enough processors that a few extra cycles here will be negligible -- especially in the light of all the rest of the work that needs to happen when a window moves between screens with different resolutions.

> 
> It would be slightly more efficient if nsWindow fired a custom event
> instead. I don't know how easy that is though - all the places I could
> easily find that do that use DOM, and I don't know how straightforward it is
> to do that from widget code.
> 
> Either way, we should avoid every window running its calculation code
> whenever some other window changes DPI scaling.

I started out looking at custom events, actually, but it wasn't clear to me how easy that was going to be, whereas the observer notification looked trivial to implement -- there was already a handy live-resize-start/end notification to cargo-cult from. :)

I'll try to update this one way or the other...
OK, here's a version that uses a custom event to notify the tabbrowser that it should refresh the tabsInTitlebar computations. Seems to work as expected in my testing. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75b50288955a
Attachment #8731463 - Flags: review?(gijskruitbosch+bugs)
Attachment #8731258 - Attachment is obsolete: true
Comment on attachment 8731463 [details] [diff] [review]
patch 2 - Notify front-end code when screen resolution changes on Windows, so that TabsInTitlebar code can refresh its computations

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

This looks fine with the listener moved to browser-tabsintitlebar.js .

I'm not a widget peer, and while the event firing looks fine to me, after the fiasco of https://groups.google.com/forum/?fromgroups=&hl=en#!topic/firefox-dev/CYG6wgvOmW8 I think it's probably wise to get that bit reviewed by :emk as well.

::: browser/base/content/tabbrowser.xml
@@ +4123,5 @@
>                  this.mCurrentBrowser.setDocShellIsActiveAndForeground(
>                    window.windowState != window.STATE_MINIMIZED);
>                }
>                break;
> +            case "resolutionchange":

Sorry, I should have realized this sooner, but can you stick the listener directly in browser-tabsintitlebar.js ? Add a handleEvent method to the TabsInTitlebar object, and just addEventListener("resolutionchange", this, false); in init() and the corresponding removeEventListener("resolutionchange", this); in uninit();
Attachment #8731463 - Flags: review?(gijskruitbosch+bugs)
Attachment #8731463 - Flags: review?(VYV03354)
Attachment #8731463 - Flags: review+
Moved the event listener to tabsintitlebar.js, as suggested (thanks); seems to work as expected. Carrying over r=gijs for the front-end side of this, flagging :emk for review on the C++ side.
Attachment #8731675 - Flags: review?(VYV03354)
Attachment #8731463 - Attachment is obsolete: true
Attachment #8731463 - Flags: review?(VYV03354)
There's one remaining piece of the window-controls-overlap issue, seen when the primary display is hi-dpi (200%) and the secondary is lo-dpi (100%), and the window is maximized on the secondary display, with titlebar and menu hidden. In this case, because the window controls are taller than the height of the tabs, we get an ugly overlap as shown in attachment 8731260 [details].

AFAICT, this happens because of the negative margin-bottom applied to titlebar. I have a patch that addresses this and seems to give me a good result in all the variations I've tested, though I'm not sure I really understand all the interactions between the things being calculated here...
Comment on attachment 8731740 [details] [diff] [review]
patch 3 - Don't pull the elements below the titlebar up so far they'll clash with the buttons, in the secondary lo-dpi case where the window controls are oversized

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

Per discussion on IRC, r=me, but can you file a followup bug to fix the resulting Fitts' law breakage and CC :phlsa, :jaws, :MattN, :mconley, :dolske and me, please? :-)

::: browser/base/content/browser-tabsintitlebar.js
@@ +210,5 @@
>          titlebarContent.style.removeProperty("margin-bottom");
>        }
>  
>        // Then we bring up the titlebar by the same amount, but we add any negative margin:
> +      let minTitlebarOrTabsHeight = Math.min(titlebarContentHeight, tabAndMenuHeight); 

Nit: this has trailing whitespace.

Can you also update the comment above this line to um, reflect pre-existing reality? Sorry about the cruft!
Attachment #8731740 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8731675 [details] [diff] [review]
patch 2 - Notify front-end code when screen resolution changes on Windows, so that TabsInTitlebar code can refresh its computations

r=me about the widget part.

By the way, Edge changes the system control size on DPI change somehow. Is it only available from Universal apps?
Attachment #8731675 - Flags: review?(VYV03354) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/55c8414f6269149453a9141b4ea11e01e1d1bb37
Bug 1256731 - patch 1 - Don't apply theme-dpi scaling to metrics of window border elements, because Windows doesn't respect per-monitor dpi scaling when it draws them. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc1615263913cb0febe6e729d7851b5109f7a8f
Bug 1256731 - patch 2 - Notify front-end code when screen resolution changes on Windows, so that TabsInTitlebar code can refresh its computations. r=gijs,emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/f11c6068824f361d00e60e19518d780aa4c514cf
Bug 1256731 - patch 3 - Don't pull the elements below the titlebar up so far they'll clash with the buttons, in the secondary lo-dpi case where the window controls are oversized. r=gijs
Comment on attachment 8731256 [details] [diff] [review]
patch 1 - Don't apply theme-dpi scaling to metrics of window border elements, because Windows doesn't respect per-monitor dpi scaling when it draws them

Approval Request Comment
[Feature/regressing bug #]: 890156 (per-monitor dpi support)

[User impact if declined]: visual glitches on secondary display in some mixed-dpi setups (excess borders or clipped tabs/menus, window buttons overlapping into toolbar when window is maximized) 

[Describe test coverage new/current, TreeHerder]: manually tested (requires multi-display, mixed-dpi configurations)

[Risks and why]: low risk, fixes to widget and front-end metrics that have no impact except on multi-display/mixed-dpi setups

[String/UUID change made/needed]: none


(Uplift request applies to all three patches here, as a unit; although written in three stages, they're all part of the same issue and work together to deal with Windows' bizarre decision NOT to scale window borders/controls according to the current screen's resolution.)
Attachment #8731256 - Flags: approval-mozilla-aurora?
Comment on attachment 8731675 [details] [diff] [review]
patch 2 - Notify front-end code when screen resolution changes on Windows, so that TabsInTitlebar code can refresh its computations

Approval Request Comment
(see above)
Attachment #8731675 - Flags: approval-mozilla-aurora?
Comment on attachment 8731740 [details] [diff] [review]
patch 3 - Don't pull the elements below the titlebar up so far they'll clash with the buttons, in the secondary lo-dpi case where the window controls are oversized

Approval Request Comment
(see above)
Attachment #8731740 - Flags: approval-mozilla-aurora?
Depends on: 1257792
Comment on attachment 8731256 [details] [diff] [review]
patch 1 - Don't apply theme-dpi scaling to metrics of window border elements, because Windows doesn't respect per-monitor dpi scaling when it draws them

Taking this in Aurora47 as we are trying to improve Firefox multi-mon experience especially for high-dpi monitors.
Attachment #8731256 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8731675 [details] [diff] [review]
patch 2 - Notify front-end code when screen resolution changes on Windows, so that TabsInTitlebar code can refresh its computations

Aurora47+
Attachment #8731675 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8731740 [details] [diff] [review]
patch 3 - Don't pull the elements below the titlebar up so far they'll clash with the buttons, in the secondary lo-dpi case where the window controls are oversized

Aurora47+
Attachment #8731740 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Decreasing tab height using #TabsToolbar, #tabbrowser-tabs { min-height: 22px !important;}

Works until maximize, then a weird gap appears above the tabs.
(In reply to Sergio from comment #23)
> Decreasing tab height using #TabsToolbar, #tabbrowser-tabs { min-height:
> 22px !important;}
> 
> Works until maximize, then a weird gap appears above the tabs.

Sample code:

@namespace xul url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul);

:root {
    --space-above-tabbar: 0 !important;
}
#TabsToolbar, #tabbrowser-tabs {
  height: 24px !important; min-height: 24px !important;
  background: grey !important;
}
(In reply to Sergio from comment #23)
> Decreasing tab height using #TabsToolbar, #tabbrowser-tabs { min-height:
> 22px !important;}
> 
> Works until maximize, then a weird gap appears above the tabs.

Could you provide a screenshot showing what you mean here? (Unless this is directly related to the original issue here, it might be better to file a new bug.)
quite sure it's caused by this set of patches, wasn't happening before, I've filed it as a new bug here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1268934

There's a webm showing the problem there.
I'll inform about this to the people that had rolled back to previous version cause this, and provide them a workaround to remove the "visit url" from the bar:

If someone need it use this code (userchrome or stylish):

@namespace url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul);

#PopupAutoCompleteRichResult > richlistbox > richlistitem[actiontype="visiturl"] {
  display: none !important;
}
... sorry comment 27 was for this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1269093

I've replied on the wrong window :S
Depends on: 1267636
Depends on: 1302168
Blocks: 1457186
Blocks: 1457189
Blocks: 1457436
Blocks: 1457437
You need to log in before you can comment on or make changes to this bug.