1356920 - Refactor tabs in title bar implementation to avoid JS layout calculations

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P4)

Description

[Mike Conley (:mconley) (:⚙️)]

Here's the stack:

rect@chrome://browser/content/browser-tabsintitlebar.js:106:23
_update@chrome://browser/content/browser-tabsintitlebar.js:157:28
updateAppearance@chrome://browser/content/browser-tabsintitlebar.js:65:5
toggle@chrome://browser/content/browser-fullScreenAndPointerLock.js:333:7
handleEvent@chrome://browser/content/browser-fullScreenAndPointerLock.js:349:9

Comment 1

[u554753]

Please correct the component if needed. Thanks!

Comment 2

[Dão Gottwald [:dao]]

If this gets fixed, browser_windowopen_reflows.js should be updated:
http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/browser/base/content/test/general/browser_windowopen_reflows.js#28-36

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Re-queuing for triage by fxperf to reconcile with qf evaluation.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

We're doing this flush during every window open, it seems - including the first. Seems important enough to prioritize higher for now.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Assigning to myself for now - I'm going to do an analysis to see if we can simplify some of this tabs in titlebar stuff, and move more of the layout work into our actual layout engine (as opposed to doing it manually in JS).

Comment 6

[Mike Conley (:mconley) (:⚙️)]

So I've been examining our TabsInTitlebar code, browser.xul and our CSS off and on for the past few days to swap this stuff back into my head and see the state of play.

Basically, it seems like we're doing a lot of work in JS to adjust the size of the titlebar so that it has appropriate dimensions _behind_ the tabs and menu toolbar. There's a lot of edge-cases that it deals with, and some other dimensions that it plays with, but I'm reasonably sure it's primary effect is to keep the titlebar correctly sized and positioned behind the menu and tab strip.

Part of the difficulty, I think, is that the titlebar nodes and the menu / tabs nodes are distant cousins of one another, so they can't directly influence one another's sizes - so we do it manually with JS, and that incurs some layout flushes to avoid flicker.

One way we might be able to make this easier is by moving the tabs and menu bars _inside_ the titlebar node, so that Layout naturally adjusts titlebars size to wrap the menu and tabs. Thankfully, it seems as if the toolbar binding is prepared to be outside of the gNavToolbox (via the optional toolboxid attribute).

So that's the direction I'm starting to investigate - moving the tab strip and the menu bar inside of the titlebar. I'm likely going to need to change how the titlebar node behaves when the native titlebar is being displayed, but I think that shouldn't be so bad.

Hey Dao, can you think of any styling or XUL-y hurdles I might hit going down this path? Is it ill-advised?

Comment 7

[Dão Gottwald [:dao]]

Obvious caveat: we support not having stuff in the title bar, so if you optimize for one mode, you might just add complexity for the other. Instead of just moving the menu and tabs toolbars from the toolbox to the title bar, I'd suggest a hybrid structure to support both modes:

<toolbox>
  <titlebar>
    <menu-toolbar/>
    <tabs-toolbar/>
  </titlebar>
  ...

With tabs in title bar disabled, <titlbar> would just be a container with no actual titlebar styling.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #7)
> I'd suggest a hybrid structure to support both modes:
> 
> <toolbox>
>   <titlebar>
>     <menu-toolbar/>
>     <tabs-toolbar/>
>   </titlebar>
>   ...
> 

Cool, yeah, I'll approach it that way. Thanks!

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Alright, I've been studying this off and on for a bit, and here's my thinking so far:

There are a bunch of variations and cases that we need to accommodate. There's a big matrix of settings that we need to account for. For each OS, I'll list the configurations and cases - just put both on the X and Y dimensions of a matrix, and you'll get a sense of what needs to be supported:

macOS:
* Tabs in titlebar enabled
* Tabs in titlebar disabled
* Fullscreen button available (Lion)
* Fullscreen button not available (Yosemite or later)
* Drag space enabled
* Drag space disabled
* Compact density
* Normal density
* Accessibility indicator displayed
* Private browsing indicator displayed
* Having exited DOM Fullscreen

Windows and Linux:
* Tabs in titlebar enabled
* Tabs in titlebar disabled
* Drag space enabled
* Drag space disabled
* Compact density
* Normal density
* Accessibility indicator displayed
* Private browsing indicator displayed
* Having exited DOM Fullscreen
* Menubar enabled by default
* Menubar disabled by default
* Menubar displayed, but autohidden
* In print preview 
* Outside of print preview

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Whoops, forgot a few:

* Restored window
* Maximized window

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Part of the challenge here are the placeholders that exist in the menubar and the TabsToolbar. browser-tabsintitlebar.js keeps the width of the caption buttons and fullscreen placeholders in sync with the titlebar-buttonbox and titlebar-secondary-buttonbox widths so that when these toolbars overlap, we leave space for the caption buttons.

The "keeping in sync" is one place where we're flushing layout synchronously, and something I think we can move into CSS.

I've got two ideas:

1) Turn the titlebar into a CSS grid, and have the caption buttons / fullscreen button be columns in that grid. The center column would be flexible and where the real content goes. We'll need to use display: contents in order to place the children of the toolbars properly within

2) Use the same platform CSS properties in the placeholders that we use for the titlebar-buttonbox and titlebar-secondary-buttonbox, but then set visibility: hidden on them to avoid painting.


I haven't tested either of these yet, but (1) is pretty intriguing from the expressive power of grids. I'm going to look at that first to see if it can be done simply, and if it gets too unwieldy, I'll start investigating where (2) gets us.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

And a few more configurations:

* RTL
* LTR

Comment 13

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #11)
> Part of the challenge here are the placeholders that exist in the menubar
> and the TabsToolbar. browser-tabsintitlebar.js keeps the width of the
> caption buttons and fullscreen placeholders in sync with the
> titlebar-buttonbox and titlebar-secondary-buttonbox widths so that when
> these toolbars overlap, we leave space for the caption buttons.
> 
> The "keeping in sync" is one place where we're flushing layout
> synchronously, and something I think we can move into CSS.
> 
> I've got two ideas:
> 
> 1) Turn the titlebar into a CSS grid, and have the caption buttons /
> fullscreen button be columns in that grid. The center column would be
> flexible and where the real content goes. We'll need to use display:
> contents in order to place the children of the toolbars properly within

We don't need CSS grid for some fixed-width stuff at the ends and the middle being flexible.

I would advise against mixing -moz-box (that we use everywhere in the browser window) with more modern layout types, because it's not specified how they should interact, and the results can be entirely unexpected. We need bug 1033225 first.

> 2) Use the same platform CSS properties in the placeholders that we use for
> the titlebar-buttonbox and titlebar-secondary-buttonbox, but then set
> visibility: hidden on them to avoid painting.

They don't paint anything, they're just there for getting the width from the OS.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #13)
> They don't paint anything, they're just there for getting the width from the
> OS.

Perhaps paint was too imprecise a word here (at least for what Gecko itself is doing), but they're definitely used to provide the OS the right area to draw certain things into. For example, -moz-window-button-box is used not only to create the rect for drawing the window caption buttons, and then the OS draws the buttons in there. At least macOS seems to get pretty confused when we have more than one instance of -moz-appearance: -moz-window-button-box in our stylesheets.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #14)
> (In reply to Dão Gottwald [::dao] from comment #13)
> > They don't paint anything, they're just there for getting the width from the
> > OS.
> 
> Perhaps paint was too imprecise a word here (at least for what Gecko itself
> is doing), but they're definitely used to provide the OS the right area to
> draw certain things into. For example, -moz-window-button-box is used not
> only to create the rect for drawing the window caption buttons, and then the
> OS draws the buttons in there.

This may be Mac-specific.

On Windows with the Windows compositor, I don't think we can actually control where the OS draws the controls, nor do we want to -- we just want to block the space. Without the Windows compositor and on Linux, the individual title bar buttons have their own -moz-appearance.

Anyway, I don't see why you'd want to hide anything. We want to keep rendering these things as we render them today.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #15)
>
> On Windows with the Windows compositor, I don't think we can actually
> control where the OS draws the controls, nor do we want to -- we just want
> to block the space.

Correct. Thanks for pointing out the difference there, I'd forgotten about the compositor / non-compositor distinction on Windows.

> Without the Windows compositor and on Linux, the
> individual title bar buttons have their own -moz-appearance.

Right - this is very similar to what we do on macOS.

> Anyway, I don't see why you'd want to hide anything. We want to keep
> rendering these things as we render them today.

Oh, absolutely. I'm not suggesting that we change how things looks here. I'm interested in finding ways of keeping the placeholders and the actual buttons widths in sync without sampling in browser-tabsintitlebar.js. What I was suggesting is that the placeholders could also gain the -moz-appearance rules within some new child nodes for the min, max and close buttons to get the right dimensions, and then we visibility: hidden them that they continue to take up the necessary space, but don't paint.

This is why I why I was suggesting grid though - we don't even need the placeholders with grid, since we'd just place the buttonbox in its own column, and grid would take care of creating the gaps in the other rows for us.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

I agree, however, that CSS grid is a likely minefield of uncertainty and undefined behaviour until bug 1033225 is fixed. I'll abandon the CSS grid approach for now and see how I can work the other plan.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

An alternative to the visibility: hidden; hack would be to add some platform support for something like -moz-appearance: -moz-window-buttons-placeholder; which sums up the height and widths of the min, max and close buttons, and returning a rect with that size and padding (and nothing else).

Comment 19

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #16)
> (In reply to Dão Gottwald [::dao] from comment #15)
> >
> > On Windows with the Windows compositor, I don't think we can actually
> > control where the OS draws the controls, nor do we want to -- we just want
> > to block the space.
> 
> Correct. Thanks for pointing out the difference there, I'd forgotten about
> the compositor / non-compositor distinction on Windows.
> 
> > Without the Windows compositor and on Linux, the
> > individual title bar buttons have their own -moz-appearance.
> 
> Right - this is very similar to what we do on macOS.
> 
> > Anyway, I don't see why you'd want to hide anything. We want to keep
> > rendering these things as we render them today.
> 
> Oh, absolutely. I'm not suggesting that we change how things looks here. I'm
> interested in finding ways of keeping the placeholders and the actual
> buttons widths in sync without sampling in browser-tabsintitlebar.js. What I
> was suggesting is that the placeholders could also gain the -moz-appearance
> rules within some new child nodes for the min, max and close buttons to get
> the right dimensions, and then we visibility: hidden them that they continue
> to take up the necessary space, but don't paint.

Getting the caption buttons' width is only one part of the calculations... if you don't get rid of everything, basically nothing is won as we'll still flush and dirty layout...

Instead I think we should abandon the placeholder concept and put the title bar buttons directly there. I thought you were suggesting this in comment 6.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #19)
> Getting the caption buttons' width is only one part of the calculations...
> if you don't get rid of everything, basically nothing is won as we'll still
> flush and dirty layout...

This is true, but it's an incremental step, and part of the final picture, at least in my conception of things.

> Instead I think we should abandon the placeholder concept and put the title
> bar buttons directly there. I thought you were suggesting this in comment 6.

I'm not following. So here's the approach I'm tending towards:

<toolbox>
  <titlebar>
    <titlebar-contents/>
    <menu-toolbar/>
    <tabs-toolbar/>
  </titlebar>
...


Where titlebar-contents actually contains the caption buttons, the fullscreen button (for Lion), and a spacer in the middle.

Because we're not using a grid or table to lay tracks out here, I don't see how we can ensure that the menu and tabs toolbars have the appropriate left and right "spaces" so that overlapping them with the titlebar-contents doesn't show overlapping elements - except by the placeholders.

So I suspect I'm missing part of the puzzle here in how you're seeing things. For example, on Windows, assuming the above DOM relationship, and assuming we'll use the same negative-margin technique to overlap the toolbars appropriately, how do we ensure that the window caption buttons don't overlap the tabs in the tabs toolbar?

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Or is your suggestion that the DOM relationship should be:

<toolbox>
  <titlebar>
    <vbox>
      <menu-toolbar/>
      <tabs-toolbar/>
    </vbox>
    <main-button-box/>
    <secondary-button-box/>
  </titlebar>
...

?

Comment 22

[Dão Gottwald [:dao]]

<toolbox>
  <titlebar>
    <menu-toolbar/>
    <tabs-toolbar>
      ...
      <main-button-box/>
      <secondary-button-box/>
    </tabs-toolbar>
  </titlebar>
  ...

With the menu bar enabled, we'd move the buttons there. Or we'd just have another copy and hide them conditionally.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #22)
> With the menu bar enabled, we'd move the buttons there. Or we'd just have
> another copy and hide them conditionally.

I see. Are you suggesting we do the same thing for things like the private browsing mode and accessibility indicators?

Comment 24

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #23)
> I see. Are you suggesting we do the same thing for things like the private
> browsing mode and accessibility indicators?

Thinking about it, that's either a lot of cloning and keeping things in sync in browser.xul, using a CustomElement for titlebar things, or using JS to either copy or move items between toolbars when the menus open or close.

Are you certain these choices are more affordable than continuing with the placeholder mechanism? Suppose we added a new -moz-appearance: -moz-window-button-box-placeholder and -moz-appearance: -moz-window-button-box-fullscreen-placeholder, to just have the Platform / native theme-ing layer take care of sizing the width of those things... would that not make more sense?

Comment 25

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #24)
> (In reply to Mike Conley (:mconley) (:⚙️) from comment #23)
> > I see. Are you suggesting we do the same thing for things like the private
> > browsing mode and accessibility indicators?
> 
> Thinking about it, that's either a lot of cloning and keeping things in sync
> in browser.xul, using a CustomElement for titlebar things, or using JS to
> either copy or move items between toolbars when the menus open or close.

We're only talking about two containers with a handful of elements. We can just put them twice in browser.xul (could even be shared via #include) or move the two containers with JS -- either way seems fine to me, though I expect the former is a bit simpler if not cheaper.

> Are you certain these choices are more affordable than continuing with the
> placeholder mechanism? Suppose we added a new -moz-appearance:
> -moz-window-button-box-placeholder and -moz-appearance:
> -moz-window-button-box-fullscreen-placeholder, to just have the Platform /
> native theme-ing layer take care of sizing the width of those things...
> would that not make more sense?

Still seems like overkill with no clear benefit. I also still don't see how you'd get rid of the remaining JS layout code.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

I like the idea of using an #include. Let me try that route - thanks for the idea!

Comment 27

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88bb2a03a7632ddebbb4f73f692192e6ad1eb202

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Making some inroads here:

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=0420582ad6581804273fcb710cb8d304e89180b3&newProject=try&newRev=c4a1ad929f0b25ab66f3d78cd790f88915aae500

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Yay - I think I've got something acceptable:

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=0420582ad6581804273fcb710cb8d304e89180b3&newProject=try&newRev=cbc2521b82d981b3a47a40e7b372a7ce3d185407

I'll clean up the patches and have them posted either today or tomorrow.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Remove all toolbar layout code from browser-tabsintitlebar. r?dao

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Move TabsToolbar and toolbar-menubar into the titlebar, and put the titlebar into the navigator-toolbox. r?dao

Depends on D7805

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Adjust CSS rules for new XUL document structure. r?dao

Depends on D7806

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Fix placement of tracking protection icon. r?dao

Depends on D7807

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Get rid of the spacer before the window caption buttons now in the TabsToolbar. r?dao

The spacer isn't necessary, since the tabbrowser-tabs node will flex to fill the available
space anyways.

Depends on D7808

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Move movingtab attribute to navigator-toolbox to account for TabsToolbar not being a sibling of nav-bar anymore. r?dao

Depends on D7809

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Hide the window caption buttons hbox if the native titlebar is being drawn. r?dao

Depends on D7810

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Keep the macOS window caption buttons vertically centered when drag space is enabled. r?dao

Depends on D7811

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Switch window caption XUL nodes from IDs to classes so that more than one can exist per doc. r?dao

Depends on D7812

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Move titlebar items out into a preprocessed file to be included at build-time. r?dao

Depends on D7813

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Put an ordinal rule on the window caption buttons to help ensure they stay at the end of the toolbar. r?dao

Depends on D7814

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Stretch window caption buttons to fill vertical space on Windows 8+ when using extra drag space. r?dao

Depends on D7815

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Update onViewToolbarsPopupShowing to search for toolbars that are grandchildren of the toolbox. r?dao

Depends on D7817

Comment 43

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Put a spacer in the menubar to push titlebar items to the end, and stop the menu from stretching vertically. r?dao

Depends on D7818

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Hide the titlebar items in the TabsToolbar if the menubar is being displayed. r?dao

Depends on D7822

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Fix selector for titlebar. r?dao

Depends on D7825

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Fix titlebar themeing on Linux. r?dao

Depends on D7826

Comment 47

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Remove browser-tabsintitlebar layout flushes from performance test whitelist. r?dao

Depends on D7828

Comment 48

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Fix bottom border of tabbar to account for new DOM structure. r?dao

Depends on D7829

Comment 49

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Fix alignment of window caption buttons for Windows Aero Basic. r?dao

Depends on D7830

Comment 50

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Hide the titlebar-buttonbox-container when in fullscreen mode. r?dao

Depends on D7831

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Adjust the positioning of the titlebar-buttonbox-container and elements below on Windows Aero Basic. r?dao

Depends on D7833

Comment 52

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Set proper vertical alignment of titlebar-buttonbox-container on Windows Aero Basic for restored windows. r?dao

Depends on D7835

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Fix the gap between the tabs and the navbar when in compact mode on Windows 8+. r?dao

Depends on D7836

Comment 54

[Mike Conley (:mconley) (:⚙️)]

One thing I'm a little bummed out about - there doesn't seem to be any significant movement on tpaint / ts_paint in automation on any platform. :( I had seen what looked like ~8% improvement on both my macOS and Windows 10 machines locally, but that's not showing up here:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=83d510c64b2f&newProject=try&newRevision=0e59f024cc19&framework=1&filter=paint

Comment 55

[Dão Gottwald [:dao]]

Attachment: Linux Screenshot

In maximized windows on Linux with the title bar disabled, I get a gap before the first tab that shouldn't be there.

Comment 56

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #55)
> Created attachment 9015799 [details]
> Linux Screenshot
> 
> In maximized windows on Linux with the title bar disabled, I get a gap
> before the first tab that shouldn't be there.

This is related to your question in https://phabricator.services.mozilla.com/D7828 about the lightweight theme, and the margin I was adding.

It seems that with our GTK backend, when the titlebar has -moz-appearance: -moz-window-titlebar;, it gets a margin of about 6 or 7 pixels that I can't override.

When we use a lightweight theme, that -moz-appearance, and therefore that margin goes away.

When you maximize the window, that margin sticks around, and that's where the little gap is coming from.

Hey Martin, do you know where that gap is coming from? I wonder if we should lift it out from the GTK back-end, and add the margin in the CSS skin - that way, I can use it (or override it) as necessary. What do you think?

Comment 57

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #56)
> It seems that with our GTK backend, when the titlebar has -moz-appearance:
> -moz-window-titlebar;, it gets a margin of about 6 or 7 pixels that I can't
> override.
> 
> When we use a lightweight theme, that -moz-appearance, and therefore that
> margin goes away.
> 
> When you maximize the window, that margin sticks around, and that's where
> the little gap is coming from.

We should be using -moz-appearance: -moz-window-titlebar-maximized in that case. Did your patch break this?

Comment 58

[Martin Stránský [:stransky] (ni? me)]

(In reply to Dão Gottwald [::dao] from comment #57)
> (In reply to Mike Conley (:mconley) (:⚙️) from comment #56)
> > It seems that with our GTK backend, when the titlebar has -moz-appearance:
> > -moz-window-titlebar;, it gets a margin of about 6 or 7 pixels that I can't
> > override.
> > 
> > When we use a lightweight theme, that -moz-appearance, and therefore that
> > margin goes away.
> > 
> > When you maximize the window, that margin sticks around, and that's where
> > the little gap is coming from.
> 
> We should be using -moz-appearance: -moz-window-titlebar-maximized in that
> case. Did your patch break this?

Yes, -moz-window-titlebar-maximized is the correct style for maximized window:

https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/browser/themes/linux/browser.css#659

Comment 59

[Martin Stránský [:stransky] (ni? me)]

(In reply to Martin Stránský [:stransky] from comment #58)
> (In reply to Dão Gottwald [::dao] from comment #57)
> > (In reply to Mike Conley (:mconley) (:⚙️) from comment #56)
> > > It seems that with our GTK backend, when the titlebar has -moz-appearance:
> > > -moz-window-titlebar;, it gets a margin of about 6 or 7 pixels that I can't
> > > override.
> > > 
> > > When we use a lightweight theme, that -moz-appearance, and therefore that
> > > margin goes away.
> > > 
> > > When you maximize the window, that margin sticks around, and that's where
> > > the little gap is coming from.
> > 
> > We should be using -moz-appearance: -moz-window-titlebar-maximized in that
> > case. Did your patch break this?
> 
> Yes, -moz-window-titlebar-maximized is the correct style for maximized
> window:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> c291143e24019097d087f9307e59b49facaf90cb/browser/themes/linux/browser.css#659

Mike, does also the -moz-window-titlebar-maximized have the margin?

Comment 60

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #57)
> We should be using -moz-appearance: -moz-window-titlebar-maximized in that
> case. Did your patch break this?

Thankfully, no - I was just being a little imprecise in comment 56.

We're correctly using -moz-window-titlebar-maximized in the maximized case. And as stransky suspected in comment 59, that appearance also has the margin.

Comment 61

[Dão Gottwald [:dao]]

How are we currently dealing with that margin?

Comment 62

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #61)
> How are we currently dealing with that margin?

We aren't. The #titlebar node has it, but the toolbars we're overlapping via browser-tabsintitlebar and negative margin magic don't get impacted - they just overlap those margins.

Now that the nodes are _inside_ the #titlebar, we're hitting this. :/

Comment 63

[Mike Conley (:mconley) (:⚙️)]

Digging into bug 1283299 and friends to see if I can suss out where this margin is coming from...

Comment 64

[Mike Conley (:mconley) (:⚙️)]

The margin is actually being treated as a border, and it's coming from here:

https://searchfox.org/mozilla-central/rev/1ce4e8a5601da8e744ca6eda69e782318afab54d/widget/gtk/nsNativeThemeGTK.cpp#1324-1325

moz_gtk_get_widget_border is returning a 7px left and 7px right border for MOZ_GTK_HEADER_BAR.

Comment 65

[Mike Conley (:mconley) (:⚙️)]

Slight correction - it's both border and padding, which moz_gtk_get_widget_border eventually gets from here:

https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/widget/gtk/gtk3drawing.cpp#2580

Comment 66

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #55)
> In maximized windows on Linux with the title bar disabled, I get a gap
> before the first tab that shouldn't be there.

Looking at this a bit closer, it seems as if it's expected that things in the titlebar, even when maximized, should be indented this much. Looking at some of the default GNOME applications that ship in Ubuntu, for example, I notice that the file explorer, settings window, all draw in the titlebar - and their back/forward buttons are all inset by the 7px margin and border, even when maximized.

So perhaps leaving this gap makes us more consistent on this platform... but I don't feel too strongly about that. What do you think, Martin?

As an alternative, we could just drop the margin and border in the titlebar, and instead put the appropriate margin and border on either side of the window caption buttons. That would also greatly simplify the lightweight theme case (and allow me to revert some of the mysterious hacking I was doing in D7828.

Comment 67

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #66)
> (In reply to Dão Gottwald [::dao] from comment #55)
> > In maximized windows on Linux with the title bar disabled, I get a gap
> > before the first tab that shouldn't be there.
> 
> Looking at this a bit closer, it seems as if it's expected that things in
> the titlebar, even when maximized, should be indented this much. Looking at
> some of the default GNOME applications that ship in Ubuntu, for example, I
> notice that the file explorer, settings window, all draw in the titlebar -
> and their back/forward buttons are all inset by the 7px margin and border,
> even when maximized.
> 
> So perhaps leaving this gap makes us more consistent on this platform... but
> I don't feel too strongly about that. What do you think, Martin?

If we had buttons in that corner, we'd probably want the same space. Our use case is significantly different.

Comment 68

[Martin Stránský [:stransky] (ni? me)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #66)
> (In reply to Dão Gottwald [::dao] from comment #55)
> > In maximized windows on Linux with the title bar disabled, I get a gap
> > before the first tab that shouldn't be there.
> 
> Looking at this a bit closer, it seems as if it's expected that things in
> the titlebar, even when maximized, should be indented this much. Looking at
> some of the default GNOME applications that ship in Ubuntu, for example, I
> notice that the file explorer, settings window, all draw in the titlebar -
> and their back/forward buttons are all inset by the 7px margin and border,
> even when maximized.
> 
> So perhaps leaving this gap makes us more consistent on this platform... but
> I don't feel too strongly about that. What do you think, Martin?
> 
> As an alternative, we could just drop the margin and border in the titlebar,
> and instead put the appropriate margin and border on either side of the
> window caption buttons. That would also greatly simplify the lightweight
> theme case (and allow me to revert some of the mysterious hacking I was
> doing in D7828.

As I wrote at https://bugzilla.mozilla.org/show_bug.cgi?id=1498356#c2 I suggest to remove the border/padding from the -moz-window-titlebar/-moz-window-titlebar-maximized and I don't think we need to follow Gtk+ theme spacing here.

Comment 69

[Martin Stránský [:stransky] (ni? me)]

I'm okay to check-in the patch now if the border/padding issue (Bug 1498356) is the last blocker here. This is a minor theme fix IMHO and can be worked out after the soft freeze, especially when the hidden titlebar is not default on Linux.

Comment 70

[Dão Gottwald [:dao]]

As mconley said this has some implications for other parts of his patches, so it would be good to at least get on the same page even about how the final implementation should look like even if only a followup might get us there. Looking at bug 1498356, I'm not sure we're quite there yet.

Comment 71

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d8b1cf868535a496d100f151547f45fc2fe51c5

Comment 72

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5addf70d3a9a301e2f86a53d958b1e4d934a352

Comment 73

[Mike Conley (:mconley) (:⚙️)]

This thing is definitely not making it to 64, so bumping to 67 for qf tracking.

Comment 74

[Mike Conley (:mconley) (:⚙️)]

So I'm rejigging things a bit here. Some of my later patches slop on some hardcoded pixel values to keep the window caption buttons vertically centered. This was super brittle, and dao was appropriately skeptical of them.

The reason I needed to do this was because of how the #TabsToolbar contained the .titlebar-button-box, and how sometimes the #titlebar (which in my patches now contains the #TabsToolbar) would sometimes be slightly taller than the #TabsToolbar when in compact UI mode when we have the -moz-window-titlebar -moz-appearance applied.

In those cases, in order to avoid the #TabsToolbar from visually separating from the #nav-bar, I opted to -moz-box-pack the #titlebar to put the #TabsToolbar at the bottom, but this also pushed the .titlebar-button-box down. My brittle patches were using negative margins to push them back up.

What I'm planning on doing instead is to wrap the majority of the children of the #TabsToolbar in a new box (basically, everything except the titlebar items), and then have a flexible space at the top of that box to push them down. That way, they're forced to the bottom, but the titlebar buttons can stay affixed to the top.

This appears to work in practice. New patches coming up soon, but I'm going to have to refactor a few things. I'll mark the patches which have changed since receiving Approval in Phabricator.

Comment 75

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d18561a7e9db7259502e02f572814af579feb68

Comment 76

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9435efdd1fb9ebefe9c186b9163352d33cc23d9

Comment 77

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e86c526d47b50ed8b00c19cd749fb430a6de882b

Comment 78

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Reorganize TabsToolbar contents to force most items to the bottom of it. r?dao

Depends on D7814

Comment 79

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Update CSS to account for new DOM structure under TabsToolbar. r?dao

Depends on D9669

Comment 80

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Fix titlebar button rendering when using a lwtheme without the Windows compositor. r?dao

Depends on D9670

Comment 81

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Reorganize TabsToolbar contents to force most items to the bottom of it. r?dao

Depends on D7814

Comment 82

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Update CSS to account for new DOM structure under TabsToolbar. r?dao

Depends on D9872

Comment 83

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1356920 - Fix titlebar button rendering when using a lwtheme without the Windows compositor. r?dao

Depends on D9873

Comment 84

[Mike Conley (:mconley) (:⚙️)]

Garrrrrr moz-phab submit, or something, got confused about my commits again. New ones got created instead of recycling old ones on some of them. *sigh*. I have to rewire these now.

Comment 85

[Mike Conley (:mconley) (:⚙️)]

These commits in Phabricator are a mess. I need to audit them and make sure the SHA's all line up. I'll do that tomorrow morning sometime.

Comment 86

[Mike Conley (:mconley) (:⚙️)]

Any objection if I squash all of these commits together and we just work from a single commit from here forward?

Comment 87

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #86)
> Any objection if I squash all of these commits together and we just work
> from a single commit from here forward?

👍

Comment 88

[Mike Conley (:mconley) (:⚙️)]

Eyyy, I was able to get my Phabricator stuff sorted out. So I'll keep the multi-commits to keep review history intact.

Comment 89

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e354a018b703
Don't calculate and flush layout inside browser-tabsintitlebar.js. r=dao

Comment 90

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/e354a018b703

Comment 91

[Martin Stránský [:stransky] (ni? me)]

Looks like this causes a major regression on Linux - Bug 1505314.

Comment 92

[Dão Gottwald [:dao]]

Mike, your patches still show up under "Waiting on Authors" in phabricator. Can you close / abandon them? Thanks.

Comment 93

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #92)
> Mike, your patches still show up under "Waiting on Authors" in phabricator.
> Can you close / abandon them? Thanks.

Done.