Bug 15713 – Outside list bullets ignore text-align style

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: Layout and Rendering

[Nick Santos]

1) Create a div with style='text-align: right'
2) Create an ordered list inside that div.

Expected Result:
List bullets should be aligned right.

Actual Result:
List bullets aligned to the left.

Test here:
http://www.nick-santos.com/tests/list_layout.html

[Dave Hyatt]

Inside list bullets work just fine.

[Julie Parent]

We are getting a lot of complaints from users of Google's editor about this issue.  Would love to see this fixed.

<rdar://problem/6141574>

CSS2.1 says "The marker box is outside the principal block box". Isn't this bug requesting a violation of CSS2.1?

[Dave Hyatt]

It is.  However every other browser honors text-align on outside list bullets.  CSS 2.1 should arguably be amended.  As things stand, however, our rendering is correct.

[Julie Parent]

The reason we care about this is for rich text editing.  Using the built in execCommands, lists always looks wrong when you align them.  You can see this in action in the midas demo: http://www.mozilla.org/editor/midasdemo/.

Example 1:
1. Type "foo<enter>bar<enter>baz"
2. Select all and right align
3. Apply bullets
Resulting HTML is: <div style="text-align: right;"><ul><li>foo<br></li><li>bar<br></li><li>baz<br></li></ul></div>

As reported earlier in this bug, this renders with the bullets on the left and the text on the right.

Example 2:
1. Type "foo<enter>bar<enter>baz"
2. Select all, apply bullets
3. Right align
Reulting HTML is: <ul><li style="text-align: right;">foo<br></li><li style="text-align: right;">bar<br></li><li style="text-align: right;">baz<br></li></ul>

In this case, it also renders with bullets on the left and text on the right, so there is no workaround for the user to get their lists to look correct.

Should we file this under the rich text component instead?

[Dave Hyatt]

Right.  The issue is that Safari's rendering is actually correct according to CSS2.1.  We can't make a fix without being sure that the spec is going to be changed.  I'll bring this up with the CSS working group.

list-style-position:inside would work around the problem BTW.  I don't know if that is an option though.

[Dave Hyatt]

Created attachment 23040 [details]
Demo file

Notice that Firefox makes the marker follow text-align, but it does not make the marker follow text-indent.  This seems inconsistent to me.

[Ojan Vafai]

A couple extra data points in favor of modifying the spec.

list-style-position:inside doesn't really provide a great solution for left-aligned lists as it puts the marker to the right of the padding-left, which is not what you want either.

To make the WYSIWYG experience actually match user expectation, you would need list-style-position:inside and to decrease the padding accordingly. Not sure that's the "right" solution though.

To be clear, IE and Opera have the marker follow both text-align and text-indent. And, as Hyatt said, FF has the marker follow only text-align, which I agree seems inconsistent.

There is (mostly) browser consistency here and the IE/Opera implementation make for a considerably more intuitive WYSIWYG experience without rich-text editors needing to do crazy hackery around browser defaults.

(In reply to comment #8)
> Created an attachment (id=23040) [edit]
> Demo file
> 
> Notice that Firefox makes the marker follow text-align, but it does not make
> the marker follow text-indent.  This seems inconsistent to me.

A weird thing that I noticed about firefox is that if you do
<li><div>text</div></li>
then the marker jumps back outside. However, IE and Opera do not do that.

[Dave Hyatt]

It's obvious to me that we should fix this, even if it violates CSS2.1.

[Dave Hyatt]

Now that we have a clean overflow model that propagates all the way up the line box tree, I think this bug could be fixed easily.

placeBoxesHorizontally should start putting the list marker in the correct spot, and we should just remove all the RenderListItem hackery.

[Renata Hodovan]

Created attachment 65893 [details]
Fix the position of outside list bullets

[Renata Hodovan]

Any feedbacks are welcome :)

[Andreas Kling]

Comment on attachment 65893 [details]
Fix the position of outside list bullets

> [phone]  Renata Hodovan  <[e-mail]>

Address should be "[e-mail]", no?

> +    int getTextAlignOffset() { return m_textAlignOffset; }

This method should be const.

I don't know this code near well enough to comment on the actual changes. It would help greatly if you'd explain what you're doing and why (in the ChangeLog.)

[Renata Hodovan]

Created attachment 68071 [details]
Fix the position of outside list bullets

