With tabsintitlebar disabled, when opening second/third windows from a maximized window, they don't open maximized

NEW
Unassigned

Status

()

Firefox
Tabbed Browser
3 years ago
a year ago

People

(Reporter: Black_Ps`, Unassigned)

Tracking

(Blocks: 1 bug, {regression})

29 Branch
All
Windows XP
regression
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite ?
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 obsolete attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:28.0) Gecko/20100101 Firefox/28.0 Waterfox/28.0 (Beta/Release)
Build ID: 20140331235340

Steps to reproduce:

When i enable title bar on Firefox 29,and i search for example with google and set google to open pages in new window,the pages are not open maximized like before,if i disable title bar it's working.



Expected results:

To open maximized like before.
(Reporter)

Updated

3 years ago
OS: Windows 8.1 → Windows XP

Updated

3 years ago
See Also: → bug 1005423
(Reporter)

Comment 1

3 years ago
Yes but i don't know if the problem is from title bar. Let's see the answer.

Updated

3 years ago
Duplicate of this bug: 1005423

Updated

3 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: firefox-backlog?
Summary: Firefox will not open maximized after update → With tabsintitlebar disabled, when opening second/third windows from a maximized window, they don't open maximized
(Reporter)

Comment 3

3 years ago
The problem is when it's enabled not disabled.
(Reporter)

Comment 4

3 years ago
I mean when Title Bar is enabled.

Comment 5

3 years ago
(In reply to Black_Ps` from comment #4)
> I mean when Title Bar is enabled.

Right, which means that the tabs are not in the titlebar. :-)

Comment 6

3 years ago
Are you going to fix it? I need to have the title bar enabled for the "hide tab bar with one tab" addon (and because I like my title bar)...

Comment 7

3 years ago
(In reply to Novgorod from comment #6)
> Are you going to fix it? I need to have the title bar enabled for the "hide
> tab bar with one tab" addon (and because I like my title bar)...

Are we generally going to fix it? I'm fairly certain we will. How quickly and/or am I personally going to be fixing it? I don't know. I've put it in our backlog, so it will get /some/ level of attention, at least.
Flags: firefox-backlog? → firefox-backlog+
Keywords: regression, regressionwindow-wanted

Comment 8

3 years ago
we are having this issue too - hyperlink with target="_blank" does not open new window as maximized

Comment 9

3 years ago
Gavin, what do I do if I think this is serious enough that it should be on our prio backlog?
Flags: needinfo?(gavin.sharp)
If you think you need to take it now, just do that, and then give it a point estimate and needinfo Marco to add it to the current iteration.

If you think we should add it for the next iteration, just mark it backlog+ and let dolske or I know you think it should be prioritized for the next iteration.
Flags: needinfo?(gavin.sharp)

Comment 11

3 years ago
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #10)
> If you think you need to take it now, just do that, and then give it a point
> estimate and needinfo Marco to add it to the current iteration.
> 
> If you think we should add it for the next iteration, just mark it backlog+
> and let dolske or I know you think it should be prioritized for the next
> iteration.

It needs investigation, and my plate is pretty full, plus I'm not 100% sure where to start (although a regression window would be a helpful beginning, I suppose), so I'd want to do the latter. Consider this comment a "letting you know" :-)

Comment 12

3 years ago
Hi, just wondering if this is still in the lineup for fixing?

Comment 13

3 years ago
I expect this is the result of the same underlying issue as bug 1063121, which we're actively working on.
See Also: bug 1005423bug 1063121

Comment 14

3 years ago
(In reply to :Gijs Kruitbosch from comment #13)
> I expect this is the result of the same underlying issue as bug 1063121,
> which we're actively working on.

Bug 1063121 didn't fix this. :-(

Gavin, can I renom this for backlog for next iteration?
Flags: qe-verify-
Flags: needinfo?(gavin.sharp)
Flags: in-testsuite?
See Also: bug 1063121

Comment 15

3 years ago
I just wanted to add that this bug also affects windows XP 64 as in the description says "x86 Windows XP". I can reproduce it in firefox 33 and in firefox 34 beta 8.
"Nom for backlog" means "add to the bottom of https://docs.google.com/a/mozilla.com/spreadsheets/d/16zM07MpI4jXHH1-ZNFm61erlRJmAqvX6266GJm-Pr20/edit?pli=1#gid=0". As usual, you don't need things to be prioritized by me to take them.
Flags: needinfo?(gavin.sharp)

Updated

3 years ago
Points: --- → 8

Comment 17

3 years ago
Hi when will this be fixed??

Comment 18

3 years ago
(In reply to Joako from comment #17)
> Hi when will this be fixed??

When someone writes a patch.

Unless someone outside the core Firefox Desktop team steps up to investigate the issue and/or write the patch, there's no clear timeline for this, though I've nominated it for inclusion in the work done by the desktop team for Firefox 38 (which doesn't necessarily mean it'd get accepted to be picked up then, and if it did, that 38 would be the first release with the fix - it could be either sooner or later, depending on how complex the fix is and so on).

What would already be helpful is if someone narrowed down the regression range of when this broke using mozregression and the UX repository (assuming this broke with the Australis merge, which may or may not be the case in itself... ).
Flags: qe-verify- → qe-verify+
Hardware: x86 → All

Comment 19

2 years ago
I've run through mozregression to isolate the range.  It does appear to be the Australis release.  It's my first time using this utility so apologies if I have made an error.  This is what I received from running the command 'mozregression --good-release 27 --bad-release 29' on a Windows 7 Pro x86 machine.

 7:20.29 LOG: MainThread Bisector INFO Last good revision: a475f94bb1b1
 7:20.29 LOG: MainThread Bisector INFO First bad revision: f2adb62d07eb
 7:20.29 LOG: MainThread Bisector INFO Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a475f94bb1b1&tochange=f2adb62d07eb


For each test I would 

open about:config 
set browser.tabs.drawInTitlebar to false 
set browser.link.open_newwindow to 2 

browse to http://jsbin.com/yobatehube/1/edit and try the test link.  

Hope it helps.

Comment 20

2 years ago
I forgot to add that the very first thing I would do is to maximize the Firefox window.  Then I would open about:config.

Comment 21

2 years ago
(In reply to ron from comment #19)
> I've run through mozregression to isolate the range.  It does appear to be
> the Australis release.  It's my first time using this utility so apologies
> if I have made an error.  This is what I received from running the command
> 'mozregression --good-release 27 --bad-release 29' on a Windows 7 Pro x86
> machine.
> 
>  7:20.29 LOG: MainThread Bisector INFO Last good revision: a475f94bb1b1
>  7:20.29 LOG: MainThread Bisector INFO First bad revision: f2adb62d07eb
>  7:20.29 LOG: MainThread Bisector INFO Pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=a475f94bb1b1&tochange=f2adb62d07eb
> 
> 
> For each test I would 
> 
> open about:config 
> set browser.tabs.drawInTitlebar to false 
> set browser.link.open_newwindow to 2 
> 
> browse to http://jsbin.com/yobatehube/1/edit and try the test link.  
> 
> Hope it helps.

This means it broke in the Australis merge, but it doesn't help in figuring out which part of Australis broke this (as you can see, there are a *lot* of changes in that regression range). I'll have a look to see if I have time to run that regression analysis myself later today.

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 22

2 years ago
http://hg.mozilla.org/projects/ux/pushloghtml?fromchange=bfb5c27c566b&tochange=339479a60c1c
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: regressionwindow-wanted

Updated

2 years ago
Blocks: 882009

Comment 23

2 years ago
It seems what's happening here is:

1) the JS calls updateTitlebarDisplay
2) this removes the chromemargin attribute because drawintitlebar is false. This is required because this attributed is there by default.
3) this calls into C++ code, specifically SetNonClientMargins.

Let's pause here for a second: at this point, the XUL, C++ and everything else agree that the window is maximized.

4) SetNonClientMargins gets called with a "reset" value (-1,-1,-1,-1), which means it calls ResetLayout() to "force a reflow of content based on the new client dimensions"
5) this calls:

  // This will trigger a frame changed event, triggering
  // nc calc size and a sizemode gecko event.
  SetWindowPos(mWnd, 0, 0, 0, 0, 0,
               SWP_FRAMECHANGED|SWP_NOACTIVATE|SWP_NOMOVE|
               SWP_NOOWNERZORDER|SWP_NOSIZE|SWP_NOZORDER);


6) this triggers windows sending a WM_WINDOWPOSCHANGED message
7) this message gets here:

