1013160 - Use space character instead of padding in bullet

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

According to the decision from CSSWG [1] and the updated spec, we should remove the padding suppressing mechanism introduced in bug 934072, and simply append space to the suffixes.

[1]: http://lists.w3.org/Archives/Public/www-style/2014May/0210.html

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

We may need to add full support of bidirection text in bullet frame. Are there any utils for conveniently doing width estimation and rendering for multiple pieces of text with different direction?

Comment 2

[Simon Montagu :smontagu]

(In reply to Xidorn Quan from comment #1)
> We may need to add full support of bidirection text in bullet frame. Are
> there any utils for conveniently doing width estimation and rendering for
> multiple pieces of text with different direction?

Yes, nsBidiPresUtils::RenderText and nsLayoutUtils::GetStringWidth (which calls nsBidiPresUtils::MeasureTextWidth when necessary).

A good example is nsImageFrame::DisplayAltText at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp#989

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

try: https://tbpl.mozilla.org/?tree=Try&rev=a0d6d83cf96c

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8426247 [details] [diff] [review]
patch

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

::: layout/generic/nsBulletFrame.cpp
@@ +1504,2 @@
>  
> +  nsString suffix;

Shouldn't this be kept as an nsAutoString, to avoid an extra allocation?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> Comment on attachment 8426247 [details] [diff] [review]
> patch
> 
> Review of attachment 8426247 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsBulletFrame.cpp
> @@ +1504,2 @@
> >  
> > +  nsString suffix;
> 
> Shouldn't this be kept as an nsAutoString, to avoid an extra allocation?

You are right, though mostly no extra allocation should occur as most suffixes are only reference to literal string.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

This patch fixes some problems revealed by the tests. OTOH, as the change generates a space for graphic bullets, it breaks many a11y tests. I have tried my best to fix them.

The only left problem is line boundary. I believe it worthes a followup bug, as it doesn't work as expected for ol as well.

David Bolter, could you please review the changes to a11y tests?

try: https://tbpl.mozilla.org/?tree=Try&rev=9cfeba53cf7a

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8427738 [details] [diff] [review]
patch

I think Alex should check these.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 8427738 [details] [diff] [review]
patch

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

::: accessible/tests/mochitest/text/test_lineboundary.html
@@ +118,5 @@
>        // list items
>  
>        testTextAtOffset([ "li1" ], BOUNDARY_LINE_START,
> +                       [ [ 0, 1, kDiscBulletText + "Item", 0, 6 ],
> +                         [ 2, 6, "Item", 2, 6] ]);

it's something completely wrong here (and below), list items are supposed to be one line in this test

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

(In reply to alexander :surkov from comment #9)
> Comment on attachment 8427738 [details] [diff] [review]
> patch
> 
> Review of attachment 8427738 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/tests/mochitest/text/test_lineboundary.html
> @@ +118,5 @@
> >        // list items
> >  
> >        testTextAtOffset([ "li1" ], BOUNDARY_LINE_START,
> > +                       [ [ 0, 1, kDiscBulletText + "Item", 0, 6 ],
> > +                         [ 2, 6, "Item", 2, 6] ]);
> 
> it's something completely wrong here (and below), list items are supposed to
> be one line in this test

Right, but this problem has been existed for any type other than graphic ones. If you change the ul to ol, it will have the same behavior that "1.Item" for offset 0-1 and "Item" for offset 2-6. So should we mark all of those tests as TODO?

Comment 11

[alexander :surkov (:asurkov)]

either way it's a regression. what's the difference between them?

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

This patch is based on the patch I submitted for bug 1015550.

tryserver: https://tbpl.mozilla.org/?tree=Try&rev=6326960332c8

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

The only related problem in in the tryserver result is R9 of B2G. I don't know why it has such problem. Should I just mark it fails-if(B2G)?

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan from comment #13)
> The only related problem in in the tryserver result is R9 of B2G. I don't
> know why it has such problem. Should I just mark it fails-if(B2G)?

No; the test is fragile, and we should see if we can adjust it to be more robust.

You can reproduce the failure on desktop if you install the Fira Sans OT font and use this as the browser's default, so that you get the same metrics as B2G.

The problem seems to occur because with Fira, the width of the float containing "<li>FLOAT</li>" is less than with typical desktop default fonts, and as a result the "2." from the following item manages to fit next to it instead of being pushed below.

A possible fix is to make the padding-left of <ol> larger (e.g. 60px instead of 40px), and correspondingly change the occurrence of margin-left:-40px in the reference to -60px.

I wonder, though, whether this highlights something else: should we be using a non-breaking space in the bullet suffix, rather than a regular (breakable) space? It seems to me that line-breaking between the bullet and the text of the item is undesirable in general. WDYT?

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #14)
> (In reply to Xidorn Quan from comment #13)
> > The only related problem in in the tryserver result is R9 of B2G. I don't
> > know why it has such problem. Should I just mark it fails-if(B2G)?
> 
> No; the test is fragile, and we should see if we can adjust it to be more
> robust.
> 
> You can reproduce the failure on desktop if you install the Fira Sans OT
> font and use this as the browser's default, so that you get the same metrics
> as B2G.
> 
> The problem seems to occur because with Fira, the width of the float
> containing "<li>FLOAT</li>" is less than with typical desktop default fonts,
> and as a result the "2." from the following item manages to fit next to it
> instead of being pushed below.
> 
> A possible fix is to make the padding-left of <ol> larger (e.g. 60px instead
> of 40px), and correspondingly change the occurrence of margin-left:-40px in
> the reference to -60px.

OK, I'll fix it this way.

> I wonder, though, whether this highlights something else: should we be using
> a non-breaking space in the bullet suffix, rather than a regular (breakable)
> space? It seems to me that line-breaking between the bullet and the text of
> the item is undesirable in general. WDYT?

You may want to discuss it in the CSSWG. the space character U+0020 is specified by the spec. They might agree us to choose a different space (say, U+00A0) as separator for predefined styles, but we cannot control what author uses.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #14)
> (In reply to Xidorn Quan from comment #13)
> > The only related problem in in the tryserver result is R9 of B2G. I don't
> > know why it has such problem. Should I just mark it fails-if(B2G)?
> 
> No; the test is fragile, and we should see if we can adjust it to be more
> robust.
> 
> You can reproduce the failure on desktop if you install the Fira Sans OT
> font and use this as the browser's default, so that you get the same metrics
> as B2G.
> 
> The problem seems to occur because with Fira, the width of the float
> containing "<li>FLOAT</li>" is less than with typical desktop default fonts,
> and as a result the "2." from the following item manages to fit next to it
> instead of being pushed below.
> 
> A possible fix is to make the padding-left of <ol> larger (e.g. 60px instead
> of 40px), and correspondingly change the occurrence of margin-left:-40px in
> the reference to -60px.

This change fails to fix this test.

I finally fix it by adding "width:200px" to rule of li.

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 8428221 [details] [diff] [review]
patch

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

::: accessible/tests/mochitest/text/test_lineboundary.html
@@ +117,5 @@
>        //////////////////////////////////////////////////////////////////////////
>        // list items
>  
>        testTextAtOffset([ "li1" ], BOUNDARY_LINE_START,
> +                       [ [ 0, 6, kDiscBulletText + "Item", 0, 6 ] ]);

I don't get how it's supposed to work if you don't fit kDiscBulletChar value

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

(In reply to alexander :surkov from comment #17)
> Comment on attachment 8428221 [details] [diff] [review]
> patch
> 
> Review of attachment 8428221 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/tests/mochitest/text/test_lineboundary.html
> @@ +117,5 @@
> >        //////////////////////////////////////////////////////////////////////////
> >        // list items
> >  
> >        testTextAtOffset([ "li1" ], BOUNDARY_LINE_START,
> > +                       [ [ 0, 6, kDiscBulletText + "Item", 0, 6 ] ]);
> 
> I don't get how it's supposed to work if you don't fit kDiscBulletChar value

What did you mean by "don't fit kDiscBulletChar value"?

It is changed to kDiscBulletText because all existing bullet will have a space appended as suffix when generating the content. It is specified by the spec CSS Counter Style 3.

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 8428221 [details] [diff] [review]
patch

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

I misread kDiscBulletChar with kDiscBulletText, do you still need to have kDiscBulletChar after the change?

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

(In reply to alexander :surkov from comment #19)
> Comment on attachment 8428221 [details] [diff] [review]
> patch
> 
> Review of attachment 8428221 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I misread kDiscBulletChar with kDiscBulletText, do you still need to have
> kDiscBulletChar after the change?

Though kDiscBulletChar is useless now, I suggest that we can keep it as it is, since the disc char and the space is generated in different phases: the disc char is the counter representation while the space is suffix.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch, r=surkov

Comment 22

[Jonathan Kew [:jfkthame]]

Comment on attachment 8429701 [details] [diff] [review]
patch, r=surkov

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

Looks OK to me, thanks. One minor suggestion below:

::: layout/generic/nsBlockFrame.cpp
@@ +6568,5 @@
>    const nsStyleList* myList = StyleList();
>    if (myList->GetListStyleImage() ||
>        myList->mListStyleType == NS_STYLE_LIST_STYLE_DISC) {
>      aText.Assign(kDiscCharacter);
> +    aText.Append(' ');

How about avoiding the two-step construction of the string here? Something like this ought to work, AFAICS:

  static const char16_t kDiscString[] = { kDiscCharacter, ' ', 0 };
  aText.Assign(nsDependentString(kDiscString, 2));

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #22)
> Comment on attachment 8429701 [details] [diff] [review]
> patch, r=surkov
> 
> Review of attachment 8429701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks OK to me, thanks. One minor suggestion below:
> 
> ::: layout/generic/nsBlockFrame.cpp
> @@ +6568,5 @@
> >    const nsStyleList* myList = StyleList();
> >    if (myList->GetListStyleImage() ||
> >        myList->mListStyleType == NS_STYLE_LIST_STYLE_DISC) {
> >      aText.Assign(kDiscCharacter);
> > +    aText.Append(' ');
> 
> How about avoiding the two-step construction of the string here? Something
> like this ought to work, AFAICS:
> 
>   static const char16_t kDiscString[] = { kDiscCharacter, ' ', 0 };
>   aText.Assign(nsDependentString(kDiscString, 2));

I don't think it is useful enough here. Logically, the two characters are from different part, so I'd prefer not to change it. In addition, in the counter-style patches, most of the code here will be replaced. After that replacement, only list-style-image continues using this code, and I wonder we could remove the additional space then, as the spec say nothing about what should be generated for image bullet.

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

tryserver: https://tbpl.mozilla.org/?tree=Try&rev=8242da1ac425

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bd21ab86f78

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Please stop merging this into the trunck due to bug 1017335.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #26)
> Please stop merging this into the trunck due to bug 1017335.

A bug comment won't stop it getting merged; it would need to be backed out from inbound.  Should we do that?

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/71a7a25d9884

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch, r=surkov

This patch solves bug 1017335 in addition.

In this patch, 0.5em is used instead of space width since it is the value we currently use, and the spec doesn't specify the spacing for generated image.

Comment 30

[Jonathan Kew [:jfkthame]]

Comment on attachment 8430702 [details] [diff] [review]
patch, r=surkov

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

::: layout/generic/nsBulletFrame.cpp
@@ +1594,5 @@
> +      // Add spacing to the padding
> +      const nsStyleVisibility* vis = StyleVisibility();
> +      nscoord halfEm = fm->EmHeight() / 2;
> +      if (GetWritingMode().IsVertical()) {
> +        mPadding.bottom += halfEm;

Cool! :) We don't actually support vertical writing-mode yet, but we're working on it - so sure, why not include this here...

@@ +1599,5 @@
> +      } else if (vis->mDirection == NS_STYLE_DIRECTION_RTL) {
> +        mPadding.left += halfEm;
> +      } else {
> +        mPadding.right += halfEm;
> +      }

...in which case I think the "modern" way to write this would be to base it all on the WritingMode:

  WritingMode wm = GetWritingMode();
  if (wm.IsVertical()) ... else if (wm.IsBidiLTR()) ... else ...

But I don't mind whether you change it now or not...we'll end up looking at all this again during the writing-modes conversion anyhow.

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch, r=jfkthame,surkov

Use WritingMode for text direction as well. In addition, GetListItemText is changed to use the same method.

Comment 32

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f7ccdfe2c6e

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1f7ccdfe2c6e

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Hmmm... After I review my patches for bug 966166, I found I shouldn't have added the space to bullets here...