Make sidebar and sidebar_text properties apply to the sidebar header

RESOLVED FIXED in Firefox 63

Status

()

defect
P3
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox61 unaffected, firefox62 unaffected, firefox63- fixed)

Details

(Whiteboard: [ntim-intern-project])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

8 months ago
Bug 1418602 only makes them apply to the bookmarks and history trees.
(Assignee)

Updated

8 months ago
Blocks: 1385518
(Assignee)

Comment 1

8 months ago
Attachment #9003445 - Flags: review?(dao+bmo)
(Assignee)

Updated

8 months ago
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 3

8 months ago
Attachment #9003449 - Flags: review?(dao+bmo)
(Assignee)

Updated

8 months ago
Attachment #9003445 - Attachment is obsolete: true
Attachment #9003445 - Flags: review?(dao+bmo)
(Assignee)

Updated

8 months ago
Attachment #9003446 - Attachment is obsolete: true
Attachment #9003446 - Flags: review?(dao+bmo)
(Assignee)

Updated

8 months ago
Component: Themes → Theme
Product: WebExtensions → Firefox
Whiteboard: [ntim-intern-project]
Priority: -- → P3
Comment on attachment 9003449 [details] [diff] [review]
Consolidate browser sidebar styling

>+#browser {
>+  --sidebar-border-color: ThreeDShadow;

Unless this is supposed to ever change at runtime, better use a preprocessor directive instead of a CSS variable.
Attachment #9003449 - Flags: review?(dao+bmo)
(Assignee)

Comment 6

8 months ago
(In reply to Dão Gottwald [::dao] from comment #5)
> Comment on attachment 9003449 [details] [diff] [review]
> Consolidate browser sidebar styling
> 
> >+#browser {
> >+  --sidebar-border-color: ThreeDShadow;
> 
> Unless this is supposed to ever change at runtime, better use a preprocessor
> directive instead of a CSS variable.

I'm planning to allow changing the sidebar border in a different bug.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 7

8 months ago
Bug 1418602 is incomplete without this bit.
Flags: needinfo?(dao+bmo)
Attachment #9003449 - Flags: review+
Comment on attachment 9003451 [details] [diff] [review]
Make sidebar and sidebar_text properties apply to the sidebar header

>+  ["--sidebar-background-color", {
>+    lwtProperty: "sidebar",
>+    optionalElementID: "sidebar-box",
>+    processColor(rgbaChannels, element) {
>+      if (!rgbaChannels) {
>+        element.removeAttribute("lwt-sidebar");
>+        return null;
>+      }
>+      const {r, g, b, a} = rgbaChannels;
>+      element.setAttribute("lwt-sidebar", "true");
>+      return `rgba(${r}, ${g}, ${b}, ${a})`;

What's the background behind sidebar-box? Trying to figure out if the alpha channel makes sense here.
(Assignee)

Comment 9

8 months ago
(In reply to Dão Gottwald [::dao] from comment #8)
> Comment on attachment 9003451 [details] [diff] [review]
> Make sidebar and sidebar_text properties apply to the sidebar header
> 
> >+  ["--sidebar-background-color", {
> >+    lwtProperty: "sidebar",
> >+    optionalElementID: "sidebar-box",
> >+    processColor(rgbaChannels, element) {
> >+      if (!rgbaChannels) {
> >+        element.removeAttribute("lwt-sidebar");
> >+        return null;
> >+      }
> >+      const {r, g, b, a} = rgbaChannels;
> >+      element.setAttribute("lwt-sidebar", "true");
> >+      return `rgba(${r}, ${g}, ${b}, ${a})`;
> 
> What's the background behind sidebar-box? Trying to figure out if the alpha
> channel makes sense here.

It's whatever background is set on #main-window, so it probably doesn't make sense. Will remove it in the next iteration of the patch.
(Assignee)

Updated

8 months ago
Attachment #9003451 - Attachment is obsolete: true
Attachment #9003451 - Flags: review?(dao+bmo)
Comment on attachment 9005816 [details] [diff] [review]
Make sidebar and sidebar_text properties apply to the sidebar header

>   ["--lwt-sidebar-background-color", {
>     lwtProperty: "sidebar",
>+    processColor(rgbaChannels) {
>+      if (!rgbaChannels) {
>+        return null;
>+      }
>+      const {r, g, b} = rgbaChannels;
>+      // Drop alpha channel
>+      return `rgb(${r}, ${g}, ${b})`;
>+    }

Can we add support for something like 'opaque: true' in the variable map to get rid of the code duplication for dropping alpha channels?
(Assignee)

Comment 12

8 months ago
(In reply to Dão Gottwald [::dao] from comment #11)
> Comment on attachment 9005816 [details] [diff] [review]
> Make sidebar and sidebar_text properties apply to the sidebar header
> 
> >   ["--lwt-sidebar-background-color", {
> >     lwtProperty: "sidebar",
> >+    processColor(rgbaChannels) {
> >+      if (!rgbaChannels) {
> >+        return null;
> >+      }
> >+      const {r, g, b} = rgbaChannels;
> >+      // Drop alpha channel
> >+      return `rgb(${r}, ${g}, ${b})`;
> >+    }
> 
> Can we add support for something like 'opaque: true' in the variable map to
> get rid of the code duplication for dropping alpha channels?

Good idea, filed bug 1487991 to do this.
Attachment #9005816 - Flags: review?(dao+bmo) → review+
(Assignee)

Updated

8 months ago
Keywords: checkin-needed

Comment 13

8 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d24dc1dba873
Consolidate browser sidebar styling. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7e99501586
Make sidebar and sidebar_text properties apply to the sidebar header. r=dao
Keywords: checkin-needed

Comment 15

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d24dc1dba873
https://hg.mozilla.org/mozilla-central/rev/9a7e99501586
https://hg.mozilla.org/mozilla-central/rev/dbd19d423cde
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(Assignee)

Updated

8 months ago
Depends on: 1488118
(Assignee)

Updated

8 months ago
Blocks: 1488000
You need to log in before you can comment on or make changes to this bug.