Closed Bug 1746774 Opened 3 years ago Closed 3 years ago

Recent history list exceeds window height after some monitor changes (availRect is not correct).

Categories

(Core :: Widget: Win32, defect, P1)

Firefox 95
defect

Tracking

()

VERIFIED FIXED
102 Branch
Tracking Status
firefox102 --- fixed

People

(Reporter: jake, Assigned: handyman)

References

Details

(Keywords: multi-monitors, Whiteboard: [win:multimonitors][win:sizing])

Attachments

(7 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:95.0) Gecko/20100101 Firefox/95.0

Steps to reproduce:

  1. Set up Windows 10 on laptop with second display (this might not all be necessary):
    • laptop 1920x1080, 125%
    • second display 1920x1080, 100%
    • second display set as main display and on the right.
  2. Have at least 22 pages in browser history.
  3. Click the books-on-shelf icon then History.

Actual results:

The 'Manage History' button below the list of items is mostly clipped by the window - see attached.

Note this is not always reproducible but has been an issue for some time.

Expected results:

'Manage History' button should be fully within the browser window.

Note window is maximized but not full screen.

Component: Untriaged → Menus

Thanks for the report. To be clear, when you say "this might not all be necessary" - does this bug reproduce at all without the secondary display? And to confirm, Firefox is on the laptop screen here, which you've configured as secondary display (on the left), is that correct?

Flags: needinfo?(jake)

I mentioned this was intermittent, but I have now found out what triggers it to occur. Sorry I also did not state which display Firefox was opened on.

Here are replacement steps to reproduce from step 3:

  1. Open Firefox maximized on the secondary display (which is the 'main' display).
  2. Unplug the secondary display (so Firefox now moves to the laptop's display and the system becomes single display).
  3. (Optional) Plug back in the secondary display and reposition Firefox on it as in 3.
  4. Click the books-on-shelf icon then History.
Flags: needinfo?(jake)

The severity field is not set for this bug.
:jaws, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jaws)
Severity: -- → S4
Priority: -- → P3

Emilio, is this likely to be related to the work you're doing in bug 1753836, bug 1741830 and friends?

Flags: needinfo?(emilio)

Yeah, let's re-test this after bug 1753836.

Flags: needinfo?(emilio)
Depends on: 1753836

Hi Jake, now that bug 1753836 has been fixed, can you test out the latest Nightly build and see if the issue is fixed for you?

Flags: needinfo?(jaws) → needinfo?(jake)

Hi Jake, now that bug 1753836 has been fixed, can you test out the latest Nightly build and see if the issue is fixed for you?

I don't currently use Nightly on my PC, and don't have time nor inclination to set it up, as no doubt it will conflict with my regular installation, or I'd have to set up a separate user account to isolate it. Can you let me know when this is in the mainstream build, and I will check it then?

Flags: needinfo?(jake) → needinfo?(jaws)

Nightly shouldn't mess with regular Firefox, it uses a totally different profile.

