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
Inside list bullets work just fine.
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?
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.
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?
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.
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.
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.
It's obvious to me that we should fix this, even if it violates CSS2.1.
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.
Created attachment 65893 [details] Fix the position of outside list bullets
Any feedbacks are welcome :)
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.)
Created attachment 68071 [details] Fix the position of outside list bullets
(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.
Attachment 68071 [details] did not build on qt: Build output: http://queues.webkit.org/results/4098004
Created attachment 68074 [details] Fix the position of outside list bullets
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()?
(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 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?
Created attachment 71420 [details] Fix the position of outside list bullets
> 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 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/
Also, doesn't this still affect editing/pasteboard/paste-4039777-fix.html? A new baseline is missing from the latest patch.
Attachment 71420 [details] did not build on qt: Build output: http://queues.webkit.org/results/4690011
Created attachment 71504 [details] Fix the position of outside list bullets
(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...
Attachment 71504 [details] did not build on qt: Build output: http://queues.webkit.org/results/4659015
Created attachment 71531 [details] Fix the position of outside list bullets
Created attachment 71562 [details] Fix the position of outside list bullets
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
Created attachment 74023 [details] Patch
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.
Created attachment 74391 [details] Patch
Comment on attachment 74391 [details] Patch r=me
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
This is landed in <http://trac.webkit.org/changeset/72527> Closing bug.
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
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?
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
(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.
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.
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.
Created attachment 86310 [details] Rollout some png files of r72533 to complete r72599.
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.
Comment on attachment 86312 [details] Change the style of patch. Clearing flags on attachment: 86312 Committed r81579: <http://trac.webkit.org/changeset/81579>
All reviewed patches have been landed. Closing bug.
This issue can be reproduced again: http://jsfiddle.net/bufzfktn/ Was this patch reverted for some reasons?
(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.
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.
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!
(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.