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

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P5)

Description

[Tim Nguyen :ntim]

Attachment: Screen Shot 2017-10-01 at 10.32.42 AM.png

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.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 3

[Tom S [:evilpie]]

Just saw this on reddit: https://www.reddit.com/r/firefox/comments/7t6hmx/extensions_in_firefox_59/dtafuci/. Any update on this?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 5

[Tim Nguyen :ntim]

Attachment: Bug 1404688 - Make headerURL optional and remove text-shadow when there is no headerURL.

Review commit: https://reviewboard.mozilla.org/r/219374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219374/

Comment 6

[Tim Nguyen :ntim]

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 7

[Tim Nguyen :ntim]

Comment on attachment 8950095 [details]
Bug 1404688 - Make headerURL optional and remove text-shadow when there is no headerURL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219374/diff/1-2/

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85e30806ade1
Make headerURL optional and remove text-shadow when there is no headerURL. r=jaws

Comment 10

[Dorel Luca [:dluca]]

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

Comment 11

[Cristina Coroiu [:ccoroiu]]

Also failed the screenshots jobs.

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162213826&repo=autoland&lineNumber=2390

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8950095 [details]
Bug 1404688 - Make headerURL optional and remove text-shadow when there is no headerURL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219374/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/067ee834b07b
Make headerURL optional and remove text-shadow when there is no headerURL. r=jaws

Comment 14

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 067ee834b07b (bug 1404688) for xpcshell failures at toolkit/mozapps/extensions/test/xpcshell/test_LightweightThemeManager.js

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=067ee834b07b32bc0fd3023ef510cbea6dad1aa2&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162322931&repo=autoland&lineNumber=2747

Backout: https://hg.mozilla.org/integration/autoland/rev/8c6dc94629fc116689e5499424319403307d6e3a

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8950095 [details]
Bug 1404688 - Make headerURL optional and remove text-shadow when there is no headerURL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219374/diff/3-4/

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8950095 [details]
Bug 1404688 - Make headerURL optional and remove text-shadow when there is no headerURL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219374/diff/4-5/

Comment 17

[Tim Nguyen :ntim]

Comment on attachment 8950095 [details]
Bug 1404688 - Make headerURL optional and remove text-shadow when there is no headerURL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219374/diff/5-6/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58d315ce1f1b
Make headerURL optional and remove text-shadow when there is no headerURL. r=jaws

Comment 19

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/58d315ce1f1b

Comment 20

[Dão Gottwald [:dao]]

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

Comment 21

[Tim Nguyen :ntim]

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

Comment 22

[Dão Gottwald [:dao]]

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

Comment 23

[Chris Mills [:cmills]]

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?

Comment 24

[Tim Nguyen :ntim]

Looks fine, thanks!