Closed Bug 1404688 Opened 2 years ago Closed 2 years ago

Theming API - Remove text-shadow from text when there is no background-image

Categories

(Firefox :: Theme, defect, P5)

defect

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

The text shadow is only used to make sure text is readable on image backgrounds.

In other situations, the text shadow looks quite odd. See attached screenshot.
Duplicate of this bug: 1404600
Summary: Theming API - Remove text-shadow from text → Theming API - Remove text-shadow from text when there is no background-image
Priority: -- → P5
Assignee: nobody → ntim.bugs
Blocks: 1386004
No longer blocks: themingapi, 1386004
Assignee: ntim.bugs → nobody
Hey shorlander,

I think we need some clarification on this one from UX on how this API should work. Do we want to apply the text-shadow only when there's a background-image? Or do we want to have the API make it so that we can remove the text shadow from every single tab, regardless of whether there's a background-image? Do we want to make it possible to apply a lighter/less visible text-shadow?

I guess the question here is: what function does the text-shadow need to serve, and what do we think Theme API users should be able to override?
Flags: needinfo?(shorlander)
Some notes from talking to Stephen,
- It's really only useful for really busy background images
- Don't need it on flat colored backgrounds
- Not sure we should make it something that developers need to worry about controlling

Based on those notes, we can move forward with only applying the text-shadow when a background-image is supplied.
Flags: needinfo?(shorlander)
Component: WebExtensions: General → WebExtensions: Themes
Assignee: nobody → ntim.bugs
Comment on attachment 8950095 [details]
Bug 1404688 - Make headerURL optional and remove text-shadow when there is no headerURL.

https://reviewboard.mozilla.org/r/219374/#review225078

::: browser/themes/windows/compacttheme.css
(Diff revision 1)
> -  /* Make the menubar text readable on aero glass (copied from browser-aero.css). */
> -  #toolbar-menubar {
> -    text-shadow: 0 0 .5em white, 0 0 .5em white, 0 1px 0 rgba(255,255,255,.4);
> -  }
> -

I removed it because it never gets applied (due to the text-shadow: none !important, which I've removed as well)
Comment on attachment 8950095 [details]
Bug 1404688 - Make headerURL optional and remove text-shadow when there is no headerURL.

https://reviewboard.mozilla.org/r/219374/#review225872
Attachment #8950095 - Flags: review?(jaws) → review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/85e30806ade1
Make headerURL optional and remove text-shadow when there is no headerURL. r=jaws
Backed out changeset 85e30806ade1 (bug 1404688) for Browser chrome failure on browser/base/content/test/general/browser_compacttheme.js. CLOSED TREE

Log of the fail:
https://treeherder.mozilla.org/logviewer.html#?job_id=162207133&repo=autoland&lineNumber=2410

Backout changeset:
https://hg.mozilla.org/integration/autoland/rev/0b7ae7a92e2be896a74f2b728c7fb8362dac8b98

Push that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=85e30806ade11c6b7b7ba8aba07693266f326a85
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/067ee834b07b
Make headerURL optional and remove text-shadow when there is no headerURL. r=jaws
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/58d315ce1f1b
Make headerURL optional and remove text-shadow when there is no headerURL. r=jaws
https://hg.mozilla.org/mozilla-central/rev/58d315ce1f1b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: needinfo?(ntim.bugs)
(In reply to Tim Nguyen :ntim from comment #6)
> Comment on attachment 8950095 [details]
> Bug 1404688 - Make headerURL optional and remove text-shadow when there is
> no headerURL.
> 
> https://reviewboard.mozilla.org/r/219374/#review225078
> 
> ::: browser/themes/windows/compacttheme.css
> (Diff revision 1)
> > -  /* Make the menubar text readable on aero glass (copied from browser-aero.css). */
> > -  #toolbar-menubar {
> > -    text-shadow: 0 0 .5em white, 0 0 .5em white, 0 1px 0 rgba(255,255,255,.4);
> > -  }
> > -
> 
> I removed it because it never gets applied (due to the text-shadow: none
> !important, which I've removed as well)

Which was a bug, right? We do want that text shadow on glass.

Btw, I really don't like big browser/theme/ and toolkit/theme/ patches "sneaking in" via "WebExtensions: Themes". Please stop doing that or at least CC me (not ideal because others may be interested as well).
Component: WebExtensions: Themes → Theme
Flags: needinfo?(ntim.bugs)
Product: Toolkit → Firefox
Target Milestone: mozilla60 → ---
Target Milestone: --- → Firefox 60
(In reply to Dão Gottwald [::dao] from comment #20)
> Which was a bug, right? We do want that text shadow on glass.

I guess so, filed bug 1439789 for it.

> Btw, I really don't like big browser/theme/ and toolkit/theme/ patches
> "sneaking in" via "WebExtensions: Themes".

"WebExtensions: Themes" doesn't seem like an unreasonable choice for this bug to me, the patch touches the WE theme code (LWTConsumer.jsm + ext-theme.js + WE theme tests + extension loading code). 


Some stuff makes sense in "WebExtensions: Themes", I'll CC you if that's the case but you might still want to CC to the relevant meta bugs (themingapi-polish, themingapi-more-ui, ...), so you get notified about newly created child bugs too.
Flags: needinfo?(ntim.bugs)
Depends on: 1439789
(In reply to Tim Nguyen :ntim from comment #21)
> (In reply to Dão Gottwald [::dao] from comment #20)
> > Which was a bug, right? We do want that text shadow on glass.
> 
> I guess so, filed bug 1439789 for it.
> 
> > Btw, I really don't like big browser/theme/ and toolkit/theme/ patches
> > "sneaking in" via "WebExtensions: Themes".
> 
> "WebExtensions: Themes" doesn't seem like an unreasonable choice for this
> bug to me, the patch touches the WE theme code (LWTConsumer.jsm +
> ext-theme.js + WE theme tests + extension loading code). 

Sure it does, but the theme/ parts aren't exactly trivial either. This could have been just two bugs each in their right component: 1. make headerURL optional, 2. remove text-shadow when there is no headerURL. Turns out there was already bug 1413144 for the first part with some relevant discussion.

Like I said this isn't just about me. Components matter because different people watch them. Please make an effort to use them accordingly.
Depends on: 1440069
Keywords: dev-doc-needed
Blocks: 1395105
I've had a go at documenting this. I added a note to the relevant manifest ref page, and the Firefox 60 rel notes:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme#images
https://developer.mozilla.org/en-US/Firefox/Releases/60#WebExtensions

Does this look OK?
Flags: needinfo?(ntim.bugs)
Looks fine, thanks!
Flags: needinfo?(ntim.bugs)
Depends on: 1450975
Depends on: 1452019
You need to log in before you can comment on or make changes to this bug.