Closed Bug 652548 Opened 11 years ago Closed 10 years ago

Panel text does not auto-contrast with panel background colour making it unreadable

Categories

(Add-on SDK Graveyard :: General, defect, P1)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: david, Assigned: ochameau)

Details

Attachments

(3 files)

This issue is briefly discussed in bug 645814.

Panel native background colour is unpredictable, and so it needs to be auto-defaulted to a contrasting colour to ensure the text is readable. This is not currently happening (tested on OSX - screenshot attached) and so we end up with black text on a dary gray background.

Without this auto-defaulting, I can't think of a reliable way to use a panel without setting the background colour which does ensure the text is readable.
Priority: -- → P1
Target Milestone: --- → 1.0
Assignee: nobody → myk
So this bug is heavily linked to bug 645814. But we will consider this one as more global. bug 645814 is more about reddit panel but main discussion about how to fix this was there.
This patch is going to set correct text color to documents that doesn't set any color cssrule. So it is not going to fix bug 645814 as reddit webpage set an explicit black color to its text.
Assignee: myk → poirot.alex
Attachment #535059 - Flags: review?(myk)
Comment on attachment 535059 [details] [diff] [review]
Insert color CSSrule into panel document that comes from internal panel computed styles

>diff --git a/examples/reddit-panel/lib/main.js b/examples/reddit-panel/lib/main.js

>+let url = "http://www.reddit.com/.mobile?keep_extension=True";
>+
>+url = "data:text/html,<style></style>Foobar";

Remove this debugging code in the patch that lands. :-)


>diff --git a/packages/addon-kit/lib/panel.js b/packages/addon-kit/lib/panel.js
>+      // Retrieve computed text color style in order to apply to to the iframe

Nit: to to -> to


>+      style.textContent = "body {color: " + textColor + ";}";

Nit: add space around the rule, i.e.:

>+      style.textContent = "body { color: " + textColor + "; }";


Alex and I discussed this and agree that it isn't ideal, because it writes a CSS rule into the panel's content, exposing that rule to the content and potentially interfering with the cascade.  But it's our best option at the moment, and it's an important polish fix.

r=myk with debugging code removal
Attachment #535059 - Flags: review?(myk) → review+
(In reply to comment #4)
> >+let url = "http://www.reddit.com/.mobile?keep_extension=True";
> >+
> >+url = "data:text/html,<style></style>Foobar";
> 
> Remove this debugging code in the patch that lands. :-)

Oops, I obviously forgot one git checkout before git diff!

Landed:
https://github.com/mozilla/addon-sdk/commit/690f704f5e11de823e86528ef9bc8e2a08fb41d3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Injecting style elements into document breaks pages that use xul instead of html.
Also I have noticed that style element is injected per panel show, increasing amount of dom elements, I think we should at least remove the one that we injected previously.

Here is a screenshot: http://cl.ly/7Ce1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #6)
> Injecting style elements into document breaks pages that use xul instead of
> html.
> Also I have noticed that style element is injected per panel show,
> increasing amount of dom elements, I think we should at least remove the one
> that we injected previously.
> 
> Here is a screenshot: http://cl.ly/7Ce1

Hmm, indeed.  Irakli, can you file bugs for these two issues?  They're definitely issues, but I don't think they warrant reopening this bug, which is, after all, fixed, even if the fix contains these problems.

(It'd make sense to reopen this bug if its fix were so problematic that we need to back it out to address the problems, but that doesn't seem to be the case here.)
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.