1575766 - High Contrast Mode is applied to DevTools when Toolbox is in a content frame

Assignee

Description

•

5 years ago

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.

Julian Descottes [:jdescottes]

Assignee

Updated

•

5 years ago

Summary: High Contrast theme DevTools is applied to DevTools when Toolbox is in a content frame → High Contrast Mode is applied to DevTools when Toolbox is in a content frame

Julian Descottes [:jdescottes]

Assignee

Comment 1

•

5 years ago

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?)

Flags: needinfo?(emilio)

Emilio Cobos Álvarez (:emilio)

Comment 2

•

5 years ago

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.

Flags: needinfo?(emilio)

Julian Descottes [:jdescottes]

Assignee

Comment 3

•

5 years ago

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..)

Julian Descottes [:jdescottes]

Assignee

Comment 4

•

5 years ago

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?

Flags: needinfo?(emilio)

Emilio Cobos Álvarez (:emilio)

Comment 5

•

5 years ago

(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.

Flags: needinfo?(emilio)

Emilio Cobos Álvarez (:emilio)

Comment 6

•

5 years ago

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

Emilio Cobos Álvarez (:emilio)

Comment 7

•

5 years ago

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...

BugBot [:suhaib / :marco/ :calixte]

Updated

•

5 years ago

Keywords: regression

Julian Descottes [:jdescottes]

Assignee

Comment 8

•

5 years ago

•

Edited

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.

Tim Nguyen :ntim

Comment 9

•

5 years ago

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.

Emilio Cobos Álvarez (:emilio)

Comment 10

•

5 years ago

(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? :(

Julian Descottes [:jdescottes]

Assignee

Comment 11

•

5 years ago

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

Emilio Cobos Álvarez (:emilio)

Comment 12

•

5 years ago

(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.

Julian Descottes [:jdescottes]

Assignee

Comment 13

•

5 years ago

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.

Julian Descottes [:jdescottes]

Assignee

Comment 14

•

5 years ago

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.

Tim Nguyen :ntim

Comment 15

•

5 years ago

(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/“.

Julian Descottes [:jdescottes]

Assignee

Comment 16

•

5 years ago

(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.

Patrick Brosset <:pbro>

Updated

•

5 years ago

Priority: -- → P2

Julian Descottes [:jdescottes]

Assignee

Comment 17

•

5 years ago

Attached file Bug 1575766 - Use chrome preferences for DevTools documents — Details

Julian Descottes [:jdescottes]

Assignee

Comment 18

•

5 years ago

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?

Flags: needinfo?(emilio)

Emilio Cobos Álvarez (:emilio)

Comment 19

•

5 years ago

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?

Flags: needinfo?(emilio)

Julian Descottes [:jdescottes]

Assignee

Comment 20

•

5 years ago

Attached file Bug 1575766 - Set mIsDevToolsDocument on DevTools documents — Details

Julian Descottes [:jdescottes]

Assignee

Comment 21

•

5 years ago

(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!

Emilio Cobos Álvarez (:emilio)

Comment 22

•

5 years ago

(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?

Julian Descottes [:jdescottes]

Assignee

Comment 23

•

5 years ago

(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.

Julian Descottes [:jdescottes]

Assignee

Updated

•

5 years ago

Blocks: 1576980

Phabricator Automation

Updated

•

5 years ago

Attachment #9088427 - Attachment description: Bug 1575766 - Use chrome prefs for DevTools documents → Bug 1575766 - Use chrome preferences for DevTools documents

Pulsebot

Comment 24

•

5 years ago

Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/79d597b5cd7e Set mIsDevToolsDocument on DevTools documents r=smaug https://hg.mozilla.org/integration/autoland/rev/040cb1876f19 Use chrome preferences for DevTools documents r=emilio

Cristina Coroiu [:ccoroiu]

Comment 25

•

5 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 5 years ago

status-firefox70: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 70

BugBot [:suhaib / :marco/ :calixte]

Updated

•

5 years ago

Assignee: nobody → jdescottes

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

status-firefox68: --- → unaffected

status-firefox69: --- → unaffected

status-firefox-esr60: --- → unaffected

status-firefox-esr68: --- → unaffected

Mihai Boldan, Desktop QA [:mboldan]

Updated

•

5 years ago

QA Whiteboard: [qa-70b-p2]

BugBot [:suhaib / :marco/ :calixte]

Updated

•

3 years ago

Has Regression Range: --- → yes

Emilio Cobos Álvarez (:emilio)

Updated

•

1 year ago

Blocks: 1862924

Bug 1575766 - Use chrome preferences for DevTools documents 5 years ago Julian Descottes [:jdescottes] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1575766 - Set mIsDevToolsDocument on DevTools documents 5 years ago Julian Descottes [:jdescottes] 47 bytes, text/x-phabricator-request		Details \| Review