Bug 15713 - Outside list bullets ignore text-align style
Summary: Outside list bullets ignore text-align style
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Renata Hodovan
URL: http://www.nick-santos.com/tests/list...
Keywords: GoogleBug, HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2007-10-26 12:07 PDT by Nick Santos
Modified: 2022-07-12 20:25 PDT (History)
28 users (show)

See Also:


Attachments
Demo file (499 bytes, text/html)
2008-08-27 14:26 PDT, Dave Hyatt
no flags Details
Fix the position of outside list bullets (60.04 KB, patch)
2010-08-30 02:22 PDT, Renata Hodovan
no flags Details | Formatted Diff | Diff
Fix the position of outside list bullets (62.90 KB, patch)
2010-09-20 05:06 PDT, Renata Hodovan
no flags Details | Formatted Diff | Diff
Fix the position of outside list bullets (62.90 KB, patch)
2010-09-20 06:16 PDT, Renata Hodovan
darin: review-
Details | Formatted Diff | Diff
Fix the position of outside list bullets (9.08 KB, patch)
2010-10-21 04:07 PDT, Renata Hodovan
kling: review-
Details | Formatted Diff | Diff
Fix the position of outside list bullets (11.76 KB, patch)
2010-10-21 15:44 PDT, Renata Hodovan
no flags Details | Formatted Diff | Diff
Fix the position of outside list bullets (11.87 KB, patch)
2010-10-22 00:31 PDT, Renata Hodovan
no flags Details | Formatted Diff | Diff
Fix the position of outside list bullets (9.16 KB, patch)
2010-10-22 06:42 PDT, Renata Hodovan
kling: review-
Details | Formatted Diff | Diff
Patch (1.58 MB, patch)
2010-11-16 12:27 PST, Renata Hodovan
no flags Details | Formatted Diff | Diff
Patch (1.65 MB, patch)
2010-11-19 08:15 PST, Renata Hodovan
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Rollout some png files of r72533 to complete r72599. (671.43 KB, patch)
2011-03-21 05:58 PDT, zhenghao
no flags Details | Formatted Diff | Diff
Change the style of patch. (671.46 KB, patch)
2011-03-21 06:07 PDT, zhenghao
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Santos 2007-10-26 12:07:41 PDT
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
Comment 1 Dave Hyatt 2007-10-27 02:25:26 PDT
Inside list bullets work just fine.

Comment 2 Julie Parent 2008-08-11 13:54:20 PDT
We are getting a lot of complaints from users of Google's editor about this issue.  Would love to see this fixed.
Comment 3 mitz 2008-08-11 14:26:58 PDT
<rdar://problem/6141574>
Comment 4 mitz 2008-08-27 13:57:21 PDT
CSS2.1 says "The marker box is outside the principal block box". Isn't this bug requesting a violation of CSS2.1?
Comment 5 Dave Hyatt 2008-08-27 14:04:18 PDT
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.
Comment 6 Julie Parent 2008-08-27 14:18:00 PDT
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?
Comment 7 Dave Hyatt 2008-08-27 14:20:02 PDT
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.

Comment 8 Dave Hyatt 2008-08-27 14:26:32 PDT
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.
Comment 9 Ojan Vafai 2008-08-27 14:55:01 PDT
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.
Comment 10 mitz 2008-08-27 16:21:18 PDT
(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.
Comment 11 Dave Hyatt 2008-10-06 11:59:33 PDT
It's obvious to me that we should fix this, even if it violates CSS2.1.
Comment 12 Dave Hyatt 2009-09-14 13:39:52 PDT
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.
Comment 13 Renata Hodovan 2010-08-30 02:22:56 PDT
Created attachment 65893 [details]
Fix the position of outside list bullets
Comment 14 Renata Hodovan 2010-09-07 01:20:10 PDT
Any feedbacks are welcome :)
Comment 15 Andreas Kling 2010-09-17 05:54:52 PDT
Comment on attachment 65893 [details]
Fix the position of outside list bullets

> +2010-08-30  Renata Hodovan  <reni@inf.u-szged.hu>

