Closed Bug 1455138 Opened 6 years ago Closed 6 years ago

Use SVG context paint for macOS tree twisties

Categories

(Toolkit :: Themes, enhancement, P3)

Product:
Toolkit
Component:
Themes
Platform:
Unspecified
macOS
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

Details

(Whiteboard: [ntim-intern-project])

Attachments

(1 file, 2 obsolete files)

Bug 1455138 - Use SVG context paint for macOS tree twisties.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
Possible since bug 1381453
Priority: -- → P3
OS: Unspecified → Mac OS X
Summary: Use SVG context paint for tree twisties → Use SVG context paint for macOS tree twisties
Blocks: 1466826
No longer blocks: 1466826
Attachment #8983378 - Attachment is obsolete: true
Attachment #8983378 - Flags: review?(gijskruitbosch+bugs)
Attachment #8983379 - Attachment is obsolete: true
Attachment #8983379 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8983379 [details]
Bug 1455138 - Use SVG context paint for macOS tree twisties.

https://reviewboard.mozilla.org/r/249292/#review255456

::: browser/themes/osx/places/places.css:79
(Diff revision 1)
> -.sidebar-placesTreechildren::-moz-tree-twisty(closed, selected) {
> -  list-style-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-collapsed-inverted");
> +.sidebar-placesTreechildren::-moz-tree-twisty(selected) {
> +  fill: currentColor;

Why do you need to repeat the `fill: currentColor` here?

::: browser/themes/osx/places/places.css:109
(Diff revision 1)
> -  .sidebar-placesTreechildren:-moz-locale-dir(rtl)::-moz-tree-twisty(closed, selected) {
> -    list-style-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-collapsed-rtl");
>    }
>  
> -  .sidebar-placesTreechildren:-moz-locale-dir(rtl)::-moz-tree-twisty(closed, selected, focus) {
> -    list-style-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-collapsed-inverted-rtl")
> +  .sidebar-placesTreechildren::-moz-tree-twisty(selected, focus) {
> +    fill-opacity: 1;

Could we include the fill-opacity property in the earlier rules? It'd be a no-op for the cell-text, but it would simplify the stylesheets...
Attachment #8983379 - Flags: review+
Comment on attachment 8983385 [details]
Bug 1455138 - Use SVG context paint for macOS tree twisties.

https://reviewboard.mozilla.org/r/249294/#review255472

::: browser/themes/osx/places/organizer.css:44
(Diff revision 1)
>    border-top: 1px solid #505d6d;
>    margin: 0 10px;
>  }
>  
> +#placesList > treechildren::-moz-tree-twisty(selected),
>  #placesList > treechildren::-moz-tree-cell-text(selected) {  

Please fix the whitespace while we're here.

::: browser/themes/osx/places/organizer.css:59
(Diff revision 1)
> +  fill: currentColor;
> +  fill-opacity: 0.6;
>  }
>  
> -#placesList > treechildren::-moz-tree-twisty(closed, selected) {
> -  list-style-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-collapsed-inverted");
> +#placesList > treechildren::-moz-tree-twisty(selected) {
> +  fill: currentColor;

Same issue as before, also in the other files.

::: browser/themes/osx/places/organizer.css:88
(Diff revision 1)
> -  #placesList > treechildren:-moz-locale-dir(rtl)::-moz-tree-twisty(closed, selected) {
> -    list-style-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-collapsed-rtl");
>    }
>  
> -  #placesList > treechildren:-moz-locale-dir(rtl)::-moz-tree-twisty(closed, selected, focus) {
> -    list-style-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-collapsed-inverted-rtl");
> +  #placesList > treechildren::-moz-tree-twisty(selected, focus) {
> +    fill-opacity: 1;

see previous review, but also for the other files...

::: browser/themes/osx/syncedtabs/sidebar.css:43
(Diff revision 1)
> -  background-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-expanded");
> -}
> +  background-image: url("chrome://global/skin/tree/arrow-expanded.svg");
> +  -moz-context-properties: fill, fill-opacity;

This is a background image, not a list-style-image. Does the context-fill work in that case, also given this is an HTML document that IIRC might not even be privileged?

