970404 - browser.display.use_document_colors;false makes start and end of the active tab colored with browser.display.background_color

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[[:Aleksej]]

Attachment: screenshot of the problem with 2014-02-10-03-02-01-mozilla-central-firefox-30.0a1.en-US.linux-x86_64 (background set to green, default was plain white)

2014-02-10-03-02-01-mozilla-central-firefox-30.0a1.en-US.linux-x86_64

Set browser.display.use_document_colors to "false" (uncheck Preferences → Content → Colors → [ ] Allow pages to choose their own colors…).

Expected results:
No effect on tab colors, or no inconsistencies introduced.

Actual results:
Start and end of the active tab are colored with the background color specified in browser.display.background_color (Preferences → Content → Colors → Background).

It's closely related to, or maybe is another side of, bug 943195.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

It seems weird to me that we apply this pref to browser chrome at all. Shouldn't this be an a11y feature for content only?

Comment 2

[:Gijs (out until Jan 5th; he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #1)
> It seems weird to me that we apply this pref to browser chrome at all.
> Shouldn't this be an a11y feature for content only?

So this is because we're using an SVG file for the sides of the tabs, and so these prefs are applying to the colors inside the SVG file. Ideally, there should be some way to opt out of this and/or claim "These are chrome SVG bits, don't honor this pref". From looking at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#689, it seems that the only current way to deal with that is for it to be a chrome docshell. Boris, do you know if we can change this for an SVG file included into a chrome document?

We're including the SVG as such (tabs.inc.css):

  background-image: url(chrome://browser/skin/tabbrowser/tab-selected-start.svg);

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, that code is just broken for SVG resource docs.

You probably want something more like:

  nsIDocument* doc = mDocument->GetDisplayDocument();
  if (!doc) {
    doc = mDocument;
  }

  nsIDocShellTreeItem* docShell = doc->GetDocShell();

Comment 4

[:Gijs (out until Jan 5th; he/him)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #3)
> Yeah, that code is just broken for SVG resource docs.
> 
> You probably want something more like:
> 
>   nsIDocument* doc = mDocument->GetDisplayDocument();
>   if (!doc) {
>     doc = mDocument;
>   }
> 
>   nsIDocShellTreeItem* docShell = doc->GetDocShell();

So this didn't help because there's no display document, as this isn't a resource document but an image.


Boris, you asked: "So the real question is how to characterize the cases that the pref should not affect".

I think the current check (ie chrome documents) /and/ chrome:// URI'd svg files should not be affected.

I don't think there is a case where we want chrome:// URI'd SVG colors overridden by the user color prefs, even when used in-content. Practically, in Firefox, I don't think there are any such svg files anyway, but also theoretically, it seems to me that that would be surprising. Additionally it'd basically be unfixable from the chrome side, whereas the user has options to still override such styles (such as user stylesheets, extensions, themes, etc.).

Boris, does that make sense?

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  So maybe mDocument->IsBeingUsedAsImage() && IsChromeURI(mDocument->GetDocumentURI()) ?  I'm assuming these images are not in a content package, so we can't just check for system principal.  If we can, we should do that instead of IsChromeURI().

Comment 6

[:Gijs (out until Jan 5th; he/him)]

Attachment: don't let users affect document colors for chrome images,

I had to change the UseDocumentColors getter as well... in fact, it's possible I could leave the other method alone? Not entirely sure. Does this look OK, Boris, or did I mess something up without realizing? :-)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8387360 [details] [diff] [review]
don't let users affect document colors for chrome images,

Looks good as far as I can tell.  r=me

Comment 8

[:Gijs (out until Jan 5th; he/him)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8387360 [details] [diff] [review]
> don't let users affect document colors for chrome images,
> 
> Looks good as far as I can tell.  r=me

Alright! Ship it:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/952d40249eea


(with some trailing whitespace + indentation fixed)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/952d40249eea

Comment 10

[:Gijs (out until Jan 5th; he/him)]

Comment on attachment 8387360 [details] [diff] [review]
don't let users affect document colors for chrome images,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: users that override web colors will have a broken-looking tabstrip
Testing completed (on m-c, etc.): on m-c, local
Risk to taking this patch (and alternatives if risky): reasonably low. Boris, would you concur?
String or IDL/UUID changes made by this patch: none

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, I think risk here is pretty low.

Comment 12

[Mike Conley (:mconley) (:⚙️) (PTO Dec 22 - Jan 2)]

The changeset in this bug didn't apply cleanly when I tried to transplant on mozilla-aurora.

I would have done it by hand, but then I saw what code it was touching. That's a big nope right there.

Gijs, would you mind writing a patch for mozilla-aurora?

Comment 13

[:Gijs (out until Jan 5th; he/him)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/2749817b69b8