RESOLVED FIXED 92818
REGRESSION: WebKit does not render selection in non-first ruby text nodes.
https://bugs.webkit.org/show_bug.cgi?id=92818
Summary REGRESSION: WebKit does not render selection in non-first ruby text nodes.
Sukolsak Sakshuwong
Reported 2012-07-31 18:18:57 PDT
Created attachment 155696 [details] test case Reproduction steps: 1. Create a page with the following contents: <!DOCTYPE html> <html> <head> <meta charset="utf-8"> </head> <body> a<ruby>一<rt>1</rt>二<rt>2</rt>三<rt>3</rt></ruby>b </body> </html> 2. Press Cmd+A (or Ctrl+A on Windows/Linux) to select all Expected result: "a 一1 二2 三3 b" is highlighted Actual result: Only "a 一1 二 三 b" is highlighted
Attachments
test case (138 bytes, text/html)
2012-07-31 18:18 PDT, Sukolsak Sakshuwong
no flags
Patch (19.99 KB, patch)
2012-07-31 22:37 PDT, Sukolsak Sakshuwong
no flags
Archive of layout-test-results from gce-cr-linux-07 (365.65 KB, application/zip)
2012-07-31 23:13 PDT, WebKit Review Bot
no flags
Patch (35.46 KB, patch)
2012-12-12 22:51 PST, Ryosuke Niwa
leviw: review+
webkit.review.bot: commit-queue-
Elliott Sprehn
Comment 1 2012-07-31 19:19:41 PDT
This doesn't happen in Safari 5.1, but happens in ToT (and Chrome 20, etc.).
Sukolsak Sakshuwong
Comment 2 2012-07-31 22:37:32 PDT
Ryosuke Niwa
Comment 3 2012-07-31 22:59:39 PDT
Comment on attachment 155722 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155722&action=review Unfortunately, I'm not qualified to review this patch :( > Source/WebCore/ChangeLog:14 > + No new tests (OOPS!). You need to mention the test you're adding here. See other change log entries for an example.
WebKit Review Bot
Comment 4 2012-07-31 23:13:46 PDT
Comment on attachment 155722 [details] Patch Attachment 155722 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13415076 New failing tests: fast/ruby/select-ruby.html
WebKit Review Bot
Comment 5 2012-07-31 23:13:50 PDT
Created attachment 155725 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Robert Hogan
Comment 6 2012-08-01 13:49:38 PDT
Comment on attachment 155722 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155722&action=review > Source/WebCore/ChangeLog:12 > + RenderRubyRun is an 'inline-block/table'-like object and therefore should be > + a selection root. However, because it's anonymous, the isSelectionRoot() method > + returned false. This caused RootInlineBox::selectionTopAdjustedForPrecedingBlock > + to return incorrect values and caused the highlight to not show up. > + Make isSelectionRoot() return true for anonymous inline-block objects. Driveby comment: To cater for the RubyRun case you're opening up isSelectionRoot() to all inline blocks and tables. This might be fine, but I think you need more tests to show that's the case.
Sukolsak Sakshuwong
Comment 7 2012-08-01 14:48:25 PDT
(In reply to comment #3) > > Source/WebCore/ChangeLog:14 > > + No new tests (OOPS!). > > You need to mention the test you're adding here. See other change log entries for an example. Will fix. (In reply to comment #6) > > Source/WebCore/ChangeLog:12 > > + RenderRubyRun is an 'inline-block/table'-like object and therefore should be > > + a selection root. However, because it's anonymous, the isSelectionRoot() method > > + returned false. This caused RootInlineBox::selectionTopAdjustedForPrecedingBlock > > + to return incorrect values and caused the highlight to not show up. > > + Make isSelectionRoot() return true for anonymous inline-block objects. > > Driveby comment: To cater for the RubyRun case you're opening up isSelectionRoot() to all inline blocks and tables. This might be fine, but I think you need more tests to show that's the case. Thanks for the comment. Since isSelectionRoot() has already returned true for all non-anonymous inline-block objects, I just opened it up for anonymous inline-block objects as well. Do we have anonymous inline-blocks objects (or anonymous inline tables) in WebKit other than RubyRun?
Levi Weintraub
Comment 8 2012-08-22 11:37:22 PDT
(In reply to comment #7) > > Driveby comment: To cater for the RubyRun case you're opening up isSelectionRoot() to all inline blocks and tables. This might be fine, but I think you need more tests to show that's the case. > > Thanks for the comment. Since isSelectionRoot() has already returned true for all non-anonymous inline-block objects, I just opened it up for anonymous inline-block objects as well. Do we have anonymous inline-blocks objects (or anonymous inline tables) in WebKit other than RubyRun? Seems like this could happen with Tables. Julien can probably tell you.
Julien Chaffraix
Comment 9 2012-08-22 12:29:01 PDT
(In reply to comment #8) > (In reply to comment #7) > > > Driveby comment: To cater for the RubyRun case you're opening up isSelectionRoot() to all inline blocks and tables. This might be fine, but I think you need more tests to show that's the case. > > > > Thanks for the comment. Since isSelectionRoot() has already returned true for all non-anonymous inline-block objects, I just opened it up for anonymous inline-block objects as well. Do we have anonymous inline-blocks objects (or anonymous inline tables) in WebKit other than RubyRun? > > Seems like this could happen with Tables. Julien can probably tell you. We can't have anonymous inline tables at this time as we haven't implemented this (see bug 15365, the bottom line is that it's non-trivial). Beware that it is expected and something we will want to fix someday though.
Radar WebKit Bug Importer
Comment 10 2012-12-12 00:59:49 PST
Ryosuke Niwa
Comment 11 2012-12-12 15:26:58 PST
Comment on attachment 155722 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155722&action=review > Source/WebCore/rendering/RenderBlock.cpp:3189 > if (!node()) > return false; I've done some investigation and it appears that we should just remove this line. It was added in http://trac.webkit.org/changeset/12986 but I don't think we need it anymore.
Elliott Sprehn
Comment 12 2012-12-12 15:33:18 PST
Comment on attachment 155722 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155722&action=review > Source/WebCore/rendering/RenderBlock.cpp:3185 > + if (isInlineBlockOrInlineTable()) This permits :before and :after to be selection roots, was that intended? >> Source/WebCore/rendering/RenderBlock.cpp:3189 >> return false; > > I've done some investigation and it appears that we should just remove this line. It was added in http://trac.webkit.org/changeset/12986 but I don't think we need it anymore. Removing this line will make PseudoElement's capable of being selection roots. Note that now the code is if (!nonPseudoNode()) return; it'll also make the anonymous renderers inside them, and :first-letter, etc. all into selection roots.
Ryosuke Niwa
Comment 13 2012-12-12 22:51:56 PST
WebKit Review Bot
Comment 14 2012-12-13 02:03:03 PST
Comment on attachment 179203 [details] Patch Attachment 179203 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15311175 New failing tests: editing/selection/range-between-block-and-inline.html fast/repaint/selection-clear.html fast/ruby/select-ruby.html
Levi Weintraub
Comment 15 2012-12-27 12:41:14 PST
Comment on attachment 179203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179203&action=review > Source/WebCore/ChangeLog:11 > + it doesn't lay down its children in block direction. Lay down == lay out? > Source/WebCore/ChangeLog:30 > + http://trac.webkit.org/changeset/12986 but all tests added by this change set as well as the rest > + of layout tests pass without this condition. Sadness :(
Ryosuke Niwa
Comment 16 2013-01-23 16:34:26 PST
Elliott Sprehn
Comment 17 2013-01-23 16:45:59 PST
(In reply to comment #15) > (From update of attachment 179203 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179203&action=review > > > Source/WebCore/ChangeLog:11 > > + it doesn't lay down its children in block direction. > > Lay down == lay out? > > > Source/WebCore/ChangeLog:30 > > + http://trac.webkit.org/changeset/12986 but all tests added by this change set as well as the rest This linked changeset is wrong, it's from 7 years ago. I don't think this patch is right though. The original check was: if (!node()) return; I changed it to be: if (!node() || isPseudoElment()) return; in https://trac.webkit.org/changeset/136744 and then I encapsulated that to: if (!nonPseudoNode) return; in https://trac.webkit.org/changeset/137336 So this patch just enabled all anonymous RenderBlock (and all subclasses) as being selection roots which doesn't seem right without more tests. Also the ASSERT(node() || isAnonymous()); is a truism. Every renderer is either anonymous, or has a node.
Note You need to log in before you can comment on or make changes to this bug.