Bug 92818

Summary: REGRESSION: WebKit does not render selection in non-first ruby text nodes.
Product: WebKit Reporter: Sukolsak Sakshuwong <sukolsak>
Component: Layout and RenderingAssignee: 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:
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-

Description Sukolsak Sakshuwong 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
Comment 1 Elliott Sprehn 2012-07-31 19:19:41 PDT
This doesn't happen in Safari 5.1, but happens in ToT (and Chrome 20, etc.).
Comment 2 Sukolsak Sakshuwong 2012-07-31 22:37:32 PDT
Created attachment 155722 [details]
Patch
Comment 3 Ryosuke Niwa 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.
Comment 4 WebKit Review Bot 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
Comment 5 WebKit Review Bot 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
Comment 6 Robert Hogan 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.
Comment 7 Sukolsak Sakshuwong 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?
Comment 8 Levi Weintraub 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.
Comment 9 Julien Chaffraix 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.
Comment 10 Radar WebKit Bug Importer 2012-12-12 00:59:49 PST
<rdar://problem/12862809>
Comment 11 Ryosuke Niwa 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.
Comment 12 Elliott Sprehn 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.
Comment 13 Ryosuke Niwa 2012-12-12 22:51:56 PST
Created attachment 179203 [details]
Patch
Comment 14 WebKit Review Bot 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
Comment 15 Levi Weintraub 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 :(
Comment 16 Ryosuke Niwa 2013-01-23 16:34:26 PST
Committed r140613: <http://trac.webkit.org/changeset/140613>
Comment 17 Elliott Sprehn 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.