1819421 - UI Density Not AutoAdjusting to Tablet on Convertible device

Status: RESOLVED FIXED

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

Description

[xplosneer]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/110.0
Firefox for Android

Steps to reproduce:

Start Firefox on Windows 11 on a Surface Pro 8.
Right click on the hamburger menu and select "Customize Toolbar."
At the bottom, select the "Density" dropdown. Click on Normal mode. Make sure that the "Use Touch for Tablet Mode" checkbox is selected.
Click "Done" to close the customization window.
Flip the keyboard over to enable tablet mode. Flip back to keyboard deployed to disable tablet mode.

Actual results:

The Windows interface switched into tablet mode as expected, including a resizing of the Task Bar, indicating that the keyboard flip was detected in some way.

The Firefox interface stayed in "Normal" UI Density mode throughout, never changing density.

Expected results:

The Firefox interface should have switched to "Touch" UI Density mode automatically. Once switched, the interface should switch back to "Normal" UI Density once the keyboard was redeployed.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Widget: Win32' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[xplosneer]

I think I've figured where this one is going wrong.

In WindowsUIUtils.cpp, tablet mode is determined by
UserInteractionMode mode;
hr = uiViewSettings->get_UserInteractionMode(&mode);
if (FAILED(hr)) {
return;
}

TabletModeState oldTabletModeState = sInTabletModeState;
sInTabletModeState = mode == UserInteractionMode_Touch ? TabletModeState::On
: TabletModeState::Off;

But that mode was deprecated in Windows 11 so hr is probably returning failed.

https://learn.microsoft.com/en-us/uwp/api/windows.ui.viewmanagement.uiviewsettings.userinteractionmode?view=winrt-26100#windows-ui-viewmanagement-uiviewsettings-userinteractionmode

The new method is ConvertibleSlateMode
https://learn.microsoft.com/en-us/windows-hardware/customize/desktop/unattend/microsoft-windows-gpiobuttons-convertibleslatemode

Comment 3

[Ray Kraesig [:rkraesig]]

Win10's Tablet Mode and Windows 11's "ConvertibleSlateMode" seem to be sufficiently different that I'm not sure we want to unconditionally apply all the code associated with the former when we detect the latter. It'll be necessary to check all the downstream users of GetInTabletMode/inTabletMode/"tablet-mode-change" individually and test their effects on Win11-Tablet-Mode-supporting devices before hooking that up. (There aren't that many users, but I don't happen to have appropriate Win11 hardware to test on.)

Fortunately, UI density (per that pref) is an exception; we almost certainly do want to apply it in Win11.

Comment 4

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1819421 - [1/2] create parallel check for Win11 tablet mode r?#win-reviewers!

Win11's tablet mode is different from Win10's tablet mode in ways both
subtle and gross. Avoid unifying them implicitly, and make it clear that
they should not be unified explicitly without testing.

As all of our current code focuses on Win10's tablet mode, no functional
changes.

Comment 5

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1819421 - [2/2] consider Win11 tablet mode for UI density r?#win-reviewers

Check for Windows 11 tablet mode when setting the UI density.

This includes a change to the semantics of "tablet-mode-change", which
at present is only used by the UI density code: it now signals Win11
tablet mode as distinct from Win10 tablet mode.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b11774655311
[1/2] create parallel check for Win11 tablet mode  r=win-reviewers,Gijs,gsvelto,gstoll
https://hg.mozilla.org/integration/autoland/rev/b4f770bfe94c
[2/2] consider Win11 tablet mode for UI density  r=win-reviewers,handyman,Gijs

Comment 7

[Atila Butkovits]

Backed out for causing linting failures at browser.js.

Backout link: https://hg.mozilla.org/integration/autoland/rev/cb5d305b01a7d475ef9f9f191586608dc5fd1180

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=b4f770bfe94c57efdde5a041e8b403c6923f1f6c&selectedTaskRun=IoZNq_SrQ6qChZIbfp0o5g.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=480808757&repo=autoland&lineNumber=336

Comment 8

[Ray Kraesig [:rkraesig]]

What linter even is that? It doesn't show up when I run ./mach lint browser/base/content/browser.js.

(Amending.)

Comment 9

[Cosmin Sabou [:CosminS]]

Ray, there were also these kind on mass failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=DhM3rbuKRwaSF7sW3O-JxA.1&resultStatus=testfailed%2Cbusted%2Cexception&revision=b4f770bfe94c57efdde5a041e8b403c6923f1f6c

Failure log: https://treeherder.mozilla.org/logviewer?job_id=480812190&repo=autoland&lineNumber=2151

Comment 10

[Ray Kraesig [:rkraesig]]

Fixed, and the Linux try run is green (as still is the Windows one).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa2645779d0a
[1/2] create parallel check for Win11 tablet mode  r=win-reviewers,Gijs,gsvelto,gstoll
https://hg.mozilla.org/integration/autoland/rev/bcab45db97e6
[2/2] consider Win11 tablet mode for UI density  r=win-reviewers,handyman,Gijs

Comment 12

[:Gijs (he/him)]

(In reply to Ray Kraesig [:rkraesig] from comment #8)

What linter even is that? It doesn't show up when I run ./mach lint browser/base/content/browser.js.

eslint; the failure log link has job details on the top left which read: Job: Linting opt JavaScript checks source-test-mozlint-eslint ES .

It's surprising to me that this didn't show up locally. It's also surprising to me that you got backed out; I would have expected this to be autofixed by the post-landing automatic commit bots.

Pinging Mark for the former (ie why this wouldn't show up locally). I'd ping someone for the latter if I knew who knew about this, but hopefully Mark can redirect that.

Comment 13

[Mark Banner (:standard8)]

(In reply to :Gijs (he/him) from comment #12)

(In reply to Ray Kraesig [:rkraesig] from comment #8)
It's surprising to me that this didn't show up locally. It's also surprising to me that you got backed out; I would have expected this to be autofixed by the post-landing automatic commit bots.

This was probably just bad timing. There was a regression a couple of weeks ago from an ESLint configuration change where the curly rule was turned off accidentally. It has been re-enabled in bug 1927798, which landed on autoland about 4 hours before the patch here landed. Hence why it couldn't be reproduced locally.

ESLint/Prettier are currently disabled for the post-landing automatic commit bots. When this was originally turned on, we discovered that the bot would automatically fix any ESLint rules with auto-fix capabilities, including ones that weren't specifically format orientated. I can't remember which ones in particular, but there were some we were worried about not being seen by developers.

Ideally we should allow ESLint & Prettier to be run separately so that the landing bot can apply any prettier changes (but not ESLint), that work is covered in bug 1803229 and others. Although, in this case, the curly fix-up still wouldn't be run, as that's part of ESLint and not Prettier.

Comment 14

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/fa2645779d0a
https://hg.mozilla.org/mozilla-central/rev/bcab45db97e6