(In reply to jake from comment #8)

I don't currently use Nightly on my PC, and don't have time nor inclination to set it up, as no doubt it will conflict with my regular installation, or I'd have to set up a separate user account to isolate it. Can you let me know when this is in the mainstream build, and I will check it then?

The fix landed in Nightly 99 which will get released as Firefox 99 on April 5th.

To Emilio's point though, you can install Nightly and it operates separately from Firefox in that it uses a separate profile and should have no conflicts. The sooner we can validate that it is or is not fixed will allow us more time to figure it out a better fix, if needed.

Flags: needinfo?(jaws)

I tested with Nightly, but am unable to determine whether or not the problem as addressed as intended.

This is because there has been another change which now sets the height of submenus (including the History submenu) to the same height as the main hamburger menu; this change is also present in Firefox 98.0.2 (see attachment I'm about to add).

Though I guess I could try setting Windows DPI scaling to 200%...

Attached image bug-1746774-nightly.png (deleted) —

The problem can still be reproduced in Nightly. If I set Windows DPI scaling to 175%, the main hamburger menu is clipped at the bottom after unplugging and plugging back in the HDMI cable. See this attached screenshot.

(In reply to jake from comment #13)

... after unplugging and plugging back in the HDMI cable.

This reminds me of what I'm experiencing in bug 1629479.

Emilio, is there any chance you still have a setup that makes it easy to investigate further based on comment #11 and later?

Flags: needinfo?(emilio)

Emilio, is there any chance you still have a setup that makes it easy to investigate further

I would hazard a guess that this could be reproduced on any setup involving a laptop with a 2nd monitor connected via HDMI. Key to reproduction is unplugging then plugging back in the HDMI monitor. Having a different DPI between screens may also be a factor.

Can you share your specific Windows display settings? As in, the monitors, the positioning, and the scale factor and resolution of each of them?

Flags: needinfo?(jake)

Also, I don't have a dual-monitor setup until tomorrow, but something useful to test would be whether screen.height changes after unplugging and plugging back the monitor.

Tested on my laptop machine with latest Nightly, some details of my setup:
Laptop with 150% DPI, connected via DP and VGA to 2 identical 24" NEC monitors via a docking station, each one of these monitors are set to 100% DPI. All 3 monitors are set to 1920x1080.
Positioning in display settings is in the following order: laptop monitor (1), the first 24" monitor (2), the second 24" monitor (3).

STR for me:

  1. Place Nightly on the laptop monitor, verify the tabs list panel (or any other panel, see bug 1629479) is not exceeding the window height.
  2. See in the browser console that screen.height computes to 720.
  3. Disconnect the laptop from the docking station, this is effectively the same as jake's unplugging the HDMI cable.
  4. See that the history list exceeds the window height.
  5. screen.height still computes to 720.

Let me know if you need anything else :)

Are the monitors positioned vertically (in a column) horizontally (in a row) or mixed (the two big monitors in a row above the laptop monitor or so)?

Attached image Positioning (deleted) —

Thanks Itiel. I can repro the issue with a similar setup (with just one extra monitor).

It seems the PanelMultiView code fails to compute the right screen available height here.

That is because the screen info shows availRect == rect, which is wrong.

That info comes from ScreenHelperWin, which gets refreshed on a WM_DISPLAYCHANGE message here.

It seems like the EnumerateMonitors call should do the right thing, but we're somehow not getting the right rcWork rect. Maybe we're missing one message that would inform us of this change or so?

David, do you know if there's any way to log windows messages we could need to react to? I recall there being a log channel for this in debug builds, but I don't know off-hand what is it.

Flags: needinfo?(emilio) → needinfo?(davidp99)
Component: Menus → Widget: Win32
Product: Firefox → Core
Summary: Recent history list exceeds window height → Recent history list exceeds window height after some monitor changes (availRect is not correct).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: multi-monitors
Whiteboard: [win:multimonitors]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #22)

David, do you know if there's any way to log windows messages we could need to react to? I recall there being a log channel for this in debug builds, but I don't know off-hand what is it.

Indeed there is. TL;DR: We should probably be calling RefreshScreens on WM_DEVICECHANGE under DBT_DEVICEARRIVAL and DBT_DEVICEREMOVECOMPLETE (or something like that).

First off, in looking at this, I started to reproduce the issue but I don't need to play gymnastics with primary display and disconnecting the monitor -- I can see the nightly tabs window sized fine when I launch with one display connected (I have many open tabs). If I then connect an external display, I get poor tabs-list sizing. But I can also reproduce the issue as reported.

Supporting Emilio's theory that we aren't updating the monitor info is that things "look" fine if both monitors have the same dimensions/DPI and the taskbar is docked in the same spot. The easiest way to see this bug seems to be to have the taskbars on different edges of the screen and drag the window between them. Actually, the easiest is simply to move the taskbar without even having two screens. The tab list seems to ignore this. I should mention that I usually had my displays stacked vertically, not horizontally.

The solution to this bug is probably WM_DEVICECHANGE but the taskbar issue is a bit more than that -- the taskbar is just an easy way to see the failure without caring about display dimensions/DPI. Indeed, WM_DEVICECHANGE should fix the taskbar issue... but only when adding/removing devices but that's not the only way for the taskbar to move so we'll also need to RefreshScreens when the taskbar is moved. (I think the WinTaskbar stuff must be capable of this but there is also SPI_SETWORKAREA.)

Max recently made Windows event logging more ergonomic (bug 1751281) so it's no longer a "special debug build" thing -- the log "WindowsEvent" records many messages sent to the window but, in order to avoid spamming, it filters consecutive duplicate events and some of the noisier types (like WM_MOUSEMOVE). It doesn't yet "symbolicate" the event data yet so it's still pretty difficult to use. Now, it's duplicating a lot of what Spy++ does and Spy++ does symbolicate many event parameters, etc but it never seems to show the ones I want.

FWIW, I reproduced the issue (with all toolbars at the bottom of the screens + horizontally oriented screens) with a primary display with 1080p@100% and the external display (not set as primary) with 2160p@150%.

I'm attaching Spy++ logs:

  1. disconnect_monitor_spy.txt is from a Firefox window under the circumstances of this bug -- the window was on an external monitor and that monitor was disconnected. Note that WM_DEVICECHANGE comes much later than WM_DISPLAYCHANGE -- it comes after the WM_DPICHANGED, etc messages have been handled. So it's probably got the updated info.
  2. one_monitor_moved_taskbar_spy.txt is taken when only one monitor was connected and the taskbar was moved from the bottom to the side of the screen. Note the WM_SETTINGCHANGE message carrying SPI_SETWORKAREA.
Flags: needinfo?(davidp99)
Attached file disconnect_monitor_spy.txt (deleted) —
Attached file one_monitor_moved_taskbar_spy.txt (deleted) —

Can you share your specific Windows display settings? As in, the monitors, the positioning, and the scale factor and resolution of each of them?

As in the OP Description STR:

  • laptop 1920x1080, 125%
  • second display 1920x1080, 100%
  • second display set as main display and on the right.

If relevant, laptop is Dell Inspiron 5570; monitor is HP Compaq L2311c, connected via HDMI-to-VGA conversion cable.

Flags: needinfo?(jake)

Emilio or David, are either of you in a position to work on this? The frontend team isn't likely to have the right combination of setup + experience + time to do so themselves. :-)

Worth noting that this is likely to also affect various other menus and/or window/tooltip/panel-positioning code, not just the tabs/history list (that's just a straightforward way to reproduce).

Flags: needinfo?(emilio)
Flags: needinfo?(davidp99)

I'm looking at multi-monitor issues anyway so I'll take a look. There is something more elaborate going on -- updating the monitor list doesn't seem to be enough.

Assignee: nobody → davidp99
Flags: needinfo?(emilio)
Flags: needinfo?(davidp99)
Priority: P3 → P1

I'm seeing EnumDisplayMonitors return bad values for rcWork (ignoring taskbar) during WM_DISPLAYCHANGE. However, the values always seem to be right during the WM_SETTINGCHANGE message with SPI_SETWORKAREA. Updating the monitor list there looks like it's fixing the issue. I don't yet know if we still need to update monitors on the other messages.

This resembles what I believe is the fix for this in Chromium -- it comes from Electron.

This corrects erroneous values we sometimes get from EnumDisplayMonitors during WM_DISPLAYCHANGE. The issue is that adding/removing a display or moving the taskbar can cause the screen availRect values obtained from Windows to be incorrect.

Whiteboard: [win:multimonitors] → [win:multimonitors][win:sizing]
Pushed by daparks@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b869511e4867 Update monitors on Windows during WM_SETTINGCHANGE/SPI_SETWORKAREA r=emilio
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch

This seem to be working well now. Thanks!

jake, can you confirm this is fixed for you as well?

Flags: needinfo?(jake)

jake, can you confirm this is fixed for you as well?

Quickish test with 102.0a1 (2022-05-07) (64-bit) seems to indicate it is fixed. I started FF, then unplugged and plugged back in the HDMI, with the same set up otherwise. Could not repro. Then closed and re-opened FF and still could not repro.

Flags: needinfo?(jake)

Thank you!

Status: RESOLVED → VERIFIED
Duplicate of this bug: 1746761
Duplicate of this bug: 1629479
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: