Open Bug 1381938 Opened 2 years ago Updated Last month

Switch to non-native theming of widgets for content on Windows

Categories

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

Unspecified
Windows
enhancement

Tracking

()

REOPENED

People

(Reporter: Alex_Gaynor, Assigned: spohl, NeedInfo)

References

(Blocks 6 open bugs)

Details

Attachments

(1 file, 6 obsolete files)

nsNativeThemeWin makes heavy use of the win32k APIs, so this is a blocker for the win32k lockdown effort.

https://docs.google.com/document/d/1d21-ff2mlNlNYX1CW0G9yRNNqSjMkjXRo9-MOXe1krs/edit?ts=596d1880#heading=h.xq1rtm3jj9o1 has a full set of stack traces, but a few example ones:

    win32u!NtUserGetWindowDC
    UxTheme!CImageFile::SelectCorrectImageFile+0x2cc
    UxTheme!CImageFile::GetScaledContentMargins+0x36
    UxTheme!CImageFile::GetBackgroundContentRect+0x36
    UxTheme!GetThemeBackgroundContentRect+0x127
    xul!nsNativeThemeWin::GetWidgetBorder+0x16b
    xul!nsBox::GetXULBorder+0x9d
    xul!nsIFrame::GetXULBorderAndPadding+0x22


    win32u!NtGdiDrawStream
    gdi32full!GdiDrawStream+0x5c
    UxTheme!CImageFile::DrawBackgroundDS+0x289
    UxTheme!CImageFile::DrawImageInfo+0x26c
    UxTheme!CImageFile::DrawBackground+0x66
    UxTheme!DrawThemeBackground+0x23c
    xul!nsNativeThemeWin::DrawWidgetBackground+0xb55
    xul!nsDisplayThemedBackground::PaintInternal+0xb0
    xul!nsDisplayThemedBackground::Paint+0x13
Component: Security: Process Sandboxing → Widget: Win32
Whiteboard: sb+
Blocks: 1151941
Priority: -- → P3
Assignee: nobody → jmathies
Priority: P3 → P1
Attached patch wip (obsolete) — Splinter Review
This is turning out to be a bit tricky, but not impossible on Windows. I'm unsure about GTK and OSX. 

First nsITheme supports a pretty short and well contained liost of apis. A few of these apis are easily remoted without much work. Then there is a subset that rely on various UX queues to make decisions, all of which rely on a presentation context object and an nsIFrame pointer for information. On Windows the use of these two pointers can be factored out.

The last api that presents a large hurdle is nsITheme's DrawThemeBackground. On Windows this api takes a gfxContext which wraps a DrawTarget and surface. This object gets handed to an old wrapper called gfxWindowsNativeDrawing which facilitates painting to the target, and in some cases facilitates alpha blending using alpha extraction.

Pretty sure we can create a fresh gfxContext + gfxWindowsNativeDrawing which interacts with a DrawTarget that is backed by a shared memory segment of some sort. Not sure if something like this exists though.. need to dig a bit more.

