Refactor tabs in title bar implementation to avoid JS layout calculations

RESOLVED FIXED in Firefox 65

Status

()

P4
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Depends on: 2 bugs, Blocks: 4 bugs, {perf})

unspecified
Firefox 65
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [ohnoreflow][qf:p1:f67][fxperf:p1])

Attachments

(24 attachments, 7 obsolete attachments)

46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
210.61 KB, image/png
Details
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
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
Flags: qe-verify?
Priority: -- → P2

Comment 1

2 years ago
Please correct the component if needed. Thanks!
Component: Untriaged → Tabbed Browser
(Assignee)

Updated

2 years ago
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Flags: qe-verify? → qe-verify-
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:p2][reserve-photon-performance]
Priority: P3 → P4
Whiteboard: [ohnoreflow][qf:p2][reserve-photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance]
(Assignee)

Updated

a year ago
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][fxperf]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf] → [ohnoreflow][qf:f61][qf:p1][fxperf:p3]
(Assignee)

Updated

10 months ago
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p3] → [ohnoreflow][qf:f64][qf:p1][fxperf:p3]

Updated

10 months ago
Whiteboard: [ohnoreflow][qf:f64][qf:p1][fxperf:p3] → [ohnoreflow][qf:p1:f64][fxperf:p3]
(Assignee)

Comment 3

9 months ago
Re-queuing for triage by fxperf to reconcile with qf evaluation.
Whiteboard: [ohnoreflow][qf:p1:f64][fxperf:p3] → [ohnoreflow][qf:p1:f64][fxperf]
(Assignee)

Comment 4

9 months ago
We're doing this flush during every window open, it seems - including the first. Seems important enough to prioritize higher for now.
Whiteboard: [ohnoreflow][qf:p1:f64][fxperf] → [ohnoreflow][qf:p1:f64][fxperf:p2]
(Assignee)

Comment 5

7 months ago
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).
Assignee: nobody → mconley
(Assignee)

Comment 6

7 months ago
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?
Flags: needinfo?(dao+bmo)
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.
Flags: needinfo?(dao+bmo)
Depends on: 1487344
(Assignee)

Comment 8

