Closed
Bug 1356920
Opened 8 years ago
Closed 6 years ago
Refactor tabs in title bar implementation to avoid JS layout calculations
Categories
(Firefox :: Theme, defect, P4)
Firefox
Theme
Tracking
()
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: mconley, Assigned: mconley)
References
(Blocks 3 open bugs, Regressed 4 open bugs)
Details
(Keywords: perf, Whiteboard: [ohnoreflow][fxperf:p1])
Attachments
(24 files, 7 obsolete files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
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
Updated•8 years ago
|
Flags: qe-verify?
Priority: -- → P2
Please correct the component if needed. Thanks!
Component: Untriaged → Tabbed Browser
Assignee | ||
Updated•8 years ago
|
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
Comment 2•8 years ago
|
||
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
Updated•8 years ago
|
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Updated•8 years ago
|
Flags: qe-verify? → qe-verify-
Updated•8 years ago
|
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:p2][reserve-photon-performance]
Updated•7 years ago
|
Priority: P3 → P4
Updated•7 years ago
|
Whiteboard: [ohnoreflow][qf:p2][reserve-photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Updated•7 years ago
|
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance]
Updated•7 years ago
|
Whiteboard: [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance]
Updated•7 years ago
|
Whiteboard: [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance]
Assignee | ||
Updated•7 years ago
|
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][fxperf]
Updated•7 years ago
|
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf] → [ohnoreflow][qf:f61][qf:p1][fxperf:p3]
Assignee | ||
Updated•7 years ago
|
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p3] → [ohnoreflow][qf:f64][qf:p1][fxperf:p3]
Updated•6 years ago
|
Whiteboard: [ohnoreflow][qf:f64][qf:p1][fxperf:p3] → [ohnoreflow][qf:p1:f64][fxperf:p3]
Assignee | ||
Comment 3•6 years 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•6 years 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•6 years 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•6 years 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)
Comment 7•6 years ago
|
||
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)
Assignee | ||
Comment 8•6 years 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•6 years 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•6 years ago
|
||
Whoops, forgot a few:
* Restored window
* Maximized window
Assignee | ||
Comment 11•6 years 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•6 years ago
|
||
And a few more configurations:
* RTL
* LTR
Comment 13•6 years ago
|
||
(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•6 years 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.
Comment 15•6 years ago
|
||
(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•6 years 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•6 years ago
|
Whiteboard: [ohnoreflow][qf:p1:f64][fxperf:p2] → [ohnoreflow][qf:p1:f64][fxperf:p1]
Assignee | ||
Comment 17•6 years 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•6 years 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).
Comment 19•6 years ago
|
||
(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•6 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 20•6 years 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 years 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>
...
?
Comment 22•6 years ago
|
||
<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 years 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 years 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?
Comment 25•6 years ago
|
||
(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 years ago
|
||
I like the idea of using an #include. Let me try that route - thanks for the idea!
Assignee | ||
Comment 27•6 years ago
|
||
Assignee | ||
Comment 28•6 years ago
|
||
Assignee | ||
Comment 29•6 years 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 30•6 years ago
|
||
Assignee | ||
Comment 31•6 years ago
|
||
Depends on D7805
Assignee | ||
Comment 32•6 years ago
|
||
Depends on D7806
Assignee | ||
Comment 33•6 years ago
|
||
Depends on D7807
Assignee | ||
Comment 34•6 years ago
|
||
The spacer isn't necessary, since the tabbrowser-tabs node will flex to fill the available
space anyways.
Depends on D7808
Assignee | ||
Comment 35•6 years ago
|
||
Depends on D7809
Assignee | ||
Comment 36•6 years ago
|
||
Depends on D7810
Assignee | ||
Comment 37•6 years ago
|
||
Depends on D7811
Assignee | ||
Comment 38•6 years ago
|
||
Depends on D7812
Assignee | ||
Comment 39•6 years ago
|
||
Depends on D7813
Assignee | ||
Comment 40•6 years ago
|
||
Depends on D7814
Assignee | ||
Comment 41•6 years ago
|
||
Depends on D7815
Assignee | ||
Comment 42•6 years ago
|
||
Depends on D7817
Assignee | ||
Comment 43•6 years ago
|
||
Depends on D7818
Assignee | ||
Comment 44•6 years ago
|
||
Depends on D7822
Assignee | ||
Comment 45•6 years ago
|
||
Depends on D7825
Assignee | ||
Comment 46•6 years ago
|
||
Depends on D7826
Assignee | ||
Comment 47•6 years ago
|
||
Depends on D7828
Assignee | ||
Comment 48•6 years ago
|
||
Depends on D7829
Assignee | ||
Comment 49•6 years ago
|
||
Depends on D7830
Assignee | ||
Comment 50•6 years ago
|
||
Depends on D7831
Assignee | ||
Comment 51•6 years ago
|
||
Depends on D7833
Assignee | ||
Comment 52•6 years ago
|
||
Depends on D7835
Assignee | ||
Comment 53•6 years ago
|
||
Depends on D7836
Updated•6 years ago
|
QA Contact: dao+bmo
Assignee | ||
Comment 54•6 years 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
Updated•6 years ago
|
QA Contact: dao+bmo
Comment 55•6 years ago
|
||
In maximized windows on Linux with the title bar disabled, I get a gap before the first tab that shouldn't be there.
Assignee | ||
Comment 56•6 years 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)
Comment 57•6 years ago
|
||
(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•6 years ago
|
||
(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)
Comment 59•6 years ago
|
||
(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•6 years 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)
Comment 61•6 years ago
|
||
How are we currently dealing with that margin?
Assignee | ||
Comment 62•6 years 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•6 years ago
|
||
Digging into bug 1283299 and friends to see if I can suss out where this margin is coming from...
Assignee | ||
Comment 64•6 years 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•6 years 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•6 years 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)
Comment 67•6 years ago
|
||
(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•6 years ago
|
||
(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•6 years ago
|
||
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)
Comment 70•6 years ago
|
||
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.
Updated•6 years ago
|
Assignee | ||
Comment 71•6 years ago
|
||
Updated•6 years ago
|
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 72•6 years ago
|
||
Assignee | ||
Comment 73•6 years 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•6 years 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.
Assignee | ||
Comment 75•6 years ago
|
||
Assignee | ||
Comment 76•6 years ago
|
||
Assignee | ||
Comment 77•6 years ago
|
||
Assignee | ||
Comment 78•6 years ago
|
||
Depends on D7814
Assignee | ||
Comment 79•6 years ago
|
||
Depends on D9669
Assignee | ||
Comment 80•6 years ago
|
||
Depends on D9670
Updated•6 years ago
|
Attachment #9014658 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9014664 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9014662 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9014661 -
Attachment is obsolete: true
Assignee | ||
Comment 81•6 years ago
|
||
Depends on D7814
Assignee | ||
Comment 82•6 years ago
|
||
Depends on D9872
Assignee | ||
Comment 83•6 years ago
|
||
Depends on D9873
Assignee | ||
Comment 84•6 years 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•6 years 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•6 years 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)
Updated•6 years ago
|
Attachment #9020223 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9020219 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #9020218 -
Attachment is obsolete: true
Comment 87•6 years ago
|
||
(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•6 years ago
|
||
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•6 years 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•6 years ago
|
Comment 90•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Comment 91•6 years ago
|
||
Looks like this causes a major regression on Linux - Bug 1505314.
Comment 92•6 years ago
|
||
Mike, your patches still show up under "Waiting on Authors" in phabricator. Can you close / abandon them? Thanks.
Flags: needinfo?(mconley)
Assignee | ||
Comment 93•6 years 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•6 years ago
|
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
Depends on: 1507382
Updated•6 years ago
|
Component: Tabbed Browser → Theme
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [ohnoreflow][qf:p1:f67][fxperf:p1] → [ohnoreflow][fxperf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•