Use SVG context paint for Windows tree twisties

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
11 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)

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

Possible since bug 1381453
No longer depends on: 1455138
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)
Blocks: 1418602
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?
(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.
(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)
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+
Fixed review comments.
Attachment #8987124 - Attachment is obsolete: true
Attachment #8987333 - Flags: review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/f7db3bf8ed5b
Status: ASSIGNED → RESOLVED
Closed: Last year
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.