http://hg.mozilla.org/mozilla-central/annotate/ec1351f9bc58/widget/windows/nsWindow.cpp#l5882

  // Handle window size mode changes
  if (wp->flags & SWP_FRAMECHANGED && mSizeMode != nsSizeMode_Fullscreen) {

    // Bug 566135 - Windows theme code calls show window on SW_SHOWMINIMIZED
    // windows when fullscreen games disable desktop composition. If we're
    // minimized and not being activated, ignore the event and let windows
    // handle it.
    if (mSizeMode == nsSizeMode_Minimized && (wp->flags & SWP_NOACTIVATE))
      return;

    WINDOWPLACEMENT pl;
    pl.length = sizeof(pl);
    ::GetWindowPlacement(mWnd, &pl);

    // Windows has just changed the size mode of this window. The call to
    // SizeModeChanged will trigger a call into SetSizeMode where we will
    // set the min/max window state again or for nsSizeMode_Normal, call
    // SetWindow with a parameter of SW_RESTORE. There's no need however as
    // this window's mode has already changed. Updating mSizeMode here
    // insures the SetSizeMode call is a no-op. Addresses a bug on Win7 related
    // to window docking. (bug 489258)
    if (pl.showCmd == SW_SHOWMAXIMIZED)
      mSizeMode = (mFullscreenMode ? nsSizeMode_Fullscreen : nsSizeMode_Maximized);
    else if (pl.showCmd == SW_SHOWMINIMIZED)
      mSizeMode = nsSizeMode_Minimized;
    else if (mFullscreenMode)
      mSizeMode = nsSizeMode_Fullscreen;
    else
      mSizeMode = nsSizeMode_Normal;

which sets the window's mSizeMode.

Now, I don't know if the SetWindowPos call really changes the sizemode and we just need to deal with that and set it back afterwards, or if it doesn't in this case and this code is just wrong in assuming that that will always be the case. The MSDN docs (if I'm reading them right) do seem to indicate that it is necessary to call SetWindowPos when you change the NC area.

In any case, this code sets the size mode to "normal" and from that point on we've lost (everything gets updated to "normal window size now!").

In summary, it seems that calling SetNonClientMargins *after* the sizemode has settled but *before* the window is visible (see below) is currently a Bad Idea.

I'm not sure what the best way to fix this is. Ideas include:

1) make the C++ code not set mSizeMode in this specific case by keeping track of why we're firing SetWindowPos. I don't know enough about the win32 api to know if this is possible or if at the point in step 7 windows already really wants to make this a normal-sized window.
2) make the C++ code keep track of this from SetNonClientMargins and re-set the sizemode back to what it was
3) change the XUL persistence code to persist the clientmargins attribute on Windows and the native code (!) that calls nsWindow with this info to call SetNonClientMargins before it updates the window with the new sizemode

I don't know this code well enough to decide what is best. Jim, thoughts?

Oh, some final info: in the ctrl-n case (ie opening a new browser window using ctrl-n), which does work, it seems we (for whatever reason) don't get around to sending the updateTitlebarDisplay thing until after the window has appeared in the taskbar, at which point the message we get in the equivalent of step 7 no longer wants to resize the window to 'normal'.
Flags: needinfo?(jmathies)

Comment 24

2 years ago
jsbin tries to defeat window.open calls and so the testcase didn't work for me. I stuck in a simple data:text/html one instead.

Comment 25

2 years ago
What are the steps to reproduce this bug? When I click on the link in the testcase, it opens a new tab.
Flags: needinfo?(jmathies)

Comment 26

2 years ago
(In reply to Jim Mathies [:jimm] from comment #25)
> What are the steps to reproduce this bug? When I click on the link in the
> testcase, it opens a new tab.

See comment 19. Turn off opening _blank links in a new tab in about:config / about:preferences, and turn on the normal Windows titlebar (either in about:config or about:customizing).
Flags: needinfo?(jmathies)

Comment 27

2 years ago
WFM on win7.

STR:
1) set browser.tabs.drawInTitlebar = false
2) set browser.link.open_newwindow = 2
3) restart
4) open test case here
5) set window to maximized
5) click on the 'Click' link in the test case

result: a new, mimimalist, chrome window opens maximized on the desktop on top of the previously maximized window.

Wondering what I'm missing here?


FWIW - this is an incredibly obscure bug, we should try to avoid mucking around in that widget code for this if possible - every time we tweak it we fix one bug and introduce five.
Flags: needinfo?(jmathies)

Comment 28

2 years ago
I created a ~2:00 demonstration video of the bug.  

https://youtu.be/i-mm324sNT8

The test URL in the header of this page has been truncated so I used jsbin.
Gijs: We are cleaning items left in an untriage state - can you please provide assistance... 
This appears to be Core: XUL related, Perhaps it can also be closed. I have confirmed fix 
Version 	43.0.4
User Agent 	Mozilla/5.0 (Windows NT 5.1; rv:43.0) Gecko/20100101 Firefox/43.0
Version 	46.0a1
User Agent 	Mozilla/5.0 (Windows NT 5.1; rv:46.0) Gecko/20100101 Firefox/46.0
Flags: needinfo?(gijskruitbosch+bugs)

Comment 30

2 years ago
Please let me know if I am not being helpful.  