Not sure if I want to pursue this on all platforms.. we still have non-native theming in the works. TBD as I do more research.
Duplicate of this bug: 1411425
Whiteboard: sb+ → [fingerprinting][sb+]
Blocks: 1411425
Priority: P1 → P2
Whiteboard: [fingerprinting][sb+] → [fingerprinting][sb+][overhead:noted]
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → WONTFIX
Reopening this for win32 theming work.
Assignee: jmathies → nobody
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Move nsNativeThemeWin calls out of the content process → Switch to css based theming of widgets for content
Summary: Switch to css based theming of widgets for content → Switch to css based theming of widgets for content on Windows
So, uhm, is the intention here to stop supporting platform-themed form controls?
This seems like a rather fundamental change that should be discussed on dev.platform
but I can't find any posts about it there.
Flags: needinfo?(jmathies)
(In reply to Mats Palmgren (:mats) from comment #5)
> So, uhm, is the intention here to stop supporting platform-themed form
> controls?
> This seems like a rather fundamental change that should be discussed on
> dev.platform
> but I can't find any posts about it there.

Native theming is already a mess on windows. Check out https://developer.microsoft.com/en-us/microsoft-edge/tools/screenshots/?url=http%3A%2F%2Fnativeformelements.com%2F?

We would still be using platformed-themed controls for Chrome.

What about this change worries you?
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #5)
> So, uhm, is the intention here to stop supporting platform-themed form
> controls?
> This seems like a rather fundamental change that should be discussed on
> dev.platform
> but I can't find any posts about it there.

We discussed this with Product but haven't posted on dev.plat. Our motivation here is security related: turning on os level security features on Windows which protects against kernel exploits. Due to this implementing these changes isn't really open for discussion.

That said understand your concern, I'll make sure the dev who works on this gives dev.platform a heads up seeking feedback.
Flags: needinfo?(jmathies)
> Native theming is already a mess on windows.

I don't see any "mess" there.  What are you referring to?

> We would still be using platformed-themed controls for Chrome.

Well, that's even worse.  Making buttons look different in "UI"
and web pages is... yikes.

> What about this change worries you?

See bug 1411425 comment 36, and dbaron's comments in bug 997190.

And even /if/ it can be implemented correctly, I consider platform-themed
form controls a feature, not a bug.  It certainly needs to be widely
discussed before a feature like this is removed.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #8)
> > Native theming is already a mess on windows.
> 
> I don't see any "mess" there.  What are you referring to?

All of the browsers draw platformed-themed controls differently. Even Microsoft is not consistent between Edge and IE 11.

As I understand it, we're the only browser on Windows 10 that's using the DrawThemeBackground API to draw our controls. I believe all of the other browsers just draw controls the same way as any other element. Continuing to use DrawThemeBackground as is, is not possible because it's not allowed when we remove win32k access from the content process.

If we did want to use DrawThemeBackground we'd need to come up with some kind of remoting strategy.

Do you have a suggestion for an alternative way to proceed here?
Flags: needinfo?(mats)
Fetching theme values from a remote process that has win32 access and
caching the values in the content process could work.

Alternatively, storing all the values we need upfront in the content
process and then dropping access to win32 APIs before any untrusted
content is processed might work - depending on what sandboxing
techniques you plan to use.  (like OpenBSD's "pledge" model)
Flags: needinfo?(mats)
win32k dropping is enabled at process creation time, so it'd need to be fetched it a remote process and passed down, not at startup.
(In reply to Mats Palmgren (:mats) from comment #10)
> Fetching theme values from a remote process that has win32 access and
> caching the values in the content process could work.

There aren't theme values. The Windows API that we're using (DrawThemeBackground) is more like draw a widget of this type and this size to this DC. Using a sync IPC to do this would make our already bad widget drawing performance much worse.
Flags: needinfo?(mats)
I'm just spitballing here, but Android's NinePatch format comes to mind.
(For the unfamiliar, see https://developer.android.com/guide/topics/graphics/drawables#nine-patch)

My thought here is that in the chrome process we could use DrawThemeBackground to create and cache a bunch of small images that we then interpret as "NinePatch" images that can be stretched to fit the widgets as appropriate.
(Of course, I suppose the tricky part is figuring out which parts of the cached image are stretchable, but hopefully we could pull enough attributes via the Theming API that we could possibly compute that)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #12)

Well, I'm not an expert on Win32 so you shouldn't ask me for solutions.
I'm just saying that I find it questionable to remove features that
affect how web content looks without a wider discussion first.
(I still haven't seen any post to dev.platform, BTW.)
Flags: needinfo?(mats)
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #11)
> win32k dropping is enabled at process creation time, so it'd need to be
> fetched it a remote process and passed down, not at startup.

I'm pretty sure it can be enabled after start-up.
Slightly irrelevant because as jrmuizel points out, without some caching plus sizing scheme we need to call the actual API.

I also think mats is right in that we should post to dev.platform about this.
Whiteboard: [fingerprinting][sb+][overhead:noted] → [fingerprinting][sb+][overhead:noted][fp-triaged]
Assignee: nobody → spohl.mozilla.bugs
Attachment #9049340 - Attachment is obsolete: true
No longer blocks: 1411425
Whiteboard: [fingerprinting][sb+][overhead:noted][fp-triaged]
Priority: P2 → P1
No longer blocks: 1411425
Attachment #9049348 - Attachment is obsolete: true
Blocks: 1381022

I don't know how you plan to use content-common.css, but if this is going to be used in forms.css, it's going to create web compat issues, since we're changing the default UA styles of HTML elements. Also, using this in the Firefox UI will likely cause regressions to specificity issues (this is fixable, but the first issue isn't).

Can't we use Chrome's approach of defining their own "native" widget appearance on Windows? So instead of making -moz-appearance: button render the native Windows button appearance, it would paint an image that we would create ourselves as the background. This avoids the need of having to refactor CSS and having to deal with web compat problems.

I don't know much about this code, but I guess that would mean rewriting widget/windows/nsNativeThemeWin.cpp to get rid of the native theme calls.

Or if we still want to keep native theme calls for the parent process, we could have a brand new widget/nsBasicTheme.cpp and make nsPresContext::GetTheme() return the theme conditionally from widget/windows/nsNativeThemeWin.cpp or widget/nsBasicTheme.cpp.

(In reply to Tim Nguyen :ntim from comment #21)

I don't know how you plan to use content-common.css, but if this is going to be used in forms.css, it's going to create web compat issues, since we're changing the default UA styles of HTML elements. Also, using this in the Firefox UI will likely cause regressions to specificity issues (this is fixable, but the first issue isn't).

The current idea isn't so much to use content-common.css in forms.css, but rather in addition to forms.css, with the rules in content-common.css taking precedence over forms.css.

In terms of web compat, can you give specific examples?

content-common.css uses a lot of the same CSS that is used by the Firefox UI, but is not itself expected to be used by the Firefox UI. This may get refactored at some point and consolidated, but we are not trying to do this here.

Flags: needinfo?(ntim.bugs)

In terms of web compat, can you give specific examples?

Mats mentioned the concerns in comment 8, it doesn't sound like the scope of the current patch will let us avoid those.

If I understand correctly the current patches, basically every property that is set by content-common.css but not set by forms.css will basically cause compat issues: one example is the box-shadow/outline on focused inputs, buttons, which is something websites will now have to override with box-shadow: none if they define their own input border/background styles.

That's just one of many issues that could arise from changing the web content default UA CSS.

Since content-common.css would not be able to set more properties than forms.css, then it sort of makes content-common.css useless, since forms.css styles would take precedence over content-common.css all the time, meaning that just changing the different -moz-appearance values to none in forms.css would have the same effect as this patch.

I think a more safe approach here is to implement a new fake "native" theme for -moz-appearance: button, textfield, etc. that doesn't involve any platform API calls using C++, so the default UA styles don't need to change at all. That's basically what Blink is doing at the moment on Windows.

It would also give you more flexibility in regards of what default appearance you can apply, which is beneficial if you need to implement extra default styling for accessibility.

Flags: needinfo?(ntim.bugs)

(In reply to Tim Nguyen :ntim from comment #25)

I think a more safe approach here is to implement a new fake "native" theme for -moz-appearance: button, textfield, etc. that doesn't involve any platform API calls using C++, so the default UA styles don't need to change at all.

I agree, Tim's suggestion is the better approach.

The corresponding Android bug for using C++ theming instead of CSS overrides is bug 958999.

(In reply to Markus Stange [:mstange] from comment #26)

(In reply to Tim Nguyen :ntim from comment #25)

I think a more safe approach here is to implement a new fake "native" theme for -moz-appearance: button, textfield, etc. that doesn't involve any platform API calls using C++, so the default UA styles don't need to change at all.

I agree, Tim's suggestion is the better approach.

To confirm my understanding: You both prefer an approach along the lines of what was mentioned in comment 13 and comment 14 due to the nature of the API as explained in comment 12, correct? The difficulty here has always been that the Windows API isn't just returning a set of values.

What if we only changed the properties that are currently set by forms.css? This would restyle the "Moz Unstyled Inputs" on the left side of http://stephenhorlander.com/form-controls.html. This wouldn't allow us to mirror the existing UX on our about: pages as we intended, but it may be an acceptable compromise. This would require some exploration to see how far we can take our styling and whether or not the result is acceptable.

Flags: needinfo?(ntim.bugs)
Flags: needinfo?(mstange)

(In reply to Stephen A Pohl [:spohl] from comment #28)

(In reply to Markus Stange [:mstange] from comment #26)

(In reply to Tim Nguyen :ntim from comment #25)

I think a more safe approach here is to implement a new fake "native" theme for -moz-appearance: button, textfield, etc. that doesn't involve any platform API calls using C++, so the default UA styles don't need to change at all.

I agree, Tim's suggestion is the better approach.

To confirm my understanding: You both prefer an approach along the lines of what was mentioned in comment 13 and comment 14 due to the nature of the API as explained in comment 12, correct? The difficulty here has always been that the Windows API isn't just returning a set of values.

No, I was suggesting more simple than comment 13 and comment 14, we could create a basic C++ stub theme that doesn't look native at all. This is what Google Chrome is doing.

Flags: needinfo?(ntim.bugs)
Attachment #9053311 - Attachment is obsolete: true
Attachment #9053310 - Attachment is obsolete: true

Changing bug title to more accurately reflect the current approach.

Summary: Switch to css based theming of widgets for content on Windows → Switch to non-native theming of widgets for content on Windows
Attachment #8977091 - Attachment is obsolete: true
Attachment #9069019 - Attachment description: Bug 1381938: Add native theme for Windows that avoids native system calls. → Bug 1381938: Add native theme for Windows that avoids native system calls. r=mstange
Attachment #9102612 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.