Support MenuTouchObserver outside of Windows
Categories
(Firefox :: Menus, enhancement)
Tracking
()
People
(Reporter: saschanaz, Assigned: saschanaz)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files, 1 obsolete file)
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?
Assignee | ||
Comment 1•8 months ago
|
||
Comment 2•8 months ago
|
||
(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•8 months ago
|
||
(In reply to :Gijs (he/him) from comment #2)
The patch uses
maxTouchPoints
which gets spoofed byresistFingerprinting
.
... though not for system callers, I also don't see anywhere gtk-y that would set it to something non-0?
Assignee | ||
Comment 4•8 months ago
|
||
(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•8 months ago
|
||
(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®exp=false
That shows uses in browser/themes/windows/browser.css
which is windows-only.
Updated•8 months ago
|
Updated•8 months ago
|
Updated•8 months ago
|
Assignee | ||
Comment 6•8 months ago
|
||
Depends on D198382
Updated•8 months ago
|
Updated•8 months ago
|
Comment 8•8 months ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0fb3738bd07b
https://hg.mozilla.org/mozilla-central/rev/3d1ef5fe232a
Assignee | ||
Comment 9•8 months ago
|
||
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•8 months ago
|
||
(In reply to Kagami [:saschanaz] (they/them) from comment #9)
Created attachment 9373243 [details]
image.pngOh 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.
Assignee | ||
Comment 11•8 months ago
•
|
||
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•8 months ago
|
||
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.
Assignee | ||
Comment 13•8 months ago
|
||
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•8 months ago
|
||
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
Updated•8 months ago
|
Updated•8 months ago
|
Assignee | ||
Comment 15•8 months ago
|
||
Part 1 patch is still in m-c, let's track the other one in bug 1875079.
Description
•