This bug does not appear fixed in 46.0a1 (2016-01-07) on my platform (Win 7).

I've created screenshots of the bug in this version.
http://imgur.com/a/toDVg

The images are of the following:
0 - (not pictured) Maximize the Firefox window
1 - Version
2 - Disable the 'Open new windows in a new tab instead' option
3 - Enable the 'Title Bar'
4 - Test site - in this case http://www.w3schools.com/tags/att_a_target.asp
5 - New window does not open maximized

If the Title Bar is changed back to disabled then the new window in image 5 opens maximized.

Comment 31

2 years ago
(In reply to Michelle Funches - QA from comment #29)
> Gijs: We are cleaning items left in an untriage state - can you please
> provide assistance... 
> This appears to be Core: XUL related, Perhaps it can also be closed. I have
> confirmed fix 
> Version 	43.0.4
> User Agent 	Mozilla/5.0 (Windows NT 5.1; rv:43.0) Gecko/20100101 Firefox/43.0
> Version 	46.0a1
> User Agent 	Mozilla/5.0 (Windows NT 5.1; rv:46.0) Gecko/20100101 Firefox/46.0

I'm fairly sure this is either Fx::somethingorother or core::widget win32, but it's definitely still valid as per comment #30. I would have liked to look at this today but I was too busy. I'll try again on Monday. Leaving needinfo.

Comment 32

2 years ago
I can still reproduce this on both win8 and win7.

Going to see quickly if there's a sensible way to avoid this issue from the JS side.
Component: Untriaged → Tabbed Browser

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)

Comment 33

2 years ago
Created attachment 8707007 [details]
MozReview Request: Bug 1005098 - don't update titlebar attributes before the window has been activated, r?MattN

Review commit: https://reviewboard.mozilla.org/r/30549/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30549/
Attachment #8707007 - Flags: review?(MattN+bmo)

Comment 34

2 years ago
Comment on attachment 8707007 [details]
MozReview Request: Bug 1005098 - don't update titlebar attributes before the window has been activated, r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30549/diff/1-2/

Comment 35

2 years ago
FWIW, it might be interesting to write a test for this, which I haven't done yet, but should be possible.
Comment on attachment 8707007 [details]
MozReview Request: Bug 1005098 - don't update titlebar attributes before the window has been activated, r?MattN

https://reviewboard.mozilla.org/r/30549/#review27921

::: browser/base/content/browser-tabsintitlebar.js:270
(Diff revision 2)
> +    if (AppConstants.platform == "win" && Services.focus.activeWindow != window) {
> +      if (!TabsInTitlebar._titlebarUpdateListener) {
> +        TabsInTitlebar._titlebarUpdateListener = e => {
> +          window.removeEventListener("activate", TabsInTitlebar._titlebarUpdateListener);
> +          updateTitlebarDisplay();
> +        };
> +        window.addEventListener("activate", TabsInTitlebar._titlebarUpdateListener);
> +      }
> +      return;
> +    }
> +    TabsInTitlebar.haveUpdatedTitlebarDisplay = true;

This is quite complicated to understand with the nesting, three variables (`haveUpdatedTitlebarDisplay`, `_titlebarUpdateListener`, and `Services.focus.activeWindow != window`), and the `return` but I think this makes sense.  It seems to work on Windows and is basically a no-op on non-Windows.
Attachment #8707007 - Flags: review?(MattN+bmo) → review+

Comment 37

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/eb1e9ec43a36

Updated

2 years ago
Blocks: 1240541

Comment 38

2 years ago
Backout:
https://hg.mozilla.org/integration/fx-team/rev/020242b2d903

Comment 39

