1447775 - Window buttons are not centered vertically when main window is maximized and drag space is enabled

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Vlad Zahorodnii [:zzag]]

Attachment: with-drag-space.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180321102906

Steps to reproduce:

1. Use latest KDE Plasma (5.12.3)
2. Check "Drag Space" checkbox (i.e. enable drag space)
3. Uncheck "Title Bar" checkbox (i.e. enable CSD)
3. Maximize window


Actual results:

Window controls(close, maximize, minimize) aren't centered vertically.


Expected results:

Window controls(close, maximize, minimize) should be centered vertically.

Comment 1

[Vlad Zahorodnii [:zzag]]

Attachment: without-drag-space.png

For reference, this is how Firefox looks with drag space being disabled.

Comment 2

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

This affects only maximized windows, right? In that case the drag space is not rendered (it's rendered at normal state only AFAIK).

Comment 3

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

Yes, I can reproduce that on gnome-shell/Adwaita theme too.

Comment 4

[Vlad Zahorodnii [:zzag]]

(In reply to Martin Stránský [:stransky] from comment #2)
> In that case the drag space is not rendered (it's rendered at normal state only AFAIK).

I'd like to point out that there is a little space above tabs when Firefox is maximized and the drag space is disabled. Yet, there is no space above tabs when the drag space is enabled (when Firefox is maximized). :D

Comment 5

[Vlad Zahorodnii [:zzag]]

Window buttons are vertically centered. Did you fix that?

Comment 6

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

Looks like we need to call some refresh when maximize/unmaximize here. When the icons are rendered incorrectly it's enough to toggle menu bar and the rendering is fixed (at least for me).

Comment 7

[Vlad Zahorodnii [:zzag]]

(In reply to Martin Stránský [:stransky] from comment #6)
> Looks like we need to call some refresh when maximize/unmaximize here. When
> the icons are rendered incorrectly it's enough to toggle menu bar and the
> rendering is fixed (at least for me).

OK, after playing with enabling/disabling drag space and maximizing/unmaximizing Firefox, I could reproduce the bug again. Toggling menu bar helped me too.

Comment 8

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

This is a variant of Bug 715867. sizemodechange/resize is fireed *before* actual window.sizemode attribute is changed. The sequence is:

1) Window size is changed
2) sizemodechange/resize is fired
3) TabsInTitlebar._layOutTitlebar() calculates new titlebar size but with old TabsToolbar.height value:

   let fullTabsHeight = rect($("TabsToolbar")).height;

4) sizemode is set to "normal", CSS rule applies:

  /* Add extra space to titlebar for dragging */
  :root[sizemode="normal"][chromehidden~="menubar"] #TabsToolbar,
  :root[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive] + #TabsToolbar {
    padding-top: var(--space-above-tabbar);
  }

5) TabsToolbar has extra padding which causes icons are not centered.

Comment 9

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

Sizemode attribute is updated by nsXULWindow::SavePersistentAttributes() which is fired by timer. I wonder how that does work on other systems...

Comment 10

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

Attachment: Bug 1447775 - Change persist mode immediately after sizemodechange change,

Recently the window sizemode attribute is updated with 500ms delay (on Linux). It causes race condition
between reflow/CSS rule and window resize callbacks and also causes visual glitch when toolbar dragspace
is enabled.

TabsToolbar height is used at resize callback to do dragspace calculation. It has old values here
because it depends on 'window[sizemode="normal"] #TabsToolbar' css rule which is applied with 500ms delay.

So we use old TabsToolbar height for the dragspace, then the sizemode us updated by PersistenceTimer and
a new TabsToolbar height is calculated but the dragspace size stays as we're not notified
about the TabsToolbar height update.

Review commit: https://reviewboard.mozilla.org/r/235984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235984/

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(nothing to do with event handling.)

Comment 12

[Jim Mathies [:jimm]]

This is probably going to regress something. :) You should fire off a try run on multiple platforms for this just to check.

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8967302 [details]
Bug 1447775 - Change persist mode immediately after sizemodechange change,

https://reviewboard.mozilla.org/r/235984/#review242340

Comment 14

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

You're right, there's a regression on MacOS:

https://treeherder.mozilla.org/logviewer.html#?job_id=173232063&repo=try&lineNumber=20399

03:35:57     INFO - GECKO(2280) | [2280, Main Thread] ###!!! ASSERTION: mBounds out of sync!: 'mWindow && mBounds == r', file /builds/worker/workspace/build/src/widget/cocoa/nsCocoaWindow.mm, line 1717
03:35:57     INFO - GECKO(2280) | #01: nsBaseWidget::GetRestoredBounds(mozilla::gfx::IntRectTyped<mozilla::LayoutDevicePixel>&) [widget/nsBaseWidget.cpp:1661]
03:35:57     INFO - 
03:35:57     INFO - GECKO(2280) | #02: nsXULWindow::SavePersistentAttributes() [xpfe/appshell/nsXULWindow.cpp:1628]

Comment 17

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

Dao, is this one also covered by bug 1356920? Thanks.

Comment 19

[Dão Gottwald [:dao]]

(In reply to Martin Stránský [:stransky] from comment #17)
> Dao, is this one also covered by bug 1356920? Thanks.

Yeah, I guess it would fix this.

Comment 20

[Dão Gottwald [:dao]]

I believe this is fixed.

Comment 21

[Maria Berlinger [:mberlinger], Services QA]

I have managed to reproduce this issue on an affected Firefox 61.0a1 (20180320220122) build using Ubuntu 18.06 x64 by following the STR from comment 0.

This issue is verified fixed using Firefox 65.0b12 and Firefox 66.0a1 (20190120213632) on the following OSes: Windows 10 x64, mac 10.13.6, Ubuntu 18.06 x64, Windows 8.1 x64.