Update tree twisties for Windows 10

RESOLVED FIXED in Firefox 45

Status

()

Toolkit
Themes
P4
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

(Blocks: 1 bug)

unspecified
mozilla46
All
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed, firefox46 fixed)

Details

Attachments

(1 attachment, 11 obsolete attachments)

(Assignee)

Description

3 years ago
The twisties for Windows 10 are looking different than the ones for Windows 7/8.
(Assignee)

Comment 1

3 years ago
Created attachment 8643562 [details] [diff] [review]
twisty.patch

Simply use overrides.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8643562 - Flags: review?(dao)
Comment on attachment 8643562 [details] [diff] [review]
twisty.patch

Please use the preWin10 pattern used elsewhere, i.e. use overrides for osversion<=6.3 rather than osversion>=10.

By the way, for high contrast themes, are these new images at least as good as the old ones?
Attachment #8643562 - Flags: review?(dao)

Updated

3 years ago
Blocks: 1158143
(Assignee)

Comment 3

3 years ago
Created attachment 8645170 [details] [diff] [review]
twisty.patch

Now defaulting to the win10 icons and added HiDPI icons for win10.

I had to add a special XP rule because the osversion<6 override was always sorted before the osversion<=6.3 override in manifest, which ended in wrong icons under XP. If there exists a solution I can use this.

For the HiDPI icons I needed to use a background-image instead of the list-style-image because the twisty always used the real icon dimensions also when defining width/height and/or max-width/max-height. Again when it exists a solution, I'll use this.
Attachment #8643562 - Attachment is obsolete: true
Attachment #8645170 - Flags: review?(dao)
Priority: -- → P4
(Assignee)

Comment 4

2 years ago
Created attachment 8664894 [details] [diff] [review]
twisty.patch

Unbitrotted patch
Attachment #8645170 - Attachment is obsolete: true
Attachment #8645170 - Flags: review?(dao)
Attachment #8664894 - Flags: review?(dao)
Comment on attachment 8664894 [details] [diff] [review]
twisty.patch