Address should be "reni@inf.u-szeged.hu", 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.)
Comment 16 Renata Hodovan 2010-09-20 05:06:00 PDT
Created attachment 68071 [details]
Fix the position of outside list bullets
Comment 17 Renata Hodovan 2010-09-20 05:07:58 PDT
(In reply to comment #15)
> (From update of attachment 65893 [details])
> > +2010-08-30  Renata Hodovan  <reni@inf.u-szged.hu>
> 
> Address should be "reni@inf.u-szeged.hu", 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.
Comment 18 Early Warning System Bot 2010-09-20 05:13:14 PDT
Attachment 68071 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4098004
Comment 19 Renata Hodovan 2010-09-20 06:16:04 PDT
Created attachment 68074 [details]
Fix the position of outside list bullets
Comment 20 Andreas Kling 2010-09-23 09:15:02 PDT
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()?
Comment 21 Renata Hodovan 2010-10-13 00:19:30 PDT
(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? :)
Comment 22 Darin Adler 2010-10-13 18:14:57 PDT
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?
Comment 23 Renata Hodovan 2010-10-21 04:07:55 PDT
Created attachment 71420 [details]
Fix the position of outside list bullets
Comment 24 Renata Hodovan 2010-10-21 04:51:52 PDT
> 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.
Comment 25 Andreas Kling 2010-10-21 04:53:56 PDT
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/
Comment 26 Andreas Kling 2010-10-21 04:55:29 PDT
Also, doesn't this still affect editing/pasteboard/paste-4039777-fix.html? A new baseline is missing from the latest patch.
Comment 27 Early Warning System Bot 2010-10-21 05:13:21 PDT
Attachment 71420 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4690011
Comment 28 Renata Hodovan 2010-10-21 15:44:49 PDT
Created attachment 71504 [details]
Fix the position of outside list bullets
Comment 29 Renata Hodovan 2010-10-21 15:47:23 PDT
(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...
Comment 30 Early Warning System Bot 2010-10-21 16:26:49 PDT
Attachment 71504 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4659015
Comment 31 Renata Hodovan 2010-10-22 00:31:15 PDT
Created attachment 71531 [details]
Fix the position of outside list bullets
Comment 32 Renata Hodovan 2010-10-22 06:42:36 PDT
Created attachment 71562 [details]
Fix the position of outside list bullets
Comment 33 Andreas Kling 2010-11-01 16:51:37 PDT
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
Comment 34 Renata Hodovan 2010-11-16 12:27:21 PST
Created attachment 74023 [details]
Patch
Comment 35 Andreas Kling 2010-11-17 02:07:17 PST
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.
Comment 36 Renata Hodovan 2010-11-19 08:15:43 PST
Created attachment 74391 [details]
Patch
Comment 37 Andreas Kling 2010-11-19 08:23:05 PST
Comment on attachment 74391 [details]
Patch

r=me
Comment 38 WebKit Commit Bot 2010-11-22 03:46:22 PST
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
Comment 39 Renata Hodovan 2010-11-22 10:04:41 PST
This is landed in <http://trac.webkit.org/changeset/72527>
Closing bug.
Comment 40 Dave Hyatt 2010-11-22 21:50:00 PST
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
Comment 41 Csaba Osztrogonác 2010-11-23 00:57:05 PST
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?
Comment 42 WebKit Review Bot 2010-11-23 01:27:28 PST
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
Comment 43 anton muhin 2010-11-23 05:21:13 PST
(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 (loislo@chromium.org) and me are rolling rebaselines out.
Comment 44 zhenghao 2011-03-03 00:25:20 PST
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 (loislo@chromium.org) and me are rolling rebaselines out.
Comment 45 anton muhin 2011-03-03 05:51:50 PST
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 (loislo@chromium.org) and me are rolling rebaselines out.
Comment 46 zhenghao 2011-03-21 05:58:37 PDT
Created attachment 86310 [details]
Rollout some png files of r72533 to complete r72599.
Comment 47 WebKit Review Bot 2011-03-21 06:01:25 PDT
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.
Comment 48 zhenghao 2011-03-21 06:07:36 PDT
Created attachment 86312 [details]
Change the style of patch.
Comment 49 WebKit Commit Bot 2011-03-21 07:36:58 PDT
Comment on attachment 86312 [details]
Change the style of patch.

Clearing flags on attachment: 86312

Committed r81579: <http://trac.webkit.org/changeset/81579>
Comment 50 WebKit Commit Bot 2011-03-21 07:37:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 51 Piotrek Koszuliński (Reinmar) 2015-12-07 05:20:49 PST
This issue can be reproduced again: http://jsfiddle.net/bufzfktn/

Was this patch reverted for some reasons?
Comment 52 Csaba Osztrogonác 2015-12-07 05:28:42 PST
(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.
Comment 53 Piotrek Koszuliński (Reinmar) 2015-12-07 08:16:49 PST
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.
Comment 54 j.swiderski 2018-01-15 00:57:33 PST
Can someone please reopen this ticket? The patch has been reverted, the bug is still there and issue hasn't been resolved.
Comment 55 mitz 2018-01-15 08:47:46 PST
Reverted in r72582.
Comment 56 Ahmad Saleem 2022-07-12 04:12:01 PDT
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!