Use SVG context paint for Windows tree twisties

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
11 months ago
8 months ago

People

(Reporter: ntim, Assigned: Paenglab)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Unspecified
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 months ago
+++ This bug was initially created as a clone of Bug #1455138 +++

Possible since bug 1381453
(Reporter)

Updated

11 months ago
No longer depends on: 1455138
(Assignee)

Comment 1

10 months ago
Posted patch Bug1466826.patch (obsolete) — Splinter Review
It seems the button[type="disclosure"] is nowhere used. Should I remove it in this patch?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8985220 - Flags: review?(dao+bmo)
(Reporter)

Updated

10 months ago
Blocks: 1418602
(Reporter)

Updated

10 months ago
Blocks: 1469287
Comment on attachment 8985220 [details] [diff] [review]
Bug1466826.patch

>+++ b/toolkit/themes/windows/global/tree/twisty-preWin10-collapsed-rtl.svg
>@@ -0,0 +1,6 @@
>+<!-- This Source Code Form is subject to the terms of the Mozilla Public
>+   - License, v. 2.0. If a copy of the MPL was not distributed with this
>+   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>+<svg width="9" height="9" xmlns="http://www.w3.org/2000/svg" stroke="context-fill" stroke-opacity="context-fill-opacity" fill="#c0e8f9" fill-opacity="context-stroke-opacity">

Ahem... Can you use context-stroke for stroke, context-stroke-opacity for stroke-opacity and context-fill-opacity for fill-opacity?
Why are you adding stuff to allowed-dupes.mn? Are the new files duplicates of existing ones?
(Assignee)

Comment 4

10 months ago
(In reply to Dão Gottwald [::dao] from comment #2)
> Comment on attachment 8985220 [details] [diff] [review]
> Bug1466826.patch
> 
> >+++ b/toolkit/themes/windows/global/tree/twisty-preWin10-collapsed-rtl.svg
> >@@ -0,0 +1,6 @@
> >+<!-- This Source Code Form is subject to the terms of the Mozilla Public
> >+   - License, v. 2.0. If a copy of the MPL was not distributed with this
> >+   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >+<svg width="9" height="9" xmlns="http://www.w3.org/2000/svg" stroke="context-fill" stroke-opacity="context-fill-opacity" fill="#c0e8f9" fill-opacity="context-stroke-opacity">
> 
> Ahem... Can you use context-stroke for stroke, context-stroke-opacity for
> stroke-opacity and context-fill-opacity for fill-opacity?

I made this to behave like the other twisties from osx or Win10. They work only with the fill and have no stroke. The -preWin10 ones use the stroke as the, for the themeable sidebars, color changeable element. The fill is fixed and only the opacity changes on hover. With this, no special casing for Win7 and -8 is needed in compacttheme.css to set the twisty color.

I hope this explanation is clear what I mean.

(In reply to Dão Gottwald [::dao] from comment #3)
> Why are you adding stuff to allowed-dupes.mn? Are the new files duplicates
> of existing ones?

This was because of the overrides in manifest. I changed this now to only use the twisty-preWin10-expanded-rtl on Win7 and -8. With this no dupe happens.
Attachment #8985220 - Attachment is obsolete: true
Attachment #8985220 - Flags: review?(dao+bmo)
Attachment #8986845 - Flags: review?(dao+bmo)
(In reply to Richard Marti (:Paenglab) from comment #4)
> (In reply to Dão Gottwald [::dao] from comment #3)
> > Why are you adding stuff to allowed-dupes.mn? Are the new files duplicates
> > of existing ones?
> 
> This was because of the overrides in manifest. I changed this now to only
> use the twisty-preWin10-expanded-rtl on Win7 and -8. With this no dupe
> happens.

I still don't understand what you're doing here. Generally using overrides for this kind of stuff is the right thing to do, and this shouldn't result in duplicate files.
(Assignee)

Comment 6

10 months ago
(In reply to Dão Gottwald [::dao] from comment #5)
> I still don't understand what you're doing here. Generally using overrides
> for this kind of stuff is the right thing to do, and this shouldn't result
> in duplicate files.

With the first patch I overrided all icons. For Win10 the expanded RTL icon was the same as for LTR. Because of this I needed the entries in allowed-dupes.mn. With the second patch I only define for Win10 the expanded icon. Then for Win7 and -8 I specially define a RTL expanded icon where it's different to the LTR.
Comment on attachment 8986845 [details] [diff] [review]
Bug1466826-win-treetwisty.patch

Let's get rid of the pre-Win10 stuff then, since bug 1469287 will want this anyway.
Attachment #8986845 - Flags: review?(dao+bmo)
(Assignee)

Comment 8

10 months ago
Now with the same twisty for all Windows versions.
Attachment #8986845 - Attachment is obsolete: true
Attachment #8987124 - Flags: review?(dao+bmo)
Comment on attachment 8987124 [details] [diff] [review]
Bug1466826-win-treetwisty.patch

>+.item.client .item-twisty-container:hover,
>+.item.client.closed .item-twisty-container:hover {
>+  fill: #4ed0f9;
> }

The .item.client.closed .item-twisty-container:hover selector seems redundant for this rule.

>+treechildren::-moz-tree-twisty(hover),
>+treechildren::-moz-tree-twisty(hover, open) {
>+  fill: #4ed0f9;
> }

The treechildren::-moz-tree-twisty(hover, open) selector seems redundant for this rule.
Attachment #8987124 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 10

10 months ago
Fixed review comments.
Attachment #8987124 - Attachment is obsolete: true
Attachment #8987333 - Flags: review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 11

10 months ago
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7db3bf8ed5b
Use SVG context paint for the Windows tree twisties. r=dao
Keywords: checkin-needed

Comment 12

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7db3bf8ed5b
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1476790
Duplicate of this bug: 434419
You need to log in before you can comment on or make changes to this bug.