Bug 92818

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:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
test case	none
Patch	none
Archive of layout-test-results from gce-cr-linux-07	none
Patch	leviw: review+, webkit.review.bot: commit-queue-

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	Details
Patch (19.99 KB, patch) 2012-07-31 22:37 PDT, Sukolsak Sakshuwong	no flags	Details Formatted Diff Diff
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	Details
Patch (35.46 KB, patch) 2012-12-12 22:51 PST, Ryosuke Niwa	leviw: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

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

Created attachment 155722 [details] Patch

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

<rdar://problem/12862809>

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

Created attachment 179203 [details] Patch

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

Committed r140613: <http://trac.webkit.org/changeset/140613>

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.