It's also not actually a XUL tree...
Attachment #8983385 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs (he/him) from comment #4)
> Comment on attachment 8983379 [details]
> Bug 1455138 - Use SVG context paint for macOS tree twisties.
> 
> https://reviewboard.mozilla.org/r/249292/#review255456
> 
> ::: browser/themes/osx/places/places.css:79
> (Diff revision 1)
> > -.sidebar-placesTreechildren::-moz-tree-twisty(closed, selected) {
> > -  list-style-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-collapsed-inverted");
> > +.sidebar-placesTreechildren::-moz-tree-twisty(selected) {
> > +  fill: currentColor;
> 
> Why do you need to repeat the `fill: currentColor` here?

It doesn't seem to work without the repetition.

> This is a background image, not a list-style-image. Does the context-fill work in that case, also given this is an HTML document that IIRC might not even be privileged?

Yes, it does, I've tested the patch, and other places in the source code use it the same way.
Comment on attachment 8983385 [details]
Bug 1455138 - Use SVG context paint for macOS tree twisties.

https://reviewboard.mozilla.org/r/249294/#review255616

reluctant r=me with the repetition removed or addressed in a follow-up bug with specifics about what doesn't work.

::: browser/themes/osx/places/organizer.css:60
(Diff revision 3)
> +  fill: currentColor;
> +  fill-opacity: 0.6;
>  }
>  
> -#placesList > treechildren::-moz-tree-twisty(closed, selected) {
> -  list-style-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-collapsed-inverted");
> +#placesList > treechildren::-moz-tree-twisty(selected) {
> +  fill: currentColor;

This not working without the repetition doesn't make sense. The previous rule should already apply, as `::-moz-tree-twisty` is less specific than `::-moz-tree-twisty(selected)`.

Can you ask someone who knows Stylo about why this wouldn't work (if landing this is blocking other stuff, maybe in a follow-up bug that you link to in a comment in the CSS) ?

::: browser/themes/osx/places/organizer.css:82
(Diff revision 3)
>    }
>  
> +  #placesList > treechildren::-moz-tree-twisty(selected, focus),
>    #placesList > treechildren::-moz-tree-cell-text(selected, focus) {
>      color: #fff;
> +    fill-opacity: 1;

In particular, you then don't seem to need to repeat the fill: currentColor bit here? :-\
Attachment #8983385 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs (he/him) from comment #9)
> Comment on attachment 8983385 [details]
> Bug 1455138 - Use SVG context paint for macOS tree twisties.
> 
> https://reviewboard.mozilla.org/r/249294/#review255616
> 
> reluctant r=me with the repetition removed or addressed in a follow-up bug
> with specifics about what doesn't work.
> 
> ::: browser/themes/osx/places/organizer.css:60
> (Diff revision 3)
> > +  fill: currentColor;
> > +  fill-opacity: 0.6;
> >  }
> >  
> > -#placesList > treechildren::-moz-tree-twisty(closed, selected) {
> > -  list-style-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-collapsed-inverted");
> > +#placesList > treechildren::-moz-tree-twisty(selected) {
> > +  fill: currentColor;
> 
> This not working without the repetition doesn't make sense. The previous
> rule should already apply, as `::-moz-tree-twisty` is less specific than
> `::-moz-tree-twisty(selected)`.
> 
> Can you ask someone who knows Stylo about why this wouldn't work (if landing
> this is blocking other stuff, maybe in a follow-up bug that you link to in a
> comment in the CSS) ?


I think it might be due to style invalidation (I've seen this bug before, specially on trees, where some properties would not update unless a paint was forced). I'll look more into it tomorrow, and see what I can do.


> ::: browser/themes/osx/places/organizer.css:82
> (Diff revision 3)
> >    }
> >  
> > +  #placesList > treechildren::-moz-tree-twisty(selected, focus),
> >    #placesList > treechildren::-moz-tree-cell-text(selected, focus) {
> >      color: #fff;
> > +    fill-opacity: 1;
> 
> In particular, you then don't seem to need to repeat the fill: currentColor
> bit here? :-\

The color: #fff; bit is probably forcing a repaint here.
ni? myself to look more into this tomorrow.
Assignee: nobody → ntim.bugs
Flags: needinfo?(ntim.bugs)
Blocks: 1418602
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6646425c8cd1
Use SVG context paint for macOS tree twisties. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/6646425c8cd1
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: needinfo?(ntim.bugs)
Whiteboard: [ntim-intern-project]
(In reply to :Gijs (Not available 3-19 Aug; he/him) from comment #9)
> Comment on attachment 8983385 [details]
> Bug 1455138 - Use SVG context paint for macOS tree twisties.
> 
> https://reviewboard.mozilla.org/r/249294/#review255616
> 
> reluctant r=me with the repetition removed or addressed in a follow-up bug
> with specifics about what doesn't work.
> 
> ::: browser/themes/osx/places/organizer.css:60
> (Diff revision 3)
> > +  fill: currentColor;
> > +  fill-opacity: 0.6;
> >  }
> >  
> > -#placesList > treechildren::-moz-tree-twisty(closed, selected) {
> > -  list-style-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-collapsed-inverted");
> > +#placesList > treechildren::-moz-tree-twisty(selected) {
> > +  fill: currentColor;
> 
> This not working without the repetition doesn't make sense. The previous
> rule should already apply, as `::-moz-tree-twisty` is less specific than
> `::-moz-tree-twisty(selected)`.
> 
> Can you ask someone who knows Stylo about why this wouldn't work (if landing
> this is blocking other stuff, maybe in a follow-up bug that you link to in a
> comment in the CSS) ?


Most of this code has been cleaned up as of bug 1469287.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: