1191230 - Update tree twisties for Windows 10

Status: RESOLVED FIXED

(Toolkit :: Themes, defect, P4)

Description

[Richard Marti (:Paenglab)]

The twisties for Windows 10 are looking different than the ones for Windows 7/8.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: twisty.patch

Simply use overrides.

Comment 2

[Dão Gottwald [: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?

Comment 3

[Richard Marti (:Paenglab)]

Attachment: 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.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: twisty.patch

Unbitrotted patch

Comment 5

[Dão Gottwald [: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?

Comment 6

[Dão Gottwald [: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?

Comment 7

[Richard Marti (:Paenglab)]

Attachment: 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.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: twisty.patch

unbitrotted

Comment 9

[Tim Nguyen :ntim]

(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.

Comment 10

[Richard Marti (:Paenglab)]

(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.

Comment 11

[Richard Marti (:Paenglab)]

Attachment: twisty.patch

Again unbitrotted.

Comment 12

[Dão Gottwald [: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.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: 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.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: 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.

Comment 15

[Richard Marti (:Paenglab)]

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?

Comment 16

[:Gijs (he/him)]

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.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: 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.

Comment 18

[:Gijs (he/him)]

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/[e-mail]                chrome://global/skin/icons/[e-mail]         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/[e-mail]       chrome://global/skin/icons/[e-mail] 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.

Comment 19

[Richard Marti (:Paenglab)]

Attachment: 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/[e-mail]                chrome://global/skin/icons/[e-mail]         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/[e-mail]       chrome://global/skin/icons/[e-mail] 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

Comment 20

[:Gijs (he/him)]

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?

Comment 21

[Richard Marti (:Paenglab)]

Attachment: twisty.patch

The XP twisties are overriding the Vista78 twisties now. Tested on XP up to Win10.

Comment 22

[Richard Marti (:Paenglab)]

Thank you, Gijs.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3fc5a790f110

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 25

[Richard Marti (:Paenglab)]

Locally this builds without a problem on Windows. To be sure I started a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18a2bd634fbf

Comment 26

[:Gijs (he/him)]

(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.

Comment 27

[Richard Marti (:Paenglab)]

No, my patch also breaks on packaging, see the try build. I don't know why now, but trying something.

Comment 28

[Richard Marti (:Paenglab)]

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

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/81e696a6c8ea

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81e696a6c8ea

Comment 31

[Richard Marti (:Paenglab)]

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

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cefffada2592

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Attachment: 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.

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8701689 [details] [diff] [review]
followup patch

OK, I was wrong.