Support SVG context paint on ::-moz-tree-twisty list-style-image

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: nhnt11, Assigned: ntim)

Tracking

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

2 years ago
Filing a separate bug for this because of an issue: CSS transform does not seem to work in the ::-moz-tree-twisty selector, so I can't rotate the arrow using CSS.

Dao, do you have any ideas? Am I missing something?

Otherwise, Stephen, could you whip up an SVG that contains all orientations of the icon? (Uh, I suppose we don't need an icon pointing upward though)
Flags: needinfo?(shorlander)
Flags: needinfo?(dao+bmo)
Reporter

Comment 1

2 years ago
I made an SVG with multiple paths, and that worked, but revealed another problem: I can't set fill from CSS. Changing this bug to refer more generally to the inability to style the list-style-image of a "twisty" using CSS.

I'm moving this to Toolkit :: XUL Widgets, please punt it somewhere else if this is wrong (sorry in advance).
Component: Theme → XUL Widgets
Flags: needinfo?(shorlander)
Flags: needinfo?(dao+bmo)
Product: Firefox → Toolkit
Summary: [Photon] Update sidebar tree arrow icons → The list-style-image of a ::-moz-tree-twisty cannot be styled (ex. fill, transform)
Reporter

Comment 2

2 years ago
The sidebar spec includes animating the rotation of the arrow when an item is expanded, so I don't see a way to work around this.
(In reply to Nihanth Subramanya [:nhnt11] from comment #0)
> Filing a separate bug for this because of an issue: CSS transform does not
> seem to work in the ::-moz-tree-twisty selector, so I can't rotate the arrow
> using CSS.
> 
> Dao, do you have any ideas? Am I missing something?
> 
> Otherwise, Stephen, could you whip up an SVG that contains all orientations
> of the icon? (Uh, I suppose we don't need an icon pointing upward though)

Do you need one SVG with all of the paths? Or four separate icons?
Reporter

Comment 4

2 years ago
(In reply to Stephen Horlander [:shorlander] from comment #3)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #0)
> > Filing a separate bug for this because of an issue: CSS transform does not
> > seem to work in the ::-moz-tree-twisty selector, so I can't rotate the arrow
> > using CSS.
> > 
> > Dao, do you have any ideas? Am I missing something?
> > 
> > Otherwise, Stephen, could you whip up an SVG that contains all orientations
> > of the icon? (Uh, I suppose we don't need an icon pointing upward though)
> 
> Do you need one SVG with all of the paths? Or four separate icons?

Hey, I managed to make an SVG with all the paths but there are still problems, so for now we're good with the icons. See comment 1.
Flags: needinfo?(nhnt11)
context-fill and transform probably just aren't implemented for these XUL tree pseudo elements. jwatt would know more.
Reporter

Updated

2 years ago
Blocks: 1385518
Reporter

Comment 6

2 years ago
Forgot to needinfo jwatt after he came back.

jwatt, what do you think of this? tl;dr is in comment 0 and comment 5
Flags: needinfo?(jwatt)
The 'context-fill' and 'transform' properties are two very different properties and will probably require two separate fixes. Can you create a separate bug for the 'context-fill' issue?

Regarding ::-moz-tree-twisty, I have to confess I don't know anything about this pseudo-class and:

https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-tree-twisty

isn't very helpful. Could you provide a quick summary? And could you provide a reduced testcase that I can dig into (one than needs the pref dom.allow_XUL_XBL_for_file to be enabled).
Flags: needinfo?(jwatt) → needinfo?(nhnt11)
Reporter

Comment 8

2 years ago
Posted file minimal testcase
(In reply to Jonathan Watt [:jwatt] from comment #7)
> The 'context-fill' and 'transform' properties are two very different
> properties and will probably require two separate fixes. Can you create a
> separate bug for the 'context-fill' issue?
> 
> Regarding ::-moz-tree-twisty, I have to confess I don't know anything about
> this pseudo-class and:
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-tree-twisty
> 
> isn't very helpful. Could you provide a quick summary? And could you provide
> a reduced testcase that I can dig into (one than needs the pref
> dom.allow_XUL_XBL_for_file to be enabled).

This selects the little arrow next to parent items in tree views. The arrow points down when the item is expanded and points at the item label when it is collapsed.

I've attached a minimal testcase. There's a single tree item that has a child and can be expanded. The twisty is the arrow that appears next to the parent element's label.

Sorry I didn't respond to this earlier.
Flags: needinfo?(nhnt11) → needinfo?(jwatt)

Comment 9

2 years ago
This would need to be implemented in nsTreeBodyFrame::PaintTwisty.
Priority: -- → P3
Blocks: 1400266
Assignee

Comment 10

a year ago
Got a working patch.
Assignee: nobody → ntim.bugs
Flags: needinfo?(jwatt)
Assignee

Comment 11

a year ago
Restricting this bug to context paint.
Summary: The list-style-image of a ::-moz-tree-twisty cannot be styled (ex. fill, transform) → Support SVG context paint on ::-moz-tree-twisty list-style-image
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Component: XUL Widgets → XUL
Product: Toolkit → Core
Assignee

Updated

a year ago
Attachment #8962141 - Flags: review?(emilio)
Attachment #8962141 - Flags: review?(dholbert)
Assignee

Comment 14

a year ago
I set some alternate reviewers since jwatt is away until next week. :)

Comment 15

a year ago
mozreview-review
Comment on attachment 8962141 [details]
Bug 1381453 - Support SVG context paint on ::-moz-tree-twisty list-style-image.

https://reviewboard.mozilla.org/r/230988/#review236448

I have no particular in-depth knowledge of this code, nor context paint. The patch looks reasonable, but I suspect Daniel is a better reviewer than me for this.

If needed feel free to re-request review and I'll dig into the code. Thanks!
Attachment #8962141 - Flags: review?(emilio)

Comment 16

a year ago
mozreview-review
Comment on attachment 8962141 [details]
Bug 1381453 - Support SVG context paint on ::-moz-tree-twisty list-style-image.

https://reviewboard.mozilla.org/r/230988/#review236758

Per IRC, this needs some automated tests. See https://dxr.mozilla.org/mozilla-central/source/layout/reftests/svg/as-image/ for some examples of context-fill & context-stroke reftests.

::: layout/xul/tree/nsTreeBodyFrame.cpp:3504
(Diff revision 2)
> -          pt.y += (twistyRect.height - imageSize.height)/2;
> +          anchorPoint.y += (twistyRect.height - imageSize.height)/2;
>          }
>  
> +        // Apply context paint if applicable
> +        Maybe<SVGImageContext> svgContext;
> +        SVGImageContext::MaybeStoreContextPaint(svgContext, twistyContext, image);

This line is too long -- please linewrap after the last comma.

::: layout/xul/tree/nsTreeBodyFrame.cpp:3508
(Diff revision 2)
> +        Maybe<SVGImageContext> svgContext;
> +        SVGImageContext::MaybeStoreContextPaint(svgContext, twistyContext, image);
> +
>          // Paint the image.
>          result &=
> -          nsLayoutUtils::DrawSingleUnscaledImage(
> +          nsLayoutUtils::DrawSingleImage(

You're switching from DrawSingleUnscaledImage to DrawSingleImage... Seems like that might change behavior in some cases, right?  Is that bad?

(If we rely on the "not-scaling" aspect of DrawSingleUnscaledImage() for correctness here [I don't know offhand since XUL trees are new to me], then that seems like this would be bad.  Maybe it'd be safer to extend DrawSingleUnscaledImage take a svgContext arg, like DrawSingleImage does?)
Attachment #8962141 - Flags: review?(dholbert) → review-
Comment hidden (mozreview-request)

Comment 18

a year ago
mozreview-review
Comment on attachment 8962141 [details]
Bug 1381453 - Support SVG context paint on ::-moz-tree-twisty list-style-image.

https://reviewboard.mozilla.org/r/230988/#review236812

Thanks! Nearly there, just a few nits:

::: layout/reftests/xul/reftest.list:78
(Diff revision 3)
>  # bug 1218954.
>  skip == treecell-image-svg-1a.xul treecell-image-svg-1-ref.xul # bug 1218954
>  skip == treecell-image-svg-1b.xul treecell-image-svg-1-ref.xul # bug 1218954
>  
>  == treechildren-padding-percent-1.xul treechildren-padding-percent-1-ref.xul
> +test-pref(svg.context-properties.content.enabled,true) == treetwisty-svg-context-paint.xul treetwisty-svg-context-paint-ref.xul

Please give this reftest a "-1" suffix, like the other tests here.

(That gives us the option of easily adding additional variants of this test down the line, as we discover new things that need testing.)

::: layout/reftests/xul/treetwisty-context-paint.svg:1
(Diff revision 3)
> +<!-- 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="18" height="18" viewBox="0 0 18 18"

Drop the license block from both SVG files here -- it's unnecessary.

Test licensing is covered here under "PD Test Code":
  https://www.mozilla.org/en-US/MPL/license-policy/

tl;dr, our tests are public-domain by default (so by including this license, you'd actually be placing these files under a more restrictive license than they would be under otherwise :))

::: layout/reftests/xul/treetwisty-context-paint.svg:6
(Diff revision 3)
> +<!-- 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="18" height="18" viewBox="0 0 18 18"
> +     fill="context-fill" stroke="context-stroke" fill-opacity="context-fill-opacity" stroke-opacity="context-stroke-opacity">
> +  <path d="M14 17H4a2 2 0 0 1-2-2V7a2 2 0 0 1 2-2h2l3-3 3 3h2a2 2 0 0 1 2 2v8a2 2 0 0 1-2 2zm-.34-9.369l-1.819 1.817a.917.917 0 1 1-1.3-1.3l1.81-1.807a2.979 2.979 0 0 0-4.168 3.694L4.38 13.808a1.271 1.271 0 0 0 0 1.808 1.3 1.3 0 0 0 1.824 0l3.819-3.783a2.98 2.98 0 0 0 3.99-2.823 2.957 2.957 0 0 0-.353-1.379zM5.5 15.005a.5.5 0 1 1 .5-.5.5.5 0 0 1-.5.5z"/>

Rather than this complex path (too long to fit on a single line in an editor), you might consider just a circle:

 <circle cx="9" cy="9" r="8"/>

(numbers chosen to be centered in this 18x18 SVG, with 1px of breathing room at the edges for the default-1px-wide stroke)

::: layout/xul/tree/nsTreeBodyFrame.cpp:3888
(Diff revision 3)
> -
> +  
> +    // Apply context paint if applicable

Looks like you're adding 2 space characters on a blank line here. (shown as red in mozreview)

Please revert those.
Attachment #8962141 - Flags: review?(dholbert) → review-
Comment hidden (mozreview-request)

Comment 20

a year ago
mozreview-review
Comment on attachment 8962141 [details]
Bug 1381453 - Support SVG context paint on ::-moz-tree-twisty list-style-image.

https://reviewboard.mozilla.org/r/230988/#review236856

Looks good. Thanks! (Don't forget to mark review nits as addressed, so that MozReview lets this be autolanded.)
Attachment #8962141 - Flags: review?(dholbert) → review+

Comment 21

a year ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/97c26c6c275c
Support SVG context paint on ::-moz-tree-twisty list-style-image. r=dholbert

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/97c26c6c275c
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1449118
Depends on: 1450101
Assignee

Updated

a year ago
Blocks: 1455138
Assignee

Updated

a year ago
Blocks: 1466826
You need to log in before you can comment on or make changes to this bug.