Closed Bug 1311352 Opened 9 years ago Closed 9 years ago

The theme API should be able to support as much of the Developer Edition as possible

Categories

(WebExtensions :: General, defect)

Product:
WebExtensions
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: triaged)

Attachments

(3 files)

Patch 0: convert tabCurveWidth and tabCurveHalfWidth into CSS variables
5.35 KB, patch
Details | Diff | Splinter Review
Patch: WIP dev edition port
27.00 KB, patch
jaws
: feedback+
Details | Diff | Splinter Review
Bug 1311352 - port the Developer Edition theme to a WebExtension format.
58 bytes, text/x-review-board-request
jaws
: review+
Details
No description provided.
Depends on: 1310144
Attachment #8802711 - Flags: feedback?(jaws)
Would be totally cool if you could still take a look, Jared! I'm continuing on it tomorrow.
(In reply to Mike de Boer [:mikedeboer] from comment #3) > Would be totally cool if you could still take a look, Jared! I'm continuing > on it tomorrow. Owait, Friday that is.
Comment on attachment 8802711 [details] [diff] [review] Patch: WIP dev edition port Review of attachment 8802711 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/ext-theme.js @@ +44,5 @@ > + ["toolbar_navbar_highlight_overlap", ["--navbar-tab-toolbar-highlight-overlap", ":root"]], > + ["toolbar_text_shadow", ["--toolbarbutton-text-shadow", ":root"]] > +]); > + > +const kBrowserOverrideColorsVarMap = new Map([ I get this, but it's thoroughly complex at first sight. Can you put a comment above this explaining how keys map to values and what each item in the corresponding array should match to? @@ +93,1 @@ > const kTransparentGif = "data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7"; It looks like this is unused now. We should remove it. @@ +274,4 @@ > } > > + let dataURL = `data:text/css,${browserStyles.join("")}`; > + dump("URL:: " + dataURL + "\n"); We shouldn't check in the dump() ::: browser/components/extensions/test/browser/browser_ext_theme_devEdition.js @@ +65,5 @@ > + > + // Pick a button to test colors. > + let button = document.getElementById("PanelUI-menu-button"); > + inIDOMUtils.addPseudoClassLock(button, ":hover"); > + style = window.getComputedStyle(button); Oh, this is cool, nice find.
Attachment #8802711 - Flags: feedback?(jaws) → feedback+
Blocks: 1313325
Comment on attachment 8805066 [details] Bug 1311352 - port the Developer Edition theme to a WebExtension format. https://reviewboard.mozilla.org/r/88942/#review88578 ::: browser/components/extensions/ext-theme.js:17 (Diff revision 4) > .getService(Components.interfaces.nsIIOService); > > -const kChromeThemeColorVarMap = new Map([ > - ["background_tab", ["--tab-background-color", ":root"]], > +// For square tabs and separate back & forward buttons, only the Mac-specific > +// styling has been implemented. Please, feel free to add more platforms and > +// [platform]/devedition.css browser style overrides as you see fit. > +const kIsMac = (AppConstants.platform == "macosx"); no parentheses necessary here. ::: browser/themes/shared/devedition.inc.css (Diff revision 4) > -.tab-background, > -.tabs-newtab-button { > - margin-inline-end: 0; > - margin-inline-start: 0; How does the removal of these lines and the other lines in this file affect Windows and Linux? It seems this patch would work fine on OSX but leave the theme broken for Windows and Linux since it's missing these changes. ::: toolkit/modules/LightweightThemeConsumer.jsm:126 (Diff revision 4) > + root.setAttribute("devtoolstheme", luminance <= 110 ? "dark" : "light"); > root.setAttribute("lwtheme", "true"); > } else { > root.removeAttribute("lwthemetextcolor"); > + root.removeAttribute("devtoolstheme"); Why should we always have [devtoolstheme] set if a lwtheme is applied?
Comment on attachment 8805066 [details] Bug 1311352 - port the Developer Edition theme to a WebExtension format. https://reviewboard.mozilla.org/r/88942/#review89034 Please re-request review after addressing the questions in my previous pass.
Attachment #8805066 - Flags: review?(jaws) → review-
Comment on attachment 8805066 [details] Bug 1311352 - port the Developer Edition theme to a WebExtension format. https://reviewboard.mozilla.org/r/88942/#review89630 ::: browser/themes/shared/devedition.inc.css (Diff revision 4) > -.tab-background, > -.tabs-newtab-button { > - margin-inline-end: 0; > - margin-inline-start: 0; I don't think I understand what you mean... this adds variable tab curve width support to all OS themes, because the changes are made in the shared files. There's no need to adjust things for other OSes, because they will surely apply there. ::: toolkit/modules/LightweightThemeConsumer.jsm:126 (Diff revision 4) > + root.setAttribute("devtoolstheme", luminance <= 110 ? "dark" : "light"); > root.setAttribute("lwtheme", "true"); > } else { > root.removeAttribute("lwthemetextcolor"); > + root.removeAttribute("devtoolstheme"); Yeah, that was because of some hardcoded stuff in the browser theme to get some color stuff sorted out. I'll see if I can make swap this over to use 'lwthemetestcolor' as well instead of this hack.
Comment on attachment 8805066 [details] Bug 1311352 - port the Developer Edition theme to a WebExtension format. https://reviewboard.mozilla.org/r/88942/#review90142 Looks good!
Attachment #8805066 - Flags: review?(jaws) → review+
https://hg.mozilla.org/projects/cedar/rev/ba6a7158ef89e9ed080398f0638fea585d085afa Bug 1311352 - port the Developer Edition theme to a WebExtension format. r=jaws
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: