Closed Bug 982355 Opened 6 years ago Closed 6 years ago

Support disclosure-{open,closed} counter styles

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug, )

Details

(Keywords: css3, dev-doc-complete)

Attachments

(2 files, 4 obsolete files)

As described in CSS Counter Styles Level 3, the pair of predefined counter styles are defined for disclosure widgets, which should be supported.
Severity: normal → enhancement
Assignee: nobody → quanxunzhen
Status: NEW → ASSIGNED
In my WIP patch for <details>/<summary> support in attachment 807101 [details] [diff] [review], I have support for |list-style: -moz-disclosure-{open,closed}|.  If you want to get that in (unprefix) separately from <details>/<summary> you might like to grab it out of there.
(In reply to Cameron McCormack (:heycam) from comment #1)
> In my WIP patch for <details>/<summary> support in attachment 807101 [details] [diff] [review]
> [details] [diff] [review], I have support for |list-style:
> -moz-disclosure-{open,closed}|.  If you want to get that in (unprefix)
> separately from <details>/<summary> you might like to grab it out of there.

Yes, thanks to your patch, I've done that on top of my patchset in bug 966166. Though, I have two questions about your impl:
1. Why did you use ratio 7:9 for the triangle? Is there any consideration?
2. Why did you deflate the drawing rect by 1 unit?
Flags: needinfo?(cam)
I used that ratio because that's what the native OS X disclosure widget triangle looks like, and I think it's visually pleasing.  I don't remember why I deflated the drawing rect.
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #3)
> I don't remember why I deflated the drawing rect.

(Maybe it was that it matched the size of the native disclosure widget triangle when using the default OS X UI font and font size.)
Attached patch patch (obsolete) — Splinter Review
Since this patch is based on my patchset in bug 966166, it cannot be applied independently. So I do not request for review now.
FYI: This is on Tab's to-do list for spec changes...

"Alter the disclosure-* counter styles to be direction-aware, use  :dir()  in the UA stylesheet to apply it."

...so stay tuned.
(In reply to aja+bugzilla from comment #6)
> FYI: This is on Tab's to-do list for spec changes...
> 
> "Alter the disclosure-* counter styles to be direction-aware, use  :dir() 
> in the UA stylesheet to apply it."
> 
> ...so stay tuned.

It has been direction-aware in my patch. Not only direction, the coming feature writing-mode has been taken into account as well. The only left problem is the characters for overriding and spoken text. They are currently not direction-aware, and I don't have a clear idea how to make them direction-aware yet.
My thinking's something like this: 

counter-style disclosure-open {
  system: cyclic;
  symbols: \25BE;
  suffix: "";
}  /* U+25BE BLACK DOWN-POINTING SMALL TRIANGLE (?) */

@counter-style disclosure-closed-rtl {
  system: cyclic;
  symbols: \25B8;
  suffix: "";
}  /* U+25B8 BLACK RIGHT-POINTING SMALL TRIANGLE (?) */

@counter-style disclosure-closed-ltr {
  system: cyclic;
  symbols: \25C2;
  suffix: "";
}  /* U+25C2 BLACK LEFT-POINTING SMALL TRIANGLE (?) */

details > summary {
  display: list-item;
  list-style-type: disclosure-closed-ltr;
  list-style-position: inside;
}

details > summary:dir(rtl) {
  list-style-type: disclosure-closed-rtl;
}
(In reply to aja+bugzilla from comment #8)
> My thinking's something like this: 
> 
> counter-style disclosure-open {
>   system: cyclic;
>   symbols: \25BE;
>   suffix: "";
> }  /* U+25BE BLACK DOWN-POINTING SMALL TRIANGLE (?) */
> 
> @counter-style disclosure-closed-rtl {
>   system: cyclic;
>   symbols: \25B8;
>   suffix: "";
> }  /* U+25B8 BLACK RIGHT-POINTING SMALL TRIANGLE (?) */
> 
> @counter-style disclosure-closed-ltr {
>   system: cyclic;
>   symbols: \25C2;
>   suffix: "";
> }  /* U+25C2 BLACK LEFT-POINTING SMALL TRIANGLE (?) */
> 
> details > summary {
>   display: list-item;
>   list-style-type: disclosure-closed-ltr;
>   list-style-position: inside;
> }
> 
> details > summary:dir(rtl) {
>   list-style-type: disclosure-closed-rtl;
> }

This is not correct. The disclosure-{closed,open} themselves should be aware of the direction, even when applied to LI.
Attached patch patch (obsolete) — Splinter Review
This patch is based on the latest patchset for bug 966166. Since those patches are landing soon, I'd like to request for review now.
Attachment #8395092 - Attachment is obsolete: true
Attachment #8438944 - Flags: review?(jfkthame)
Comment on attachment 8438944 [details] [diff] [review]
patch

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

Why do we need code in nsBulletFrame to explicitly draw disclosure triangles? Can't we just use symbol characters to render them in all cases?
Attached image Comparison
(In reply to Jonathan Kew (:jfkthame) from comment #11)
> Why do we need code in nsBulletFrame to explicitly draw disclosure
> triangles? Can't we just use symbol characters to render them in all cases?

Please see the attachment. I think the generated ones look better than the characters.
We'd want to apply a suitable symbol font to the characters, rather than whatever "random" font happens to be in use (or to be found via fallback), but with that enhancement I think characters could look fine. E.g. for OS X, something like

  data:text/html,<div style="font-family:times,menlo;">
    &%23x25B6;&nbsp;&nbsp;character closed<br>
    &%23x25Bc;&nbsp;&nbsp;character open

seems to look fine, IMO. On Windows, perhaps Lucida Sans Unicode or Segoe UI Symbol would be suitable.

Rendering as glyphs should in principle be more performant, as well as requiring less code (I hope).
(In reply to Jonathan Kew (:jfkthame) from comment #13)
> We'd want to apply a suitable symbol font to the characters, rather than
> whatever "random" font happens to be in use (or to be found via fallback),
> but with that enhancement I think characters could look fine. E.g. for OS X,
> something like
> 
>   data:text/html,<div style="font-family:times,menlo;">
>     &%23x25B6;&nbsp;&nbsp;character closed<br>
>     &%23x25Bc;&nbsp;&nbsp;character open
> 
> seems to look fine, IMO. On Windows, perhaps Lucida Sans Unicode or Segoe UI
> Symbol would be suitable.
> 
> Rendering as glyphs should in principle be more performant, as well as
> requiring less code (I hope).

I don't think it would be more performant, considering that using font would require additional steps including looking for font, checking range, reading glyph data, considering font features, etc. Here, we just compute three points and draw a triangle. Why do you think rendering as glyph would be more performant?

OTOH, with choosing a suitable font, we still need to handle these predefines differently, since we should not choose font for other styles, especially custom counter styles. In addition, although we need not compute the points, we still have to handle the direction by code in this case. We have to decide which glyph should be used according to the writing mode, as the glyphs won't flip automatically. Hence most of the current code would not be changed. Furthermore, to have proper symbol fonts on all platforms, we would have to add a new platform-dependent pref, which again costs code. For these reasons, I wonder if it could actually reduce the code size.
(In reply to Xidorn Quan (UTC+10) from comment #14)
> (In reply to Jonathan Kew (:jfkthame) from comment #13)
> > We'd want to apply a suitable symbol font to the characters, rather than
> > whatever "random" font happens to be in use (or to be found via fallback),
> > but with that enhancement I think characters could look fine. E.g. for OS X,
> > something like
> > 
> >   data:text/html,<div style="font-family:times,menlo;">
> >     &%23x25B6;&nbsp;&nbsp;character closed<br>
> >     &%23x25Bc;&nbsp;&nbsp;character open
> > 
> > seems to look fine, IMO. On Windows, perhaps Lucida Sans Unicode or Segoe UI
> > Symbol would be suitable.
> > 
> > Rendering as glyphs should in principle be more performant, as well as
> > requiring less code (I hope).
> 
> I don't think it would be more performant, considering that using font would
> require additional steps including looking for font, checking range, reading
> glyph data, considering font features, etc. Here, we just compute three
> points and draw a triangle. Why do you think rendering as glyph would be
> more performant?

Because the glyph will be rasterized once and cached by the font subsystem, rather than an individual polygon being rasterized for every disclosure symbol.

> OTOH, with choosing a suitable font, we still need to handle these
> predefines differently, since we should not choose font for other styles,
> especially custom counter styles. In addition, although we need not compute
> the points, we still have to handle the direction by code in this case. We
> have to decide which glyph should be used according to the writing mode, as
> the glyphs won't flip automatically. Hence most of the current code would
> not be changed. Furthermore, to have proper symbol fonts on all platforms,
> we would have to add a new platform-dependent pref, which again costs code.
> For these reasons, I wonder if it could actually reduce the code size.

OK, you may be right that the other overhead of finding the font, etc., would offset the benefit of glyph caching. I guess it's fine to go with the triangle-drawing method for now.

(I still wonder whether switching all of disc, circle, square and disclosure-closed/open over to use a symbol-font-based approach might be good some day. But let's defer that - and if we do ever explore it as an option, we should actually measure performance to see which approach wins.)
Comment on attachment 8438944 [details] [diff] [review]
patch

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

::: layout/generic/nsBulletFrame.cpp
@@ +360,5 @@
> +
> +      WritingMode wm = GetWritingMode();
> +      bool isVertical = wm.IsVertical();
> +      bool closed =
> +        listStyleType->GetStyle() == NS_STYLE_LIST_STYLE_DISCLOSURE_CLOSED;

For consistency with isVertical and isDown, name this isClosed.

@@ +390,5 @@
> +        } else {
> +          // to left
> +          points[0] = rect.TopRight();
> +          points[1] = (rect.TopLeft() + rect.BottomLeft()) / 2;
> +          points[2] = rect.BottomRight();

Please reorder these points so that the triangle is being described in a clockwise direction, like the two above. Or make the other two counter-clockwise; I don't mind which, but let's be consistent.

::: layout/style/CounterStyleManager.cpp
@@ +673,5 @@
>      case NS_STYLE_LIST_STYLE_SQUARE:
>        aResult.Assign(kSquareCharacter);
> +      break;
> +    case NS_STYLE_LIST_STYLE_DISCLOSURE_CLOSED:
> +      aResult.Assign(kDisclosureClosedCharacter);

This needs to be sensitive to the direction, doesn't it? In RTL, it would be 0x25C2.

(And in vertical writing modes, both closed and open will need to change. But that's optional at this stage, as we're some way from enabling vertical modes as yet. Though I notice you've included support in nsBulletFrame::PaintBullet.)

@@ +903,5 @@
> +      aResult.Assign(kDisclosureClosedCharacter);
> +      return true;
> +    case NS_STYLE_LIST_STYLE_DISCLOSURE_OPEN:
> +      aResult.Assign(kDisclosureOpenCharacter);
> +      return true;

writing mode / direction?

Ah, I see you mentioned this in comment 7 already. Can you pass the writing mode from the caller down through GetSpokenCounterText() and GetCounterText(), and eventually use that to select the appropriate characters?
(In reply to Jonathan Kew (:jfkthame) from comment #16)
> Comment on attachment 8438944 [details] [diff] [review]
> patch
> 
> Review of attachment 8438944 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsBulletFrame.cpp
> @@ +360,5 @@
> > +
> > +      WritingMode wm = GetWritingMode();
> > +      bool isVertical = wm.IsVertical();
> > +      bool closed =
> > +        listStyleType->GetStyle() == NS_STYLE_LIST_STYLE_DISCLOSURE_CLOSED;
> 
> For consistency with isVertical and isDown, name this isClosed.
> 
> @@ +390,5 @@
> > +        } else {
> > +          // to left
> > +          points[0] = rect.TopRight();
> > +          points[1] = (rect.TopLeft() + rect.BottomLeft()) / 2;
> > +          points[2] = rect.BottomRight();
> 
> Please reorder these points so that the triangle is being described in a
> clockwise direction, like the two above. Or make the other two
> counter-clockwise; I don't mind which, but let's be consistent.
> 
> ::: layout/style/CounterStyleManager.cpp
> @@ +673,5 @@
> >      case NS_STYLE_LIST_STYLE_SQUARE:
> >        aResult.Assign(kSquareCharacter);
> > +      break;
> > +    case NS_STYLE_LIST_STYLE_DISCLOSURE_CLOSED:
> > +      aResult.Assign(kDisclosureClosedCharacter);
> 
> This needs to be sensitive to the direction, doesn't it? In RTL, it would be
> 0x25C2.
> 
> (And in vertical writing modes, both closed and open will need to change.
> But that's optional at this stage, as we're some way from enabling vertical
> modes as yet. Though I notice you've included support in
> nsBulletFrame::PaintBullet.)
> 
> @@ +903,5 @@
> > +      aResult.Assign(kDisclosureClosedCharacter);
> > +      return true;
> > +    case NS_STYLE_LIST_STYLE_DISCLOSURE_OPEN:
> > +      aResult.Assign(kDisclosureOpenCharacter);
> > +      return true;
> 
> writing mode / direction?
> 
> Ah, I see you mentioned this in comment 7 already. Can you pass the writing
> mode from the caller down through GetSpokenCounterText() and
> GetCounterText(), and eventually use that to select the appropriate
> characters?

It sounds good to have WritingMode passed down through GetCounterText(), but I wonder whether we should do the same thing for GetSpokenCounterText(). I'm considering just generating the default bullet character for disclosure-*. What's your opinion?
Attached patch patch (obsolete) — Splinter Review
Attachment #8438944 - Attachment is obsolete: true
Attachment #8438944 - Flags: review?(jfkthame)
Attachment #8439681 - Flags: review?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #15)
> OK, you may be right that the other overhead of finding the font, etc.,
> would offset the benefit of glyph caching. I guess it's fine to go with the
> triangle-drawing method for now.
> 
> (I still wonder whether switching all of disc, circle, square and
> disclosure-closed/open over to use a symbol-font-based approach might be
> good some day. But let's defer that - and if we do ever explore it as an
> option, we should actually measure performance to see which approach wins.)

Maybe it's worth having a followup bug for this idea.
(In reply to Xidorn Quan (UTC+10) from comment #17)
> It sounds good to have WritingMode passed down through GetCounterText(), but
> I wonder whether we should do the same thing for GetSpokenCounterText(). I'm
> considering just generating the default bullet character for disclosure-*.
> What's your opinion?

ISTM that the open/closed state is a semantically important piece of information (or at least it could be), and so it seems more appropriate to be able to indicate this in the spoken form. More so (most likely) than the distinction between circle, disc and square, for which we do generate different text.

So unless it's really problematic, I think we should try to handle this.
Attached patch patch (obsolete) — Splinter Review
Make spoken text aware of writing mode.
Attachment #8439681 - Attachment is obsolete: true
Attachment #8439681 - Flags: review?(jfkthame)
Attachment #8439896 - Flags: review?(jfkthame)
Comment on attachment 8439896 [details] [diff] [review]
patch

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

LGTM. :)

::: layout/generic/nsBulletFrame.cpp
@@ +575,5 @@
> +      }
> +      AppendSpacingToPadding(fm);
> +      break;
> +
> +

nit: one blank line is plenty here.
Attachment #8439896 - Flags: review?(jfkthame) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9603399b9ee1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.