| Summary: | Ellipsis is not properly placed for truncated text inside blocks with horizontal border or padding | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Mario Sanchez Prada <mario> |
| Component: | Text | Assignee: | Mario Sanchez Prada <mario> |
| Status: | NEW --- | ||
| Severity: | Normal | CC: | buildbot, bunhere, cdumez, commit-queue, dtrebbien, esprehn+autocc, glenn, gyuyoung.kim, hyatt, kling, kondapallykalyan, mitz, rniwa, sergio, zoltan |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | 134597, 71194 | ||
| Bug Blocks: | |||
| Attachments: | |||
|
Description
Mario Sanchez Prada
2014-02-25 09:16:18 PST
Created attachment 225983 [details] Patch proposal Now that the patch for bug 71194 has landed, I can propose this other backport from Blink Adding potential reviewers to CC Comment on attachment 225983 [details] Patch proposal Attachment 225983 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5193464824201216 New failing tests: fast/css/text-overflow-ellipsis-text-align-left.html fast/css/vertical-text-overflow-ellipsis-text-align-center.html fast/css/text-overflow-ellipsis-block-with-border-and-padding.html fast/css/text-overflow-ellipsis-text-align-center.html fast/css/vertical-text-overflow-ellipsis-text-align-right.html fast/css/text-overflow-ellipsis-text-align-right.html Created attachment 225987 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 225983 [details] Patch proposal Attachment 225983 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5542912490209280 New failing tests: fast/css/text-overflow-ellipsis-text-align-left.html fast/css/vertical-text-overflow-ellipsis-text-align-center.html fast/css/text-overflow-ellipsis-block-with-border-and-padding.html fast/css/text-overflow-ellipsis-text-align-center.html fast/css/vertical-text-overflow-ellipsis-text-align-right.html fast/css/text-overflow-ellipsis-text-align-right.html Created attachment 225989 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 225983 [details] Patch proposal Attachment 225983 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6217790365106176 New failing tests: fast/css/text-overflow-ellipsis-text-align-left.html fast/css/vertical-text-overflow-ellipsis-text-align-center.html fast/css/text-overflow-ellipsis-block-with-border-and-padding.html fast/css/text-overflow-ellipsis-text-align-center.html fast/css/vertical-text-overflow-ellipsis-text-align-right.html fast/css/text-overflow-ellipsis-text-align-right.html Created attachment 225993 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 225983 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=225983&action=review > LayoutTests/ChangeLog:14 > + * platform/gtk/fast/css/text-overflow-ellipsis-block-with-border-and-padding-expected.png: Added. You are missing other platforms' expected results. > LayoutTests/fast/css/text-overflow-ellipsis-block-with-border-and-padding.html:9 > + padding: 0px 5px 0px 20px; It would be great to see more test with different combination's of paddings/margins, including negative vales. (left > right, left == right, etc) > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2099 > + curr->adjustLogicalPosition(logicalLeft - (availableLogicalWidth - totalLogicalWidth), 0); Normally availableLogicalWidth <= totalLogicalWidth, so (availableLogicalWidth - totalLogicalWidth) usually ends up being <= 0. Since you are pushing to the right direction starting from logicalLeft, to make it more clear, i'd code it like -> logicalLeft + (totalLogicalWidth - availableLogicalWidth) Thanks for the quick review! I'll address all the issues mentioned, but in the meanwhile see my comments below... (In reply to comment #9) > (From update of attachment 225983 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225983&action=review > > > LayoutTests/ChangeLog:14 > > + * platform/gtk/fast/css/text-overflow-ellipsis-block-with-border-and-padding-expected.png: Added. > > You are missing other platforms' expected results. Yes, but I was hoping to fix that situation in the next version of the patch (taking the output from the EWS bots), or as a follow-up gardening patch, by using the webkit-patch rebaseline tool (as I'm unable to test this myself in all the other platforms) > > LayoutTests/fast/css/text-overflow-ellipsis-block-with-border-and-padding.html:9 > > + padding: 0px 5px 0px 20px; > > It would be great to see more test with different combination's of paddings/margins, including negative vales. (left > right, left == right, etc) I can add a few more cases in the layout test, sure. > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2099 > > + curr->adjustLogicalPosition(logicalLeft - (availableLogicalWidth - totalLogicalWidth), 0); > > Normally availableLogicalWidth <= totalLogicalWidth, so (availableLogicalWidth - totalLogicalWidth) usually ends up being <= 0. Since you are pushing to the right direction starting from logicalLeft, to make it more clear, i'd code it like -> logicalLeft + (totalLogicalWidth - availableLogicalWidth) Ok. Comment on attachment 225983 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=225983&action=review > LayoutTests/ChangeLog:8 > + Added new test to check that ellipsis are always properly set, Is there any reason to not use reftests for these? (In reply to comment #11) > (From update of attachment 225983 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225983&action=review > > > LayoutTests/ChangeLog:8 > > + Added new test to check that ellipsis are always properly set, > > Is there any reason to not use reftests for these? Good point. I can try to re-write this as a ref test to make it more "cross-platform aware" Created attachment 226112 [details]
Patch proposal
Attaching a new patch trying to address the issues mentioned, adding more test cases and puttin expectations into a reftest
(In reply to comment #13) > Created an attachment (id=226112) [details] > Patch proposal > > Attaching a new patch trying to address the issues mentioned, adding more test cases and puttin expectations into a reftest So awesome! Are you planning to turn the old tests into reftests too in the future? (In reply to comment #14) > (In reply to comment #13) > > Created an attachment (id=226112) [details] [details] > > Patch proposal > > > > Attaching a new patch trying to address the issues mentioned, adding more test cases and putting expectations into a reftest > > So awesome! Thanks! > Are you planning to turn the old tests into reftests too in the future? At the moment (current patch) I'd rather focus on backporting this patch into WebKit, since there are other things I need to take care of at the moment, but I would not discard converting those tests to reftests in the near future. Ping reviewers? Ping again? :) I'm okay converting these test separately into reftests, since that's out of the scope of this patch. The patch looks good to me, but I'm not a reviewer. I agree with Zoltan here. We should probably split the test change into a separate patch. Also, it seems desirable to hard code ellipses if possible so that we can verify ellipses are actually there. (In reply to comment #19) > I agree with Zoltan here. We should probably split the test change into a > separate patch. Also, it seems desirable to hard code ellipses if possible > so that we can verify ellipses are actually there. As per the conversation on IRC, I would like to propose the alternative of getting back to the old-style test expectations to finish the backport and then file a bug later to convert to reftests all these tests related to ellipsis (not just the one added with this patch) The advantage of that would be that we could fix this issue in WebKit already now (while having the code tested) and that we would later on "fix" all the tested together, in the same commit (which I probably could propose to port back to blink too) How does it sound? (In reply to comment #20) > (In reply to comment #19) > > I agree with Zoltan here. We should probably split the test change into a > > separate patch. Also, it seems desirable to hard code ellipses if possible > > so that we can verify ellipses are actually there. > > As per the conversation on IRC, I would like to propose the alternative of getting back to the old-style test expectations to finish the backport and then file a bug later to convert to reftests all these tests related to ellipsis (not just the one added with this patch) > > The advantage of that would be that we could fix this issue in WebKit already now (while having the code tested) and that we would later on "fix" all the tested together, in the same commit (which I probably could propose to port back to blink too) > > How does it sound? I understand this might not be a prioritary thing (that's why I'm not pushing too much either), but two months to agree on backporting this fix starts to look to me like "too much" too :) Please provide some feedback to know at least whether my proposal is ok or not Created attachment 234328 [details]
Patch proposal
Rebased patch which, for some reason, does not apply cleanly after 4 months
I sincerely appreciate your efforts here, Mario. To the WebKit devs: would someone please commit this patch? (In reply to comment #23) > I sincerely appreciate your efforts here, Mario. > Thanks. > To the WebKit devs: would someone please commit this patch? JFTR, I can commit it myself but I need a positive review first, which is what I'm looking for. Anyway, it seems I was too fast and the patch uploaded needs some more changes (does not build yet), I will create a new version now, test it and upload a new patch soon. Also, I'll try to get some feedback over IRC too Created attachment 234336 [details] Patch proposal Updated patch not to use pixelSnappedLogical{Left|Right}OffsetForLine, no longer used since r169048 (In reply to comment #25) > Created an attachment (id=234336) [details] > Patch proposal > > Updated patch not to use pixelSnappedLogical{Left|Right}OffsetForLine, no longer used since r169048 For the sake of completeness, we are finally going with the "fix tests first + submit patch later approach", as discussed on IRC: <rniwa> msanchez: did you split the test change? <rniwa> msanchez: as far as I could recall & read the comment <rniwa> msanchez: it’s currently blocked on you to split the patch <rniwa> msanchez: or has that already been done? <rniwa> msanchez: if so, we need to make whichever bug used to make the test change a blocker <rniwa> for this bug <msanchez> rniwa: My understanding was that we would land the patch as it is now (with a traditional test instead of a reftest) and then convert all the tests related to ellipsis to reftests as a second step <msanchez> seems there was some confusion then... <msanchez> well, actually there was no confusion. That's exactly what my last comment says, but that one remain unanswered, so I guess what happened is that each of us kept our own version in our minds :) <msanchez> rniwa: ^ <rniwa> msanchez: please convert the test first <rniwa> msanchez: otherwise we have to rebaseline the expected results for this change <rniwa> and then convert the test <rniwa> if we convert now, we don’t have to do rebaselines <msanchez> rniwa: ok then <msanchez> makes sense to me too (and the reason of "fixing it quickly" is no longer a good one anyway) So, we need to fix the newly reported bug 134597 first, then come back to this one. Comment on attachment 234336 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=234336&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1843 > + float snappedLogicalLeft = logicalLeftOffsetForLine(curr->lineTop(), firstLine); I don’t understand why this local variable has the word “snapped” in it. Is it pixel snapped? I thought that generally layout did not do pixel snapping, rather painting did. (In reply to comment #27) > (From update of attachment 234336 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=234336&action=review > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1843 > > + float snappedLogicalLeft = logicalLeftOffsetForLine(curr->lineTop(), firstLine); > > I don’t understand why this local variable has the word “snapped” in it. Is it > pixel snapped? I thought that generally layout did not do pixel snapping, rather > painting did. Sorry, this comes from the previous version of the patch, where I called pixelSnappedLogicalLeftOffsetForLine(). I will change that in the new version of the patch I will submit once I get bug 134597 fixed. Thanks for the review Created attachment 234926 [details] Patch proposal Uploading a new version converting all the affected tests that I could into reftests, as requested. For the tests related to vertical text I simply could not convert them to reftests because there seems to be another bug preventing the ellipsis from showing up in those cases, so we agreed on rebaselining those ones instead along with this patch (see https://bugs.webkit.org/show_bug.cgi?id=134724#c5). Not asking for review yet because I want to see how the EWS behaves before doing so. Comment on attachment 234926 [details] Patch proposal Attachment 234926 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5308318331437056 New failing tests: fast/css/text-overflow-ellipsis-text-align-justify.html fast/css/text-overflow-ellipsis-text-align-left.html fast/css/text-overflow-ellipsis-text-align-center.html fast/css/text-overflow-ellipsis-text-align-right.html Created attachment 234931 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 234926 [details] Patch proposal Attachment 234926 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6297627540848640 New failing tests: fast/css/text-overflow-ellipsis-text-align-justify.html fast/css/text-overflow-ellipsis-text-align-left.html fast/css/text-overflow-ellipsis-text-align-center.html fast/css/text-overflow-ellipsis-text-align-right.html Created attachment 234934 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 234926 [details] Patch proposal Attachment 234926 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4792786964447232 New failing tests: fast/css/text-overflow-ellipsis-text-align-justify.html fast/css/text-overflow-ellipsis-text-align-left.html fast/css/text-overflow-ellipsis-text-align-center.html fast/css/text-overflow-ellipsis-text-align-right.html Created attachment 234942 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 234336 [details] Patch proposal Cleared Darin Adler's review+ from obsolete attachment 234336 [details] so that this bug does not appear in http://webkit.org/pending-commit. (In reply to comment #30) > (From update of attachment 234926 [details]) > Attachment 234926 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.appspot.com/results/5308318331437056 > > New failing tests: > fast/css/text-overflow-ellipsis-text-align-justify.html > fast/css/text-overflow-ellipsis-text-align-left.html > fast/css/text-overflow-ellipsis-text-align-center.html > fast/css/text-overflow-ellipsis-text-align-right.html For some strange reason, the -expected.html files render the ellipsis (hardcoded as …) in the Mac slightly higher compared to how they are rendered as the result of wrapping the text in the regular text. Any idea on why this might happening? If not, I'm starting to the believe that the it would be better to just land the previous patch (after applying the comments pointed out by Darin) and leave the conversion of tests into reftests for another moment, as it's proving to be quite challenging, at least for me and now (busy with other things atm): vertical text presents other issues, path for bug 134597 presents different results in the Mac as well, and now this. Opinions? (In reply to comment #37) > (In reply to comment #30) > > (From update of attachment 234926 [details] [details]) > > Attachment 234926 [details] [details] did not pass mac-ews (mac): > > Output: http://webkit-queues.appspot.com/results/5308318331437056 > > > > New failing tests: > > fast/css/text-overflow-ellipsis-text-align-justify.html > > fast/css/text-overflow-ellipsis-text-align-left.html > > fast/css/text-overflow-ellipsis-text-align-center.html > > fast/css/text-overflow-ellipsis-text-align-right.html > > For some strange reason, the -expected.html files render the ellipsis (hardcoded as …) in the Mac slightly higher compared to how they are > rendered as the result of wrapping the text in the regular text. > > Any idea on why this might happening? If not, I'm starting to the believe that the it would be better to just land the previous patch > (after applying the comments pointed out by Darin) and leave the conversion of tests into reftests for another moment, as it's proving > to be quite challenging, at least for me and now (busy with other things atm): vertical text presents other issues, path for bug 134597 > presents different results in the Mac as well, and now this. > > Opinions? I'll be around until this Friday but then I'll be away for a few weeks, so I would appreciate if I could get some feedback before then. FWIW, I did some experiments today and my feeling is that there might be some other bugs around that are preventing me from providing a patch that does not fail in the Mac because of really minor difference between the actual and expected results when I try to convert those tests into reftests. Thus, I believe it's better to land the patch as it is now (with the new test + reftest), get this fixed now once and for all and then rebaseline whatever needs rebaselining. If we can agree on that being the way to go, it would be as easy as landing the attachment 22611 [details], which already got an r+ from Darin (after addressing his comments on the name of those variables). (In reply to comment #38) > [...] > I'll be around until this Friday but then I'll be away for a few weeks, so I > would appreciate if I could get some feedback before then. [...] > If we can agree on that being the way to go, it would be as easy as landing > the attachment 22611 [details], which already got an r+ from Darin (after > addressing his comments on the name of those variables). It's Thursday already and I'm afraid I won't have more time this week that I could actually devote to this, and then I will be away (from keyboard) for 3-4 weeks more or less, so please feel free to pick this up and finish it yourselves if you think it's fine. My honest opinion is that attachment 22611 [details] should be landed now as it is and then rebaseline tests. There seem a few issues around in the Mac port that, at least for me, complicates quite a bit progressing with the idea of converting everything to reftests first, and then avoid rebaseline (e.g. ellipsis in vertical text, the weird thing found out with underlines while working on 134597...), so IMHO it's better to fix this now than leaving it around even longer. But I won't insist much for now, I don't think it's fair to do it right before leaving on holidays. Maybe afterwards, if this is still here when I'm back (and if I still have a Mac around) :) *** Bug 131592 has been marked as a duplicate of this bug. *** |