Summary: | REGRESSION: WebKit does not render selection in non-first ruby text nodes. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sukolsak Sakshuwong <sukolsak> | ||||||||||
Component: | Layout and Rendering | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, eric, esprehn, hyatt, jchaffraix, leviw, mitz, ojan.autocc, rniwa, robert, rolandsteiner, webkit-bug-importer, webkit.review.bot | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
This doesn't happen in Safari 5.1, but happens in ToT (and Chrome 20, etc.). Created attachment 155722 [details]
Patch
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. 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 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
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. (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? (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. (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. 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. 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. Created attachment 179203 [details]
Patch
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 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 :( Committed r140613: <http://trac.webkit.org/changeset/140613> (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. |
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