1874301 - Support MenuTouchObserver outside of Windows

Status: RESOLVED FIXED

(Firefox :: Menus, enhancement)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

We have a report that the menu doesn't get bigger by touch input on Steam Deck, and it seems we use that behavior only on Windows: https://searchfox.org/mozilla-central/rev/961a9e56a0b5fa96ceef22c61c5e75fb6ba53395/browser/base/content/browser.js#1951-1953

    if (AppConstants.platform == "win") {
      MenuTouchModeObserver.init();
    }

That doesn't seem like a good condition, can we instead use something like PlatformSupportsTouch() as the condition? Do we have a good reason to keep limiting this to Windows?

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1874301 - Part 1: Toggle touchmode from nsXULPopupManager r=#desktop-theme-reviewers

Comment 2

[:Gijs (he/him)]

(In reply to Kagami [:saschanaz] (they/them) from comment #0)

That doesn't seem like a good condition, can we instead use something like PlatformSupportsTouch() as the condition?

Looks like that's a static thing that might be cached for the lifetime of the condition, so if I attach an external touch screen after the browser starts it'd be wrong?

The patch uses maxTouchPoints which gets spoofed by resistFingerprinting.

I think there should probably just not be any conditions for enabling the observer code; the popupshowing code will toggle the attribute if appropriate and otherwise it's a no-op.

Arguably the attribute setting/removing should be moved into the platform (specifically, layout/xul bits that open popups), then it'll be usable on windows other than the main browser window, and Thunderbird etc. as well (with a little bit of CSS work on their part).

Do we have a good reason to keep limiting this to Windows?

So fwiw I didn't know/recall why this was Windows only. Looking at annotate/blame finds me bug 1364896 and specifically bug 1364896 comment 9 and comment 27 and comment 28. This is effectively a dupe of bug 1383003, AFAICT.

I left a comment on phab but: the patch as-is is also insufficient as a bunch of the CSS is Windows only still.

Comment 3

[:Gijs (he/him)]

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

The patch uses maxTouchPoints which gets spoofed by resistFingerprinting.

... though not for system callers, I also don't see anywhere gtk-y that would set it to something non-0?

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

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

I think there should probably just not be any conditions for enabling the observer code; the popupshowing code will toggle the attribute if appropriate and otherwise it's a no-op.

Arguably the attribute setting/removing should be moved into the platform (specifically, layout/xul bits that open popups), then it'll be usable on windows other than the main browser window, and Thunderbird etc. as well (with a little bit of CSS work on their part).

I'm okay with making it unconditional 👍

I left a comment on phab but: the patch as-is is also insufficient as a bunch of the CSS is Windows only still.

Per searchfox [touchmode] selector is used in panelUI-shared.css, is it a Windows specific file?

Comment 5

[:Gijs (he/him)]

(In reply to Kagami [:saschanaz] (they/them) from comment #4)

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

I left a comment on phab but: the patch as-is is also insufficient as a bunch of the CSS is Windows only still.

Per searchfox [touchmode] selector is used in panelUI-shared.css, is it a Windows specific file?

No, that's shared. I don't know what query you're looking at. I left this on phab: https://searchfox.org/mozilla-central/search?q=%5Btouchmode&path=css&case=false&regexp=false

That shows uses in browser/themes/windows/browser.css which is windows-only.

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1874301 - Part 2: 1.3x zoom for touchmode r=#desktop-theme-reviewers

Depends on D198382

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fb3738bd07b
Part 1: Initialize MenuTouchModeObserver on all OS r=desktop-theme-reviewers,dao
https://hg.mozilla.org/integration/autoland/rev/3d1ef5fe232a
Part 2: 1.3x zoom for touchmode r=desktop-theme-reviewers,dao

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/0fb3738bd07b
https://hg.mozilla.org/mozilla-central/rev/3d1ef5fe232a

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: image.png

Oh no, I just found that zoom breaks overflow on the app menu: the scrollbar overflows too 🤔

Emilio, any idea why or how to fix? And also I found data:text/html,<div style="zoom: 2">foo</div> zooms the text too on Chrome but not on Firefox 👀

Comment 10

[:Gijs (he/him)]

(In reply to Kagami [:saschanaz] (they/them) from comment #9)

Created attachment 9373243 [details]
image.png

Oh no, I just found that zoom breaks overflow on the app menu: the scrollbar overflows too 🤔

Emilio, any idea why or how to fix? And also I found data:text/html,<div style="zoom: 2">foo</div> zooms the text too on Chrome but not on Firefox 👀

Can we back that change out for now, reopen this bug, and revisit how to do touchmode? We're 1 day before soft freeze.

Comment 11

[Kagami Rosylight [:saschanaz] (they/them)]

Self answer: it seems panel-viewstack's max-height is now zoomed too and thus started mismatching from the actual height of the menu. calculateMaxHeight should be adjusted with zoom factor too.

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

Can we back that change out for now, reopen this bug, and revisit how to do touchmode? We're 1 day before soft freeze.

That's definitely an option, I'd wait for Emilio's response for now though.

(Edit: I'll make a patch)

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I can try to look at the overflow stuff, but yeah the font-size issue is known, that's https://github.com/w3c/csswg-drafts/issues/9397.

Kagami, is backing out just the last patch enough? Let's file a separate issue for zoom if so, and I'd be happy to take a look.

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

Thanks for the link! And yeah let's just back out the last one. I have a patch but I'm not super confident that there's no remaining bug. I'll ping sheriffs.

Comment 14

[Serban Stanca [:SerbanS]]

Backed out https://hg.mozilla.org/mozilla-central/rev/3d1ef5fe232a as requested by Kagami for causing css overflow issue that blocks accessing app menu on certain devices. https://hg.mozilla.org/mozilla-central/rev/f593f07c97724141b65e9eb1b9aa4a3bfdb47b2a

Comment 15

[Kagami Rosylight [:saschanaz] (they/them)]

Part 1 patch is still in m-c, let's track the other one in bug 1875079.