7 months ago
(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!
(Assignee)

Comment 9

7 months ago
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
(Assignee)

Comment 10

7 months ago
Whoops, forgot a few:

* Restored window
* Maximized window
(Assignee)

Comment 11

7 months ago
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.
(Assignee)

Comment 12

7 months ago
And a few more configurations:

* RTL
* LTR
(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.
(Assignee)

Comment 14

7 months ago
(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.
(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.
(Assignee)

Comment 16

7 months ago
(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.
(Assignee)

Updated

7 months ago
Whiteboard: [ohnoreflow][qf:p1:f64][fxperf:p2] → [ohnoreflow][qf:p1:f64][fxperf:p1]
(Assignee)

Comment 17

7 months ago
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.
(Assignee)

Comment 18

7 months ago
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).
(Assignee)

Updated

7 months ago
Depends on: 1488582
(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.
(Assignee)

Updated

7 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 20

6 months ago
(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?
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 21

6 months ago
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>
...

?
<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.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 23

6 months ago
(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?
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 24

6 months ago
(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?
(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.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 26

6 months ago
I like the idea of using an #include. Let me try that route - thanks for the idea!
(Assignee)

Updated

6 months ago
Depends on: 1474340
Blocks: 1485291
(Assignee)

Comment 29

6 months ago
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.
(Assignee)

Comment 34

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

Depends on D7808
QA Contact: dao+bmo
(Assignee)

Comment 54

6 months ago
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
Posted image 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.
Blocks: 1447775
(Assignee)

Comment 56

5 months ago
(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?
Flags: needinfo?(stransky)
(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?
(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
Flags: needinfo?(stransky)
(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?
Flags: needinfo?(mconley)
(Assignee)

Comment 60

5 months ago
(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.
Flags: needinfo?(mconley)
How are we currently dealing with that margin?
(Assignee)

Comment 62

5 months ago
(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. :/
(Assignee)

Comment 63

5 months ago
Digging into bug 1283299 and friends to see if I can suss out where this margin is coming from...
(Assignee)

Comment 64

5 months ago
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.
(Assignee)

Comment 65

5 months ago
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
(Assignee)

Comment 66

5 months ago
(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.
Flags: needinfo?(stransky)
(Assignee)

Updated

5 months ago
Depends on: 1498356
(Assignee)

Updated

5 months ago
Blocks: 1498400
(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.
(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.
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.
Flags: needinfo?(stransky)
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.
(Assignee)

Updated

5 months ago
Blocks: 1498602
See Also: → bug 1498740
(Assignee)

Updated

5 months ago
Blocks: 1499706
Attachment #9014640 - Attachment description: Bug 1356920 - Bump the ordinal attribute on the window caption buttons to help ensure they stay at the end of the toolbar. r?dao → Bug 1356920 - Put an ordinal rule on the window caption buttons to help ensure they stay at the end of the toolbar. r?dao
(Assignee)

Comment 73

5 months ago
This thing is definitely not making it to 64, so bumping to 67 for qf tracking.
Whiteboard: [ohnoreflow][qf:p1:f64][fxperf:p1] → [ohnoreflow][qf:p1:f67][fxperf:p1]
(Assignee)

Comment 74

5 months ago
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.
Attachment #9014658 - Attachment is obsolete: true
Attachment #9014664 - Attachment is obsolete: true
Attachment #9014662 - Attachment is obsolete: true
Attachment #9014661 - Attachment is obsolete: true
(Assignee)

Comment 84

5 months ago
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.
(Assignee)

Comment 85

5 months ago
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.
(Assignee)

Comment 86

5 months ago
Any objection if I squash all of these commits together and we just work from a single commit from here forward?
Flags: needinfo?(dao+bmo)
Attachment #9020223 - Attachment is obsolete: true
Attachment #9020219 - Attachment is obsolete: true
Attachment #9020218 - Attachment is obsolete: true
(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?

👍
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 88

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

Updated

5 months ago
Depends on: 1503272
(Assignee)

Updated

5 months ago
Blocks: 1385729

Comment 89

5 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e354a018b703
Don't calculate and flush layout inside browser-tabsintitlebar.js. r=dao
(Assignee)

Updated

5 months ago
Blocks: 1503272
No longer depends on: 1503272

Comment 90

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e354a018b703
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Looks like this causes a major regression on Linux - Bug 1505314.
Depends on: 1505314
Mike, your patches still show up under "Waiting on Authors" in phabricator. Can you close / abandon them? Thanks.
Flags: needinfo?(mconley)
(Assignee)

Comment 93

5 months ago
(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.
Flags: needinfo?(mconley)

Updated

5 months ago
Depends on: 1505906

Updated

5 months ago
Depends on: 1505927
Depends on: 1506071
Blocks: 1506190
Blocks: 1506341
Summary: 6.33ms uninterruptible reflow at rect@chrome://browser/content/browser-tabsintitlebar.js:106:23 → Refactor tabs in title bar implementation to avoid JS layout calculations
(Assignee)

Updated

4 months ago
Depends on: 1505766
Depends on: 1507382
(Assignee)

Updated

4 months ago
Depends on: 1506457

Updated

4 months ago
Depends on: 1507536
Component: Tabbed Browser → Theme
Blocks: 967917

Updated

4 months ago
Blocks: 1332447

Updated

4 months ago
Depends on: 1508954
No longer blocks: 1498602
Depends on: 1498602
No longer depends on: 1498602
Depends on: 1498602
(Assignee)

Updated

4 months ago
Depends on: 1511053
Depends on: 1511106
Blocks: 1511496

Updated

4 months ago
Depends on: 1511905
Depends on: 1512159

Updated

4 months ago
Depends on: 1510855
Blocks: 1513200
Depends on: 1513468
Blocks: 1410878
Depends on: 1521803
Depends on: 1521688

Updated

a month ago
Depends on: 1526224
You need to log in before you can comment on or make changes to this bug.