2 years ago
(In reply to Pulsebot from comment #38)
> Backout:
> https://hg.mozilla.org/integration/fx-team/rev/020242b2d903

So the width of some tests here no longer was what we expected it to be (only happened on win7 - winxp and win8 were fine):

https://treeherder.mozilla.org/logviewer.html#?job_id=6667719&repo=fx-team
https://treeherder.mozilla.org/logviewer.html#?job_id=6668347&repo=fx-team

and this test broke because of height differences, too (win7 and win8, not winxp):

https://treeherder.mozilla.org/logviewer.html#?job_id=6670394&repo=fx-team

I *think* this could be fixed by making the whole sha-doodle dependent on whether TabsInTitlebar.enabled isn't true, but that seems hacky as ...

I'm also a little suspicious about how we have no real test coverage for that case and so maybe there are issues in that case, too, but we just don't find them. I did try to run the test in question with TabsInTitlebar disabled, and it passed both with and without this change...

Matt, thoughts?
Flags: needinfo?(MattN+bmo)
(In reply to :Gijs Kruitbosch from comment #39)
> So the width of some tests here no longer was what we expected it to be
> (only happened on win7 - winxp and win8 were fine):
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=6667719&repo=fx-team
> https://treeherder.mozilla.org/logviewer.html#?job_id=6668347&repo=fx-team

FYI this fails for me locally on Windows 8.1 Pro in the same way (and passes before your patch).

> and this test broke because of height differences, too (win7 and win8, not
> winxp):
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=6670394&repo=fx-team

Likewise for this test.

> I *think* this could be fixed by making the whole sha-doodle dependent on
> whether TabsInTitlebar.enabled isn't true, but that seems hacky as ...

Made what dependent on "TabsInTitlebar.enabled isn't true"? These tests or the code you added?

> I'm also a little suspicious about how we have no real test coverage for
> that case and so maybe there are issues in that case, too, but we just don't
> find them. I did try to run the test in question with TabsInTitlebar
> disabled, and it passed both with and without this change...

Sorry, which case?

Have you convinced yourself that the expected test output is actually correct? Perhaps the test is wrong? Feel free to focus on one test file at a time to simplify.

I think maybe we should talk on Vidyo tomorrow as I'm not understanding the whole situation so ping me tomorrow.
Flags: needinfo?(MattN+bmo)

Comment 41

2 years ago
(In reply to Matthew N. [:MattN] from comment #40)
> (In reply to :Gijs Kruitbosch from comment #39)
> > So the width of some tests here no longer was what we expected it to be
> > (only happened on win7 - winxp and win8 were fine):
> > 
> > https://treeherder.mozilla.org/logviewer.html#?job_id=6667719&repo=fx-team
> > https://treeherder.mozilla.org/logviewer.html#?job_id=6668347&repo=fx-team
> 
> FYI this fails for me locally on Windows 8.1 Pro in the same way (and passes
> before your patch).

Both? They're the same except for the extension (one is .html and one is .xul), and I remember I ran one when this broke when I landed, and it passed on my win8.1 pro machine...

> > and this test broke because of height differences, too (win7 and win8, not
> > winxp):
> > 
> > https://treeherder.mozilla.org/logviewer.html#?job_id=6670394&repo=fx-team
> 
> Likewise for this test.

Yes, I can reproduce this, too.

> > I *think* this could be fixed by making the whole sha-doodle dependent on
> > whether TabsInTitlebar.enabled isn't true, but that seems hacky as ...
> 
> Made what dependent on "TabsInTitlebar.enabled isn't true"? These tests or
> the code you added?

The code I added. Sorry for being so vague and unclear. :-(

> > I'm also a little suspicious about how we have no real test coverage for
> > that case and so maybe there are issues in that case, too, but we just don't
> > find them. I did try to run the test in question with TabsInTitlebar
> > disabled, and it passed both with and without this change...
> 
> Sorry, which case?

The !TabsInTitlebar.enabled case. I don't think any of our window opening tests verify what happens when that is set.

> Have you convinced yourself that the expected test output is actually
> correct? Perhaps the test is wrong? Feel free to focus on one test file at a
> time to simplify.

Not in-depth, but considering the bug as described here, and the cause being updating the chrome margins at the "wrong" time, I wouldn't be at all surprised that changing the timing causes minor changes in how we compute widths/heights of windows, especially to do with window border sizes (where I think at least on win7 we use smaller sizes than is the default, but I could be misremembering that). If we make such changes at a later/earlier time than we used to, that could explain us being off by a couple of pixels compared to the current case, and also why some of these tests were intermittent before and already have a ~2px fault tolerance.

> I think maybe we should talk on Vidyo tomorrow as I'm not understanding the
> whole situation so ping me tomorrow.

Sure! I'll ping you when you're more likely to be awake. :-)
(In reply to Jim Mathies [:jimm] from comment #27)
> FWIW - this is an incredibly obscure bug, we should try to avoid mucking
> around in that widget code for this if possible - every time we tweak it we
> fix one bug and introduce five.

This sounds like a rather unhealthy strategy. The situation is only going to get worse if we add platform-specific hacks as Gijs did in his patch, because then we have obscure front-end code interacting with obscure widget code and nobody dares to touch anything anymore.

Comment 43

a year ago
Not actively working on this.
Assignee: gijskruitbosch+bugs → nobody
Status: ASSIGNED → NEW

Updated

a year ago
Attachment #8707007 - Attachment is obsolete: true
Attachment #8707007 - Flags: review+

Updated

a year ago
Duplicate of this bug: 1235434
You need to log in before you can comment on or make changes to this bug.