Bug 109277

Summary: Restore pre-r118852 behavior for EllipsisBox::nodeAtPoint()
Product: WebKit Reporter: Andy Estes <aestes>
Component: New BugsAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, darin, davidbarr, ddkilzer, dglazkov, eric, ojan.autocc, rniwa, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 63781    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch simon.fraser: review+

Andy Estes
Reported 2013-02-08 02:45:08 PST
Restore pre-r118852 behavior for EllipsisBox::nodeAtPoint()
Attachments
Patch (5.97 KB, patch)
2013-02-08 02:51 PST, Andy Estes 		no flags
DetailsFormatted DiffDiff
Patch (11.13 KB, patch)
2013-02-08 17:31 PST, Andy Estes 		simon.fraser: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Andy Estes
Comment 1 2013-02-08 02:51:55 PST
Created attachment 187273 [details] Patch
David Barr
Comment 2 2013-02-08 03:11:46 PST
Andy, thanks for working on this. Does fast/css/text-overflow-ellipsis-hit-test.html still pass with this change? The original change closed one bug and opened another due to two behaviours sharing a code path. ie. -webkit-line-clamp vs text-overflow:ellipsis. Without distinguishing between these, we are exchanging one bug for another.
Andy Estes
Comment 3 2013-02-08 03:20:41 PST
(In reply to comment #2) > Andy, thanks for working on this. Does fast/css/text-overflow-ellipsis-hit-test.html still pass with this change? The original change closed one bug and opened another due to two behaviours sharing a code path. ie. -webkit-line-clamp vs text-overflow:ellipsis. Without distinguishing between these, we are exchanging one bug for another. I'll check the status of that test, but I expect this will break it. The point of this change is simply to revert to the behavior prior to the original change. I think we're willing to re-introduce a spec compliance bug in order to fix an important WebKit client. We lived with the original behavior for over 8 years, so we can live with it a little longer until we find a regression-free fix.
WebKit Review Bot
Comment 4 2013-02-08 06:31:23 PST
Comment on attachment 187273 [details] Patch Attachment 187273 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16434641 New failing tests: fast/css/text-overflow-ellipsis-hit-test.html
Darin Adler
Comment 5 2013-02-08 08:20:34 PST
Another way of putting this is that this is a roll-out that we should do until we find a better fix for the original bug. I think a roll-out makes sense in this case. If possible, we should add a regression test demonstrating the problem that necessitates the roll-out.
Build Bot
Comment 6 2013-02-08 08:30:40 PST
Comment on attachment 187273 [details] Patch Attachment 187273 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16430619 New failing tests: fast/css/text-overflow-ellipsis-hit-test.html
Build Bot
Comment 7 2013-02-08 09:07:47 PST
Comment on attachment 187273 [details] Patch Attachment 187273 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16434685 New failing tests: fast/css/text-overflow-ellipsis-hit-test.html
Simon Fraser (smfr)
Comment 8 2013-02-08 09:17:39 PST
Comment on attachment 187273 [details] Patch Code looks good, but you should add a test.
Tony Chang
Comment 9 2013-02-08 10:38:16 PST
You could check in the failing expectated.txt for fast/css/text-overflow-ellipsis-hit-test.html since that is the new expected behavior.
Ryosuke Niwa
Comment 10 2013-02-08 11:10:40 PST
Since we're rolling out a patch, the test added by the patch should also be rolled out: LayoutTests/fast/css/text-overflow-ellipsis-hit-test.html
Andy Estes
Comment 11 2013-02-08 17:31:17 PST
Created attachment 187390 [details] Patch
Andy Estes
Comment 12 2013-02-08 17:50:29 PST
Committed r142335: <http://trac.webkit.org/changeset/142335>
Note You need to log in before you can comment on or make changes to this bug.