(In reply to Richard Marti (:Paenglab) from comment #3)
> Created attachment 8645170 [details] [diff] [review]
> twisty.patch
> 
> Now defaulting to the win10 icons and added HiDPI icons for win10.
> 
> I had to add a special XP rule because the osversion<6 override was always
> sorted before the osversion<=6.3 override in manifest, which ended in wrong
> icons under XP. If there exists a solution I can use this.

You mean it was automatically sorted even if you changed the order?

I believe you can use multiple osversion flags to exclude XP there, e.g. 'osversion=>6 osversion<=6.3'.

> For the HiDPI icons I needed to use a background-image instead of the
> list-style-image because the twisty always used the real icon dimensions
> also when defining width/height and/or max-width/max-height. Again when it
> exists a solution, I'll use this.

Weird. So 'width: 9px; /* The image's width is 9 pixels */' in the existing rules also has no effect? Would using SVGs avoid this problem?
Attachment #8664894 - Flags: review?(dao)
(In reply to Dão Gottwald [:dao] from comment #2)
> By the way, for high contrast themes, are these new images at least as good
> as the old ones?

What about this question?
(Assignee)

Comment 7

2 years ago
Created attachment 8666455 [details] [diff] [review]
twisty.patch

(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 8664894 [details] [diff] [review]
> twisty.patch
> 
> (In reply to Richard Marti (:Paenglab) from comment #3)
> > Created attachment 8645170 [details] [diff] [review]
> > twisty.patch
> > 
> > Now defaulting to the win10 icons and added HiDPI icons for win10.
> > 
> > I had to add a special XP rule because the osversion<6 override was always
> > sorted before the osversion<=6.3 override in manifest, which ended in wrong
> > icons under XP. If there exists a solution I can use this.
> 
> You mean it was automatically sorted even if you changed the order?

Yes.

> I believe you can use multiple osversion flags to exclude XP there, e.g.
> 'osversion=>6 osversion<=6.3'.

I already tried such things but it didn't work.

I renamed the XP twisties to twisty-clsd-preWin6.png and twisty-open-preWin6.png. Now the overrides are sorted behind the -preWin10 overrides. I could also let the XP files with the -XP extension and only rename them in jar.mn. Then they would be easier searchable in the tree.

> > For the HiDPI icons I needed to use a background-image instead of the
> > list-style-image because the twisty always used the real icon dimensions
> > also when defining width/height and/or max-width/max-height. Again when it
> > exists a solution, I'll use this.
> 
> Weird. So 'width: 9px; /* The image's width is 9 pixels */' in the existing
> rules also has no effect? Would using SVGs avoid this problem?

Yes, weird. And even worse, SVGs don't work as list-style-image as twisty in the tree. They are working as background-image like the PNGs.

(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > By the way, for high contrast themes, are these new images at least as good
> > as the old ones?
> 
> What about this question?

Yes, I would say they work as good as the old ones. And I would also say better than the original twisties in Explorer where they are almost invisible on non-hover.
Attachment #8664894 - Attachment is obsolete: true
Attachment #8666455 - Flags: review?(dao)
(Assignee)

Comment 8

2 years ago
Created attachment 8676292 [details] [diff] [review]
twisty.patch

unbitrotted
Attachment #8666455 - Attachment is obsolete: true
Attachment #8666455 - Flags: review?(dao)
Attachment #8676292 - Flags: review?(dao)

Comment 9

2 years ago
(In reply to Richard Marti (:Paenglab) from comment #7)
> > > For the HiDPI icons I needed to use a background-image instead of the
> > > list-style-image because the twisty always used the real icon dimensions
> > > also when defining width/height and/or max-width/max-height. Again when it
> > > exists a solution, I'll use this.
> > 
> > Weird. So 'width: 9px; /* The image's width is 9 pixels */' in the existing
> > rules also has no effect? Would using SVGs avoid this problem?
> 
> Yes, weird. And even worse, SVGs don't work as list-style-image as twisty in
> the tree. They are working as background-image like the PNGs.
They should, if you specify a width and a height on the root <svg> element inside the svg file.
(Assignee)

Comment 10

2 years ago
(In reply to Tim Nguyen [:ntim] from comment #9)
> (In reply to Richard Marti (:Paenglab) from comment #7)
> > Yes, weird. And even worse, SVGs don't work as list-style-image as twisty in
> > the tree. They are working as background-image like the PNGs.
> They should, if you specify a width and a height on the root <svg> element
> inside the svg file.

Yes, then svg works. But if we use svg files we have them to use also for XP and Vista/Win7/Win8 because of the overrides.
(Assignee)

Comment 11

2 years ago
Created attachment 8679085 [details] [diff] [review]
twisty.patch

Again unbitrotted.
Attachment #8676292 - Attachment is obsolete: true
Attachment #8676292 - Flags: review?(dao)
Attachment #8679085 - Flags: review?(dao)
Comment on attachment 8679085 [details] [diff] [review]
twisty.patch

(In reply to Richard Marti (:Paenglab) from comment #10)
> Yes, then svg works. But if we use svg files we have them to use also for XP
> and Vista/Win7/Win8 because of the overrides.

That sounds fine, i.e. there's no reason why we'd actively avoid SVG for these Windows versions. We'd just need to get the SVGs which means someone would need to do some extra work... Can you ask shorlander or phlsa about this?

>-  skin/classic/global/tree/twisty-clsd-XP.png                    (tree/twisty-clsd-XP.png)
>-  skin/classic/global/tree/twisty-open-XP.png                    (tree/twisty-open-XP.png)
>+  skin/classic/global/tree/twisty-clsd-preWin6.png               (tree/twisty-clsd-preWin6.png)
>+  skin/classic/global/tree/twisty-open-preWin6.png               (tree/twisty-open-preWin6.png)

XP is clearer than "preWin6", so please don't make this change.
Attachment #8679085 - Flags: review?(dao)
(Assignee)

Comment 13

2 years ago
Created attachment 8681648 [details] [diff] [review]
twisty.patch

Now with SVG files for the twisties.

I tried to use only three svg files but the override doesn't work with ID liking (or what's the name is for this). I'm using now every state a separate svg file like for png.

(In reply to Dão Gottwald [:dao] from comment #12)
> Comment on attachment 8679085 [details] [diff] [review]
> twisty.patch
> 
> >-  skin/classic/global/tree/twisty-clsd-XP.png                    (tree/twisty-clsd-XP.png)
> >-  skin/classic/global/tree/twisty-open-XP.png                    (tree/twisty-open-XP.png)
> >+  skin/classic/global/tree/twisty-clsd-preWin6.png               (tree/twisty-clsd-preWin6.png)
> >+  skin/classic/global/tree/twisty-open-preWin6.png               (tree/twisty-open-preWin6.png)
> 
> XP is clearer than "preWin6", so please don't make this change.

Like I wrote in comment 3 with -XP in the name the override is wrongly sorted in manifest before the -preWin10. I'm using now -XP as file name but rename it during build to -preWin6. Is this okay like this?

I could also use -Vista78 instead of -preWin10 then -XP would be sorted correctly after -Vista78.
Attachment #8679085 - Attachment is obsolete: true
Attachment #8681648 - Flags: review?(dao)
(Assignee)

Comment 14

2 years ago
Created attachment 8685589 [details] [diff] [review]
twisty.patch

Updated after landing of bug 1210703.

I moved twisty-open.png and twisty-clsd.png to linux because the png files are no more used on windows.
Attachment #8681648 - Attachment is obsolete: true
Attachment #8681648 - Flags: review?(dao)
Attachment #8685589 - Flags: review?(dao)
(Assignee)

Comment 15

2 years ago
Comment on attachment 8685589 [details] [diff] [review]
twisty.patch

It seems Dão has no time for a review and I'd like to have this in 45 for the ESR releases too.

Gijs, please could you look at this?
Attachment #8685589 - Flags: review?(dao) → review?(gijskruitbosch+bugs)

Comment 16

2 years ago
Comment on attachment 8685589 [details] [diff] [review]
twisty.patch

Review of attachment 8685589 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Richard Marti (:Paenglab) from comment #13)
> I tried to use only three svg files but the override doesn't work with ID
> liking (or what's the name is for this).

Ugh. I would prefer to fix the override system. Should be something similar to bug 1034999.

But for the moment, can't you write overrides like:

override foo.xul#whatever bar.xul#whatever

?

That will avoid having to load other files and so hover state updates will be quicker and/or not require disk IO (!). You already get to write all the overrides but now with filenames...


Too many issues to give you r+, but also, I would be more comfortable if Dão reviewed this.

::: toolkit/themes/windows/global/jar.mn
@@ +98,5 @@
> +  skin/classic/global/tree/twisty-open.svg                       (tree/twisty-open.svg)
> +  skin/classic/global/tree/twisty-open-rtl.svg                   (tree/twisty-open-rtl.svg)
> +  skin/classic/global/tree/twisty-open-hover.svg                 (tree/twisty-open-hover.svg)
> +  skin/classic/global/tree/twisty-open-hover-rtl.svg             (tree/twisty-open-hover-rtl.svg)
> +  skin/classic/global/tree/twisty-clsd-preWin6.svg               (tree/twisty-clsd-XP.svg)

I think we should use the same filename and resulting jar name. If the sorting doesn't work this way, update the sorting?

::: toolkit/themes/windows/global/tree/twisty-clsd.svg
@@ +2,5 @@
> +<!-- 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 xmlns="http://www.w3.org/2000/svg" width="9" height="9" viewBox="0 0 9 9">
> +  <path style="stroke:#b6b6b6; stroke-width:1.6; fill:none;" d="m 2.5,0.5 4,4 -4,4"/>

What is the 1.6 stroke-width about? Shouldn't it be 1 or 2 or 1.5?

::: toolkit/themes/windows/global/tree/twisty-open-XP.svg
@@ +15,5 @@
> +    <linearGradient id="gradient1" xlink:href="#linearGradient1" gradientUnits="userSpaceOnUse" x1="4.5" y1="2" x2="4.5" y2="7"/>
> +    <linearGradient id="gradient2" xlink:href="#linearGradient2" gradientUnits="userSpaceOnUse" x1="4.5" y1="6" x2="4.5" y2="3"/>
> +  </defs>
> +  <rect style="fill:url(#gradient1);stroke:#5d5cc2;stroke-linejoin:round;stroke-opacity:0.8" width="8" height="8" x="0.5" y="0.5"/>
> +  <rect style="fill:url(#gradient2);stroke:none;" width="5" height="5" x="2" y="2"/>

stroke: none is not necessary here, and there should be spaces after ":", here and in other files.

::: toolkit/themes/windows/global/tree/twisty-open.svg
@@ +2,5 @@
> +<!-- 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 xmlns="http://www.w3.org/2000/svg" width="9" height="9" viewBox="0 0 9 9">
> +  <path style="stroke:#636363; stroke-width:1.6; fill:none;" d="m 8.5,2.5 -4,4 -4,-4"/>

I don't think the viewBox makes a difference here, does it?

fill:none also seems unnecessary. Same feedback for some of the other files.
Attachment #8685589 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 17

2 years ago
Created attachment 8697049 [details] [diff] [review]
twisty.patch

(In reply to :Gijs Kruitbosch from comment #16)
> Comment on attachment 8685589 [details] [diff] [review]
> twisty.patch
> 
> Review of attachment 8685589 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Richard Marti (:Paenglab) from comment #13)
> > I tried to use only three svg files but the override doesn't work with ID
> > liking (or what's the name is for this).
> 
> Ugh. I would prefer to fix the override system. Should be something similar
> to bug 1034999.
> 
> But for the moment, can't you write overrides like:
> 
> override foo.xul#whatever bar.xul#whatever

Yes, this works. Thank you for the tip.

> I think we should use the same filename and resulting jar name. If the
> sorting doesn't work this way, update the sorting?

Done. Using now -XP and -Vista78. With this the entries are correctly sorted to work as expected. -Vista7 already exists in the tree.

> What is the 1.6 stroke-width about? Shouldn't it be 1 or 2 or 1.5?

This is to fit best the shape of the twisty. There are only diagonal lines and this adds no additional blurriness.

> I don't think the viewBox makes a difference here, does it?

Not needed and removed.
Attachment #8685589 - Attachment is obsolete: true
Attachment #8697049 - Flags: review?(gijskruitbosch+bugs)

Comment 18

2 years ago
Comment on attachment 8697049 [details] [diff] [review]
twisty.patch

Review of attachment 8697049 [details] [diff] [review]:
-----------------------------------------------------------------

Did you forget to qref or something? There are various references to twisty-clsd/twisty-open svg files in the jar.mn and pageInfo.css - but those files don't exist.

::: toolkit/themes/windows/global/jar.mn
@@ +145,5 @@
>  % override chrome://global/skin/icons/close@2x.png                chrome://global/skin/icons/close-XPVista7@2x.png         osversion<=6.1
>  % override chrome://global/skin/icons/close-inverted.png          chrome://global/skin/icons/close-inverted-XPVista7.png   osversion<=6.1
>  % override chrome://global/skin/icons/close-inverted@2x.png       chrome://global/skin/icons/close-inverted-XPVista7@2x.png osversion<=6.1
> +
> +% override chrome://global/skin/tree/twisty.svg#clsd              chrome://global/skin/tree/twisty-Vista78.svg#clsd           osversion<=6.3

This is going to override the XP one again, right? Because osversion<=6.3 is true there, too.

::: toolkit/themes/windows/global/tree/twisty-Vista78.svg
@@ +23,5 @@
> +    }
> +  </style>
> +  <defs>
> +  <path id="clsd-shape" d="m 2.5,0.5 4,4 -4,4 z"/>
> +  <path id="open-shape" d="M 7.5,3 7.5,7.5 3,7.5 3,6.5 6.5,3 Z"/>

Nit: busted spacing here and the previous line.
Attachment #8697049 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 19

2 years ago
Created attachment 8697091 [details] [diff] [review]
twisty.patch

(In reply to :Gijs Kruitbosch from comment #18)
> Comment on attachment 8697049 [details] [diff] [review]
> twisty.patch
> 
> Review of attachment 8697049 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you forget to qref or something? There are various references to
> twisty-clsd/twisty-open svg files in the jar.mn and pageInfo.css - but those
> files don't exist.

Yes I overlooked pageInfo.css but in jar.mn where is all okay. Maybe you have seen the png files for Linux.

> ::: toolkit/themes/windows/global/jar.mn
> @@ +145,5 @@
> >  % override chrome://global/skin/icons/close@2x.png                chrome://global/skin/icons/close-XPVista7@2x.png         osversion<=6.1
> >  % override chrome://global/skin/icons/close-inverted.png          chrome://global/skin/icons/close-inverted-XPVista7.png   osversion<=6.1
> >  % override chrome://global/skin/icons/close-inverted@2x.png       chrome://global/skin/icons/close-inverted-XPVista7@2x.png osversion<=6.1
> > +
> > +% override chrome://global/skin/tree/twisty.svg#clsd              chrome://global/skin/tree/twisty-Vista78.svg#clsd           osversion<=6.3
> 
> This is going to override the XP one again, right? Because osversion<=6.3 is
> true there, too.

In manifest it is sorted then in the right way:

override chrome://global/skin/tree/twisty.svg#clsd    chrome://global/skin/tree/twisty-Vista78.svg#clsd   osversion<=6.3
override chrome://global/skin/tree/twisty.svg#clsd    chrome://global/skin/tree/twisty-XP.svg#clsd      osversion<6

Like this the XP line is overriding the Vista78 line

> ::: toolkit/themes/windows/global/tree/twisty-Vista78.svg
> @@ +23,5 @@
> > +    }
> > +  </style>
> > +  <defs>
> > +  <path id="clsd-shape" d="m 2.5,0.5 4,4 -4,4 z"/>
> > +  <path id="open-shape" d="M 7.5,3 7.5,7.5 3,7.5 3,6.5 6.5,3 Z"/>
> 
> Nit: busted spacing here and the previous line.

Fixed
Attachment #8697049 - Attachment is obsolete: true
Attachment #8697091 - Flags: review?(gijskruitbosch+bugs)

Comment 20

2 years ago
Comment on attachment 8697091 [details] [diff] [review]
twisty.patch

Review of attachment 8697091 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/themes/windows/global/jar.mn
@@ +138,5 @@
>  % override chrome://global/skin/toolbar/spring.png                chrome://global/skin/toolbar/spring-XP.png               osversion<6
>  % override chrome://global/skin/tree/sort-asc.png                 chrome://global/skin/tree/sort-asc-XP.png                osversion<6
>  % override chrome://global/skin/tree/sort-dsc.png                 chrome://global/skin/tree/sort-dsc-XP.png                osversion<6
> +% override chrome://global/skin/tree/twisty.svg#clsd              chrome://global/skin/tree/twisty-XP.svg#clsd             osversion<6
> +% override chrome://global/skin/tree/twisty.svg#open              chrome://global/skin/tree/twisty-XP.svg#open             osversion<6

Relying on the ordering in the manifest being the inverse of the one here seems very fragile and likely to change.

Can you instead make the XP files override the Vista78 ones, and leave a comment about why? Because then both overrides will apply and things Should Just Work, irrespective of the ordering - though it'd be good if you tested that... If you can't, then please let me know - I'll be back to my Windows VMs on Monday and can check then. (I tested on my local machine with a random manifest using a double redirect, but it'd be good to be very sure...)

The usual way to do this is to have single-version-specific overrides (cf. https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/jar.mn#293-312 ) but that seems like it'd be very verbose in this case. Did you file a bug about the #ref thing and overrides?
Attachment #8697091 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 21

2 years ago
Created attachment 8697620 [details] [diff] [review]
twisty.patch

The XP twisties are overriding the Vista78 twisties now. Tested on XP up to Win10.
Attachment #8697091 - Attachment is obsolete: true
Attachment #8697620 - Flags: review?(gijskruitbosch+bugs)

Updated

2 years ago
Attachment #8697620 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 22

2 years ago
Thank you, Gijs.
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Blocks: 1232004
Backed out in https://hg.mozilla.org/integration/fx-team/rev/22137526f21e because either it or bug 1227527 caused the build bustage in https://treeherder.mozilla.org/logviewer.html#?job_id=6186947&repo=fx-team which only seems to have affected Windows builds and B2G emulator builds, and I can't figure out which of these bugs could have done it.
Flags: needinfo?(richard.marti)
(Assignee)

Comment 25

2 years ago
Locally this builds without a problem on Windows. To be sure I started a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18a2bd634fbf
Flags: needinfo?(richard.marti)

Comment 26

2 years ago
(In reply to Richard Marti (:Paenglab) from comment #25)
> Locally this builds without a problem on Windows. To be sure I started a
> try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18a2bd634fbf

bug 1227527 comment 47 confirms it was the other patch, so re-setting checkin-needed.
Keywords: checkin-needed
(Assignee)

Comment 27

2 years ago
No, my patch also breaks on packaging, see the try build. I don't know why now, but trying something.
Keywords: checkin-needed

Updated

2 years ago
Depends on: 1232421
(Assignee)

Comment 28

2 years ago
Try build with needed patches from bug 1232421: https://treeherder.mozilla.org/#/jobs?repo=try&revision=564e8eb96187
Keywords: checkin-needed

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/81e696a6c8ea
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Comment 31

2 years ago
Comment on attachment 8697620 [details] [diff] [review]
twisty.patch

Approval Request Comment
[Feature/regressing bug #]: adds twisty appearance like in native Windows 10
[User impact if declined]: user has no native appearance, for ESR user then for a very long time
[Risks and why]: only SVG icon and CSS changes, aurora only just branched 5 days ago - very low risk
[String/UUID change made/needed]: no
Attachment #8697620 - Flags: approval-mozilla-aurora?
status-firefox45: --- → affected
Attachment #8697620 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 32

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/cefffada2592
status-firefox45: affected → fixed
Depends on: 1234439
Created attachment 8701689 [details] [diff] [review]
followup patch

The '#XXX' parts do not involve in file locating, and thus are useless.

In addition, adding this breaks 'mach build faster' (see bug 1234439). Although they've decided to fix the build system to tolerate '#' here, there's a chance that some code could still be broken by this.

Given it is useless, and potentially harmful, I suggest we just remove them and use the usual form.
Attachment #8701689 - Flags: review?(dao)
Comment on attachment 8701689 [details] [diff] [review]
followup patch

OK, I was wrong.
Attachment #8701689 - Attachment is obsolete: true
Attachment #8701689 - Flags: review?(dao)
You need to log in before you can comment on or make changes to this bug.