Use space character instead of padding in bullet

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks 2 bugs, {css3})

Trunk
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Assignee: nobody → quanxunzhen
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Keywords: css3
(Assignee)

Comment 1

5 years ago
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?
Flags: needinfo?(jfkthame)
(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
Flags: needinfo?(jfkthame)
(Assignee)

Comment 3

5 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8426247 - Flags: review?(jfkthame)
(Assignee)

Updated

5 years ago
Summary: Remove padding suppressing mechanism of bullet → Use space character instead of padding in bullet
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?
(Assignee)

Comment 6

5 years ago
(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.
Attachment #8426247 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 7

5 years ago
Posted patch patch (obsolete) — Splinter Review
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
Attachment #8426247 - Attachment is obsolete: true
Attachment #8427738 - Flags: review?(jfkthame)
Attachment #8427738 - Flags: review?(dbolter)
Comment on attachment 8427738 [details] [diff] [review]
patch

I think Alex should check these.
Attachment #8427738 - Flags: review?(dbolter) → review?(surkov.alexander)
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
Attachment #8427738 - Flags: review?(surkov.alexander) → review-
(Assignee)

Comment 10

5 years ago
(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?
(Assignee)

Updated

5 years ago
Flags: needinfo?(surkov.alexander)
either way it's a regression. what's the difference between them?
Flags: needinfo?(surkov.alexander)
(Assignee)

Updated

5 years ago
Depends on: 1015550
(Assignee)

Comment 12

5 years ago
Posted patch patch (obsolete) — Splinter Review
This patch is based on the patch I submitted for bug 1015550.

tryserver: https://tbpl.mozilla.org/?tree=Try&rev=6326960332c8
Attachment #8427738 - Attachment is obsolete: true
Attachment #8427738 - Flags: review?(jfkthame)
Attachment #8428221 - Flags: review?(surkov.alexander)
Attachment #8428221 - Flags: review?(jfkthame)
(Assignee)

Comment 13

5 years ago
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)?
(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?
(Assignee)

Comment 15

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
(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 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
(Assignee)

Comment 18

5 years ago
(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 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?
Attachment #8428221 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 20

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
Posted patch patch, r=surkov (obsolete) — Splinter Review
Attachment #8428221 - Attachment is obsolete: true
Attachment #8428221 - Flags: review?(jfkthame)
Attachment #8429701 - Flags: review?(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));
Attachment #8429701 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 23

5 years ago
(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.
(Assignee)

Comment 26

5 years ago
Please stop merging this into the trunck due to bug 1017335.
(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?
Depends on: 1017335
(Assignee)

Updated

5 years ago
No longer depends on: 1017335
(Assignee)

Updated

5 years ago
Depends on: 1017335
(Assignee)

Comment 29

5 years ago
Posted patch patch, r=surkov (obsolete) — Splinter Review
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.
Attachment #8429701 - Attachment is obsolete: true
Attachment #8430702 - Flags: review?(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.
Attachment #8430702 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 31

5 years ago
Use WritingMode for text direction as well. In addition, GetListItemText is changed to use the same method.
Attachment #8430702 - Attachment is obsolete: true
Attachment #8431228 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1f7ccdfe2c6e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 34

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

Updated

5 years ago
Depends on: 1019470
Depends on: 1020087
No longer depends on: 1021238

Updated

5 years ago
Blocks: 990318

Updated

4 years ago
Depends on: 1142369
You need to log in before you can comment on or make changes to this bug.