STRs:

• enable High Contrast Mode (if not on Windows, follow instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/CSS_Guidelines#High_contrast_mode or simply set browser.display.document_color_use to 2)
• check that devtools.toolbox.content-frame is true in about:config
• open DevTools

The Toolbox will be rendered in High Contrast Mode (HCM).
This is an unintended side effect of using a frame with type = content instead of Chrome. I guess ideally DevTools would support HCM (see bug 1022557) however in its current state the toolbox can be hard to use. Images are missing so you don't see the toggle icons, some selection states are not clearly reflected etc...

I think the logic driving this is at https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/servo/components/style/gecko/media_queries.rs#273-277

I think in a first step, we should try to add an exception to the code linked above, since this behavior change was unintended.

I also need to test the change on the real Windows High Contrast mode. Only tested with browser.display.document_color_use set to 2 for now.

Same symptoms with Windows High Contrast Modes, as long as browser.display.document_color_use is set to 0 (high contrast only) or 2 (always override).

Emilio, do you know if we can add an exception for the devtools toolbox at https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/servo/components/style/gecko/media_queries.rs#273-277 ?

    /// Returns whether document colors are enabled.
    #[inline]
    pub fn use_document_colors(&self) -> bool {
        let doc = self.document();
        if doc.mIsBeingUsedAsImage() {
            return true;
        }
        let document_color_use = static_prefs::pref!("browser.display.document_color_use");
        let prefs = self.pref_sheet_prefs();
        match document_color_use {
            1 => true,
            2 => prefs.mIsChrome,
            _ => !prefs.mUseAccessibilityTheme,
        }
    }

The toolbox frame used to be a chrome frame so, mUseAccessibilityTheme was always false and mIsChrome was always true. Maybe have an additional chrome privileged only attribute? (although I guess we would have to add it to all our iframes?)

So you effectively are entering into the content path now. I assume the document is still a chrome:// document URI, but it isn't loaded in a chrome docshell?

It may be worth changing this line to be something like:

return aDoc.IsInChromeDocShell() || aDoc.IsDocumentURISchemeChrome();

That'd fix it, I guess, and I don't see why chrome:// URIs should behave differently depending on where they're loaded.

Thanks Emilio, I'll try your suggestion!

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

So you effectively are entering into the content path now. I assume the document is still a chrome:// document URI, but it isn't loaded in a chrome docshell?

Yes, exactly. Our URL is still chrome://devtools/content/framework/toolbox.xul (and we can still use chrome privileged APIs etc..)

Even though the document is at a chrome:// , the src attribute is set to "about:devtools-toolbox", which is handled by the following component https://searchfox.org/mozilla-central/source/devtools/startup/AboutDevToolsToolboxRegistration.jsm

So with the suggested fix, the toolbox iframe still has high contrast colors applied, but the inner devtools frames don't (because the src attributes for the panels are directly pointing to chrome:// resources).

In ShouldUseChromePrefs I could add a devtools toolbox specific check such as

  nsIURI* uri = aDoc.GetDocBaseURI();
  if (uri && uri->SchemeIs("about")) {
    if (uri->GetSpecOrDefault().EqualsLiteral("about:devtools-toolbox")) {
      return true;
    }
  }

Or go back to the idea of adding a new attribute on the frame. What do you think?

(In reply to Julian Descottes [:jdescottes] from comment #4)

Even though the document is at a chrome:// , the src attribute is set to "about:devtools-toolbox", which is handled by the following component https://searchfox.org/mozilla-central/source/devtools/startup/AboutDevToolsToolboxRegistration.jsm

I see, so document.documentURI is about:devtools-toolbox. That's unfortunate.

Do you know why we have an about page for this at all, out of curiosity? Why can't it just point to the chrome:// URI?

In ShouldUseChromePrefs I could add a devtools toolbox specific check such as

  nsIURI* uri = aDoc.GetDocBaseURI();
  if (uri && uri->SchemeIs("about")) {
    if (uri->GetSpecOrDefault().EqualsLiteral("about:devtools-toolbox")) {
      return true;
    }
  }

The right check should be using GetDocumentURI, otherwise any document with <base href="about:devtools-toolbox"> would automatically not get it. But special-casing URIs there is a bit ugly... Anyhow, special-casing the URI may be ok. I'd like it done in a different way, I guess, so that we don't check the URI all the time, but I'm happy to write that patch if we determine it is the right way to go about this.

Or go back to the idea of adding a new attribute on the frame. What do you think?

I think I'm missing some context. So we want to load the toolbox with type="content", all good there. But are there other about pages for which we do the same already, right? If so, don't they have this issue too? That is, why would about:devtools-toolbox be special?

How would the frame attribute be checked? What would be its name and its semantics? Would it work for stuff in a different process?

Basically I'm not sure what the right check is, but this does not only affect high contrast, it also affects other default colors or what not.

Also there's another code in layout that check IsInChromeDocShell() nsPresContext::IsChrome() in particular.

Anyhow, I think special-casing it is not too terrible, but I'd like to come up with a proper reason for it.

I just wrote bug 1575806, which removes the last caller to IsDocumentURISchemeChrome(), so a preferred way to do this would be to rename that bit to DocumentURINeedsChromeSheetPreferences() or something of that sort, and set that bit to true when the document URI is about:devtools-toolbox too...

Thanks for looking at this Emilio!

Do you know why we have an about page for this at all, out of curiosity? Why can't it just point to the chrome:// URI?

It was initially added in Bug 1233463, but I don't think the initial motivations still apply today.
We also use the about:devtools-toolbox URL to open the toolbox in a tab (from about:debugging mostly). Here it is exposed to users via the address bar so it's better to have a clean "about:" URL here.
We have code that assumes that the toolbox URL is about:devtools-toolbox, it will have to be updated a bit to handle both URLs transparently.

are there other about pages for which we do the same already [...] why would about:devtools-toolbox be special

I think about:devtools-toolbox is the only about: page loaded in a frame inside browser.xul. Other about: pages are normally loaded in a tab. The regression here is only about the DevTools regular toolbox.

How would the frame attribute be checked? What would be its name and its semantics? Would it work for stuff in a different process?

To be defined, we could have something such as useChromeSheetPreferences, make it chrome privileged only, parent process only. But I don't know if the plumbing necessary to get this to work is worth it.

Anyhow, I think special-casing it is not too terrible, but I'd like to come up with a proper reason for it.

If I have to give a reason: special casing about:devtools-toolbox would be the least impacting change for us right now, and we could think about changing the URL of the toolbox in a follow up.

We want the content H-C adjustments to be enabled for other content-loaded chrome:// URI pages (eg. about:preferences, about:addons), since they use non-system colors and they also cover the whole tab content area, which means they should just be adapted like web-content is... More importantly, we've always been aware of those adjustments affecting the content area so the CSS has always been optimized for them.

DevTools is a bit special here, since it used to be part of the chrome, meaning the colors always appeared as authored, so the CSS was never really optimized for those adjustments. I don't think updating the DevTools CSS is a high-priority for now, and it would be a non-trivial effort, so I'd just disable the adjustments there.

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

We want the content H-C adjustments to be enabled for other content-loaded chrome:// URI pages (eg. about:preferences, about:addons), since they use non-system colors and they also cover the whole tab content area, which means they should just be adapted like web-content is... More importantly, we've always been aware of those adjustments affecting the content area so the CSS has always been optimized for them.

Well, then that's harder, because we can't just disable them for al chrome:// uris wholesale. Do we need a set of URIs to whitelist? :(

I am not sure I follow, why would the other about: pages be impacted if we add an exception only for about:devtools-toolbox?

(In reply to Julian Descottes [:jdescottes] from comment #11)

I am not sure I follow, why would the other about: pages be impacted if we add an exception only for about:devtools-toolbox?

They wouldn't, but then the about:devtools-toolbox subframes would still have H-C adjustments... Or otherwise we'd break other chrome:// pages that per commment 9 expect high-contrast adjustments.

But those pages are loaded via about:preferences, about:addons so they would not be picked up by aDoc.IsDocumentURISchemeChrome();.
With my current local patch that uses

bool PreferenceSheet::ShouldUseChromePrefs(const Document& aDoc) {
  nsIURI* uri = aDoc.GetDocBaseURI();
  if (uri && uri->SchemeIs("about")) {
    if (uri->GetSpecOrDefault().EqualsLiteral("about:devtools-toolbox")) {
      return true;
    }
  }

  return aDoc.IsInChromeDocShell() || aDoc.IsDocumentURISchemeChrome();
}

about:preferences, about:addons etc... still apply the high contrast adjustments.

Sorry I was wrong, I see about:addons also loads content with chrome:// URLs inside the page. So indeed it regresses part of their UI when enabling High Contrast mode.

(In reply to Julian Descottes [:jdescottes] from comment #14)

Sorry I was wrong, I see about:addons also loads content with chrome:// URLs inside the page. So indeed it regresses part of their UI when enabling High Contrast mode.

I think the problem Emilio is mentioning is more about the about:devtools-toolbox subframes (devtools/client/*/index.html) having the H-C adjustments when they shouldn’t. If your local patch covers that without breaking other things, then I think it should be ok. If not, maybe you could also whitelist pages starting with “chrome://devtools/“.

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

(In reply to Julian Descottes [:jdescottes] from comment #14)

Sorry I was wrong, I see about:addons also loads content with chrome:// URLs inside the page. So indeed it regresses part of their UI when enabling High Contrast mode.

I think the problem Emilio is mentioning is more about the about:devtools-toolbox subframes (devtools/client/*/index.html) having the H-C adjustments when they shouldn’t. If your local patch covers that without breaking other things, then I think it should be ok. If not, maybe you could also whitelist pages starting with “chrome://devtools/“.

My patch removed the aDoc.IsBeingUsedAsImage() check as we discussed earlier in the thread, so that took care of the DevTools subframes. But it also regressed the about:addons subframes :)
We need either a whitelist or another approach.

Emilio, I tried a mixed approach in https://phabricator.services.mozilla.com/D43614 : set a IsDevToolsDocument flag on the document if URI is about:devtools-toolbox or if the parentDocument already has IsDevToolsDocument set to true.

Do you think this would be a reasonable solution?

Is this devtools page ever going to have out-of-process iframes? Or unknown iframes? If so that doesn't quite cut it.

Otherwise I guess this works, though we should aim to remove the special-case here... Does devtools plan to help eventually move to the same model as the other in-content about pages?

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

Is this devtools page ever going to have out-of-process iframes? Or unknown iframes? If so that doesn't quite cut it.

I don't think so.

Otherwise I guess this works, though we should aim to remove the special-case here... Does devtools plan to help eventually move to the same model as the other in-content about pages?

I think we should work on making DevTools look good with High Contrast mode, and after that we could remove the workaround here. I will file a follow up!

(In reply to Julian Descottes [:jdescottes] from comment #21)

I think we should work on making DevTools look good with High Contrast mode, and after that we could remove the workaround here. I will file a follow up!

Great! Would it be worth to put this workaround behind a pref then?

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

Great! Would it be worth to put this workaround behind a pref then?

Good point, otherwise it would be hard to work on improving DevTools' High Contrast mode support.

https://hg.mozilla.org/mozilla-central/rev/79d597b5cd7e
https://hg.mozilla.org/mozilla-central/rev/040cb1876f19