[Renata Hodovan]

(In reply to comment #15)
> (From update of attachment 65893 [details])
> > [phone]  Renata Hodovan  <[e-mail]>
> 
> Address should be "[e-mail]", no?
Yeah, you're right. I misspelled it.

> > +    int getTextAlignOffset() { return m_textAlignOffset; }
> 
> This method should be const.
Okay.

> I don't know this code near well enough to comment on the actual changes. It would help greatly if you'd explain what you're doing and why (in the ChangeLog.)
I simply saved the start position of the current line by outside markers and this value should be added to the marker's position.
This patch caused one new failed layout test, but it's expected is updated now.

[Early Warning System Bot]

Attachment 68071 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4098004

[Renata Hodovan]

Created attachment 68074 [details]
Fix the position of outside list bullets

[Andreas Kling]

Comment on attachment 68074 [details]
Fix the position of outside list bullets

This looks fairly good to me, I'm not comfortable r+'ing though. Hopefully someone in CC can have a look.

One question: Are you sure m_textAlignOffset can't be used uninitialized in any way? IOW, will InlineFlowBox::placeBoxesHorizontally() always be called before RenderListItem::positionListMarker()?

[Renata Hodovan]

(In reply to comment #20)
> (From update of attachment 68074 [details])
> This looks fairly good to me, I'm not comfortable r+'ing though. Hopefully someone in CC can have a look.
> 
> One question: Are you sure m_textAlignOffset can't be used uninitialized in any way? IOW, will InlineFlowBox::placeBoxesHorizontally() always be called before RenderListItem::positionListMarker()?

I'm not sure but it seems to me that the placeBoxesHorizontally() calculates the outer dimensions of the boxes, so it should run earlier than positionListMarker().

Hyatt? What is your opinion? :)

[Darin Adler]

Comment on attachment 68074 [details]
Fix the position of outside list bullets

View in context: https://bugs.webkit.org/attachment.cgi?id=68074&action=review

Thanks for taking this on.

This change needs to come with a new test case. Simply updating the results of test cases that were intended to test other things is not sufficiently clear in this case.

It’s considered inelegant to have data hanging off an object that’s only used by code outside the class, and this is the case with m_textAlignOffset.

I also think we could come up with a better name for this concept than “text align offset”. What is this value? An offset for aligning text? I’m not sure that’s the best way to describe it.

I’d like to see code that would help us notice and debug if this value was used without ever being set since the flow of control is indirect. Perhaps a special uninitialized value and an assertion in the getter would do the trick.

review- because of insufficient test cases

> WebCore/rendering/InlineFlowBox.cpp:346
> +                RenderListMarker* currMarker = toRenderListMarker(curr->renderer());
> +                currMarker->setTextAlignOffset(xPos);

The local variable is no help here. It would be clearer to have it on one line.

> WebCore/rendering/RenderListMarker.h:47
> +    int getTextAlignOffset() const { return m_textAlignOffset; }

In the WebKit project we don’t use get in the names of getter functions such as this one. It should be named textAlignOffset.

> WebCore/rendering/RenderListMarker.h:84
> +    int m_textAlignOffset;

The set-up here seems fragile. This is not initialized, so the code will only work correctly if InlineFlowBox::placeBoxesHorizontally is called before positionListMarker. Is this guaranteed?

[Renata Hodovan]

Created attachment 71420 [details]
Fix the position of outside list bullets

[Renata Hodovan]

> This change needs to come with a new test case. Simply updating the results of test cases that were intended to test other things is not sufficiently clear in this case.
Added.

> It’s considered inelegant to have data hanging off an object that’s only used by code outside the class, and this is the case with m_textAlignOffset.
> 
> I also think we could come up with a better name for this concept than “text align offset”. What is this value? An offset for aligning text? I’m not sure that’s the best way to describe it.
You're right, it was really not too correct. I renamed it to outsideMarkerOffset.

> I’d like to see code that would help us notice and debug if this value was used without ever being set since the flow of control is indirect. Perhaps a special uninitialized value and an assertion in the getter would do the trick.
It's done. First outsideMarkerOffsets value is INT_MAX and an assertion checks it in getter.

[Andreas Kling]

Comment on attachment 71420 [details]
Fix the position of outside list bullets

View in context: https://bugs.webkit.org/attachment.cgi?id=71420&action=review

> WebCore/ChangeLog:8
> +        Fix the outside markers position with saving the start position of the current line and adding this value to the markers postion.

s/markers/marker's/g
s/with/by/

> WebCore/rendering/RenderListMarker.h:48
> +    const int outsideMarkerOffset() const 

Unnecessary const, should be "int outsideMarkerOffset() const"

> LayoutTests/ChangeLog:5
> +        Need a short description and bug URL (OOPS!)

Oops, indeed!

> LayoutTests/fast/lists/outSideListMarkers.html:20
> +<p><u>text-align: rigth</u></p>

s/rigth/right/

[Andreas Kling]

Also, doesn't this still affect editing/pasteboard/paste-4039777-fix.html? A new baseline is missing from the latest patch.

[Early Warning System Bot]

Attachment 71420 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4690011

[Renata Hodovan]

Created attachment 71504 [details]
Fix the position of outside list bullets

[Renata Hodovan]

(In reply to comment #26)
> Also, doesn't this still affect editing/pasteboard/paste-4039777-fix.html? A new baseline is missing from the latest patch.
Yeah, it was missing :$ Sorry...

[Early Warning System Bot]

Attachment 71504 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4659015

[Renata Hodovan]

Created attachment 71531 [details]
Fix the position of outside list bullets

[Renata Hodovan]

Created attachment 71562 [details]
Fix the position of outside list bullets

[Andreas Kling]

Comment on attachment 71562 [details]
Fix the position of outside list bullets

I took this patch for a spin, and it broke the following tests on Snow Leopard:

css1/box_properties/border_left.html
css1/box_properties/margin_right.html
css1/box_properties/padding_left.html
css1/box_properties/padding_right.html
css2.1/t0805-c5521-brdr-l-02-e.html
editing/selection/extend-by-word-002.html
fast/css/background-shorthand-invalid-url.html
fast/css/empty-pseudo-class.html
fast/css/first-child-pseudo-class.html
fast/css/first-of-type-pseudo-class.html
fast/css/last-child-pseudo-class.html
fast/css/last-of-type-pseudo-class.html
fast/css/list-outline.html
fast/css/only-child-pseudo-class.html
fast/css/only-of-type-pseudo-class.html
fast/lists/001.html
fast/lists/002.html
fast/lists/003.html
fast/lists/007.html
fast/lists/008.html
fast/lists/ordered-list-with-no-ol-tag.html

Some of them are definitely progressions, but please look through them and rebaseline as appropriate in the next patch iteration.

I uploaded the results here for your convenience:
http://chaos.troll.no/~kling/15713/layout-test-results/results.html

[Renata Hodovan]

Created attachment 74023 [details]
Patch

[Andreas Kling]

Comment on attachment 74023 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74023&action=review

You asked me to look at this, so here are some (somewhat superficial) comments.
Overall I think you need to get better at explaining your changes.

> WebCore/ChangeLog:9
> +        Fix the outside marker's position by setting a suitable value to its logicalLeft member. For handling embedded markers correctly
> +        a loop is added which determines these marker's width. Some variables become unnecessary and they are removed from the code.

Setting it to "a suitable value" is too unspecific.
You can leave out the sentence about unnecessary variables.

> WebCore/rendering/InlineFlowBox.cpp:276
> +        if (curr->renderer()->isListMarker() && !toRenderListMarker(curr->renderer())->isInside())
> +            markerWidth += toRenderListMarker(curr->renderer())->width() - toRenderListMarker(curr->renderer())->marginLeft();

IMO this would be clearer like so:

if (!curr->renderer()->isListMarker())
    continue;
RenderListMarker* marker = toRenderListMarker(curr->renderer());
if (marker->isInside())
    markerWidth += marker->width() - marker->marginLeft();

> WebCore/rendering/InlineFlowBox.cpp:356
> +                markerWidth -= (toRenderListMarker(curr->renderer())->width() - toRenderListMarker(curr->renderer())->marginLeft());

Same as above:

RenderListMarker* marker = toRenderListMarker(curr->renderer());
markerWidth -= marker->width() - marker->marginLeft();

> WebCore/rendering/RenderListItem.cpp:267
> +            if (!style()->isHorizontalWritingMode())
> +                markerLogicalLeft += paddingStart();

I don't understand this part.

[Renata Hodovan]

Created attachment 74391 [details]
Patch

[Andreas Kling]

Comment on attachment 74391 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 74391 [details]
Patch

Rejecting patch 74391 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 74391]" exit_code: 2
Last 500 characters of output:
t/lists/007-vertical-expected.checksum
patching file LayoutTests/platform/mac/fast/lists/007-vertical-expected.txt
patching file LayoutTests/platform/mac/fast/lists/008-vertical-expected.checksum
patching file LayoutTests/platform/mac/fast/lists/008-vertical-expected.txt
patching file LayoutTests/platform/mac/fast/lists/outSideListMarkers-expected.checksum

Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Andreas Kling', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/6223111

[Renata Hodovan]

This is landed in <http://trac.webkit.org/changeset/72527>
Closing bug.

[Dave Hyatt]

Reopening.

This patch had a few problems:

(1) It broke vertical outside list bullets.  I'm not sure why you thought the math for vertical bullets had to be different.  It should be identical to horizontal.

(2) It broke nested blocks inside list items when the blocks had borders/padding.  You can't just discard lineOffset.  Even respecting text-align, this was breakage that caused us to not match other browsers.

(3) It clearly violates the CSS2.1 Candidate Recommendation.  Now I am sympathetic to the compatibility argument, but let's see if we can get some traction in the Working Group.  I'm very reluctant (in standards mode at least... quirks mode is another story) to just deliberately check in a patch that contradicts the CSS2.1 text.

I sent the following email to try to get discussion going on this:

http://lists.w3.org/Archives/Public/www-style/2010Nov/0355.html

[Csaba Osztrogonác]

Original commits -- rollout commits
------------------------------------
http://trac.webkit.org/changeset/72527 - http://trac.webkit.org/changeset/72582
http://trac.webkit.org/changeset/72529 - http://trac.webkit.org/changeset/72583
http://trac.webkit.org/changeset/72530 - http://trac.webkit.org/changeset/72589
http://trac.webkit.org/changeset/72533 - not rolled out yet!!!
http://trac.webkit.org/changeset/72544 - http://trac.webkit.org/changeset/72589


I think http://trac.webkit.org/changeset/72533 should be rolled out too,because http://trac.webkit.org/changeset/72527 was rolled out. But I'm not sure if
the whole r72533 is related to r72527. Anton, could you check it, please?

[WebKit Review Bot]

http://trac.webkit.org/changeset/72589 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
inspector/styles-source-lines-inline.html

[anton muhin]

(In reply to comment #41)
> Original commits -- rollout commits
> ------------------------------------
> http://trac.webkit.org/changeset/72527 - http://trac.webkit.org/changeset/72582
> http://trac.webkit.org/changeset/72529 - http://trac.webkit.org/changeset/72583
> http://trac.webkit.org/changeset/72530 - http://trac.webkit.org/changeset/72589
> http://trac.webkit.org/changeset/72533 - not rolled out yet!!!
> http://trac.webkit.org/changeset/72544 - http://trac.webkit.org/changeset/72589
> 
> 
> I think http://trac.webkit.org/changeset/72533 should be rolled out too,because http://trac.webkit.org/changeset/72527 was rolled out. But I'm not sure if
> the whole r72533 is related to r72527. Anton, could you check it, please?

Csaba, thanks a lot for adding into the loop.  llya ([e-mail]) and me are rolling rebaselines out.

Anton, does this http://trac.webkit.org/changeset/72599 change rollout ttp://trac.webkit.org/changeset/72533 ? I think it's incomplete, though.

It only rollouted the checksum files, while all png files were left modified. This could make layout test pass, because when we run these tests on chromium,
as long as the checksum matches, they pass without generate image diff. So the problem is not visible on chromium now.
(In reply to comment #43)
> (In reply to comment #41)
> > Original commits -- rollout commits
> > ------------------------------------
> > http://trac.webkit.org/changeset/72527 - http://trac.webkit.org/changeset/72582
> > http://trac.webkit.org/changeset/72529 - http://trac.webkit.org/changeset/72583
> > http://trac.webkit.org/changeset/72530 - http://trac.webkit.org/changeset/72589
> > http://trac.webkit.org/changeset/72533 - not rolled out yet!!!
> > http://trac.webkit.org/changeset/72544 - http://trac.webkit.org/changeset/72589
> > 
> > 
> > I think http://trac.webkit.org/changeset/72533 should be rolled out too,because http://trac.webkit.org/changeset/72527 was rolled out. But I'm not sure if
> > the whole r72533 is related to r72527. Anton, could you check it, please?
> 
> Csaba, thanks a lot for adding into the loop.  llya ([e-mail]) and me are rolling rebaselines out.

[anton muhin]

That pretty much should be the case.  In any event I used standard rebaseline tool, so this patch is to recreate at any convenient moment.

(In reply to comment #44)
> Anton, does this http://trac.webkit.org/changeset/72599 change rollout ttp://trac.webkit.org/changeset/72533 ? I think it's incomplete, though.
> 
> It only rollouted the checksum files, while all png files were left modified. This could make layout test pass, because when we run these tests on chromium,
> as long as the checksum matches, they pass without generate image diff. So the problem is not visible on chromium now.
> (In reply to comment #43)
> > (In reply to comment #41)
> > > Original commits -- rollout commits
> > > ------------------------------------
> > > http://trac.webkit.org/changeset/72527 - http://trac.webkit.org/changeset/72582
> > > http://trac.webkit.org/changeset/72529 - http://trac.webkit.org/changeset/72583
> > > http://trac.webkit.org/changeset/72530 - http://trac.webkit.org/changeset/72589
> > > http://trac.webkit.org/changeset/72533 - not rolled out yet!!!
> > > http://trac.webkit.org/changeset/72544 - http://trac.webkit.org/changeset/72589
> > > 
> > > 
> > > I think http://trac.webkit.org/changeset/72533 should be rolled out too,because http://trac.webkit.org/changeset/72527 was rolled out. But I'm not sure if
> > > the whole r72533 is related to r72527. Anton, could you check it, please?
> > 
> > Csaba, thanks a lot for adding into the loop.  llya ([e-mail]) and me are rolling rebaselines out.

Created attachment 86310 [details]
Rollout some png files of r72533 to complete r72599.

[WebKit Review Bot]

Attachment 86310 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

LayoutTests/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 1 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 86312 [details]
Change the style of patch.

[WebKit Commit Bot]

Comment on attachment 86312 [details]
Change the style of patch.

Clearing flags on attachment: 86312

Committed r81579: <http://trac.webkit.org/changeset/81579>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Piotrek Koszuliński (Reinmar)]

This issue can be reproduced again: http://jsfiddle.net/bufzfktn/

Was this patch reverted for some reasons?

[Csaba Osztrogonác]

(In reply to comment #51)
> This issue can be reproduced again: http://jsfiddle.net/bufzfktn/
> 
> Was this patch reverted for some reasons?

The original fix was landed in http://trac.webkit.org/changeset/72527 and 
was reverted by http://trac.webkit.org/changeset/72582 only one day later.

But you can see everything about the reason in this bug report.

[Piotrek Koszuliński (Reinmar)]

I don't understand. The patch has been reverted because the patch was buggy. But why haven't been the ticket reopened? I think it was agreed that the issue is valid.

Can someone please reopen this ticket? The patch has been reverted, the bug is still there and issue hasn't been resolved.

Reverted in r72582.

[Ahmad Saleem]

I am able to reproduce this bug in Safari 15.5 on macOS 12.4 (it is matching with Chrome Canary 105) using attached demo file and also JSFiddle mentioned in Comment 51 but differs from the Firefox Nightly.

I updated JSFiddle from Comment 51 to add "center" case as well - Link - https://jsfiddle.net/t8gvc97b/

In the above, Safari 15.5 matches with Chrome but differs from Firefox by showing "bullet" on the far left side rather than next to text. Thanks!

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #56)
> I am able to reproduce this bug in Safari 15.5 on macOS 12.4 (it is matching
> with Chrome Canary 105) using attached demo file and also JSFiddle mentioned
> in Comment 51 but differs from the Firefox Nightly.
> 
> I updated JSFiddle from Comment 51 to add "center" case as well - Link -
> https://jsfiddle.net/t8gvc97b/
> 
> In the above, Safari 15.5 matches with Chrome but differs from Firefox by
> showing "bullet" on the far left side rather than next to text. Thanks!

Safari Technology Preview 195, Chrome Canary 127 and Firefox Nightly 128 are matching with each other on above JSFiddle and also on `Demo` file, so I think this has been fixed now.

I am marking this as 'RESOLVED CONFIGURATION CHANGED' but please reopen if it is still not fixed.