REGRESSION: WebKit does not render selection in non-first ruby text nodes.
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 Jan 2013 00:33:06 +0000 (00:33 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 Jan 2013 00:33:06 +0000 (00:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=92818

Reviewed by Levi Weintraub.

Source/WebCore:

The patch is based on the one submitted by Sukolsak Sakshuwong.

The bug was caused by the fact isSelectionRoot was returning false on RenderRubyRun even though
it doesn't lay down its children in block direction.

The selection painting code assumes that all blocks in each selection root are laid down in
the containing block direction. In particular, InlineTextBox::paintSelection calls
RootInlineBox::selectionTopAdjustedForPrecedingBlock in order to determine the end of the previous
line, which in turn calls blockBeforeWithinSelectionRoot. blockBeforeWithinSelectionRoot goes
through block nodes that appears before "this" block, and selectionTopAdjustedForPrecedingBlock
assumes that to compute the end of the previous line.

Now suppose we have markup such as <ruby>Ichi<rt>One</rt></ruby><ruby>Ni<rt>Two</rt></ruby>. When
selectionTopAdjustedForPrecedingBlock is called on the line box generated for "Two", it tries to
determine the bottom of the inline box above that of "Two", which blockBeforeWithinSelectionRoot
determines to be that of "One". At this point, everything goes wrong and the selection height is
computed to be 0.

The fix to this problem is to allow RenderRubyRun to be a selection root. Since RenderRubyRun is
already an inline-block, it suffices to bypass the !nonPseudoNode() check. In fact, there is no
need to check this condition anymore as far as I can tell. The check was added in
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.

Test: fast/ruby/select-ruby.html

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::isSelectionRoot):

LayoutTests:

Add a test, authored by Sukolsak Sakshuwong.

* fast/ruby/select-ruby.html: Added.
* platform/mac/fast/ruby/ruby-base-merge-block-children-crash-expected.png: Added.
* platform/mac/fast/ruby/select-ruby-expected.png: Added.
* platform/mac/fast/ruby/select-ruby-expected.txt: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@140613 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/ruby/select-ruby.html [new file with mode: 0644]
LayoutTests/platform/mac/fast/ruby/ruby-base-merge-block-children-crash-expected.png [new file with mode: 0644]
LayoutTests/platform/mac/fast/ruby/select-ruby-expected.png [new file with mode: 0644]
LayoutTests/platform/mac/fast/ruby/select-ruby-expected.txt [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlock.cpp

index 4d9afcf9531a2b6e2e073178b0a356d457cd6129..a8a3585bd550b1449aebae3347c68c6759f9ef60 100644 (file)
@@ -1,3 +1,17 @@
+2012-12-12  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION: WebKit does not render selection in non-first ruby text nodes.
+        https://bugs.webkit.org/show_bug.cgi?id=92818
+
+        Reviewed by Levi Weintraub.
+
+        Add a test, authored by Sukolsak Sakshuwong.
+
+        * fast/ruby/select-ruby.html: Added.
+        * platform/mac/fast/ruby/ruby-base-merge-block-children-crash-expected.png: Added.
+        * platform/mac/fast/ruby/select-ruby-expected.png: Added.
+        * platform/mac/fast/ruby/select-ruby-expected.txt: Added.
+
 2013-01-23  Martin Robinson  <mrobinson@igalia.com>
 
         WebKit should support decoding multi-byte entities in XML content
diff --git a/LayoutTests/fast/ruby/select-ruby.html b/LayoutTests/fast/ruby/select-ruby.html
new file mode 100644 (file)
index 0000000..a6987a7
--- /dev/null
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+body {
+    font-family: Ahem;
+    font-size: 16px;
+    -webkit-font-smoothing: none;
+    color: rgba(0, 0, 0, 0.1);
+}
+</style>
+</head>
+<body>
+<!-- All texts inside the ruby should be highlighted. -->
+a<ruby>1<rt>1</rt>2<rt>2</rt>3<rt>3</rt>4<rt>4</rt></ruby>b
+<script>
+var range = document.createRange();
+range.selectNodeContents(document.body);
+window.getSelection().addRange(range);
+</script>
+</body>
+</html>
diff --git a/LayoutTests/platform/mac/fast/ruby/ruby-base-merge-block-children-crash-expected.png b/LayoutTests/platform/mac/fast/ruby/ruby-base-merge-block-children-crash-expected.png
new file mode 100644 (file)
index 0000000..bfc73ae
Binary files /dev/null and b/LayoutTests/platform/mac/fast/ruby/ruby-base-merge-block-children-crash-expected.png differ
diff --git a/LayoutTests/platform/mac/fast/ruby/select-ruby-expected.png b/LayoutTests/platform/mac/fast/ruby/select-ruby-expected.png
new file mode 100644 (file)
index 0000000..2d26ebd
Binary files /dev/null and b/LayoutTests/platform/mac/fast/ruby/select-ruby-expected.png differ
diff --git a/LayoutTests/platform/mac/fast/ruby/select-ruby-expected.txt b/LayoutTests/platform/mac/fast/ruby/select-ruby-expected.txt
new file mode 100644 (file)
index 0000000..05095e6
--- /dev/null
@@ -0,0 +1,41 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x40
+  RenderBlock {HTML} at (0,0) size 800x40
+    RenderBody {BODY} at (8,8) size 784x24 [color=#00000019]
+      RenderText {#text} at (0,8) size 16x16
+        text run at (0,8) width 16: "a"
+      RenderRuby (inline) {RUBY} at (0,0) size 64x16
+        RenderRubyRun (anonymous) at (16,8) size 16x16
+          RenderRubyText {RT} at (0,-8) size 16x8
+            RenderText {#text} at (4,0) size 8x8
+              text run at (4,0) width 8: "1"
+          RenderRubyBase (anonymous) at (0,0) size 16x16
+            RenderText {#text} at (0,0) size 16x16
+              text run at (0,0) width 16: "1"
+        RenderRubyRun (anonymous) at (32,8) size 16x16
+          RenderRubyText {RT} at (0,-8) size 16x8
+            RenderText {#text} at (4,0) size 8x8
+              text run at (4,0) width 8: "2"
+          RenderRubyBase (anonymous) at (0,0) size 16x16
+            RenderText {#text} at (0,0) size 16x16
+              text run at (0,0) width 16: "2"
+        RenderRubyRun (anonymous) at (48,8) size 16x16
+          RenderRubyText {RT} at (0,-8) size 16x8
+            RenderText {#text} at (4,0) size 8x8
+              text run at (4,0) width 8: "3"
+          RenderRubyBase (anonymous) at (0,0) size 16x16
+            RenderText {#text} at (0,0) size 16x16
+              text run at (0,0) width 16: "3"
+        RenderRubyRun (anonymous) at (64,8) size 16x16
+          RenderRubyText {RT} at (0,-8) size 16x8
+            RenderText {#text} at (4,0) size 8x8
+              text run at (4,0) width 8: "4"
+          RenderRubyBase (anonymous) at (0,0) size 16x16
+            RenderText {#text} at (0,0) size 16x16
+              text run at (0,0) width 16: "4"
+      RenderText {#text} at (80,8) size 16x16
+        text run at (80,8) width 16: "b"
+      RenderText {#text} at (0,0) size 0x0
+selection start: position 1 of child 2 {#text} of body
+selection end:   position 1 of child 4 {#text} of body
index 53fdb0997add22b2734498bcf1135c794fcfd49b..60de1418953c49866e41caa4583b372c83b45986 100644 (file)
@@ -1,3 +1,39 @@
+2012-12-12  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION: WebKit does not render selection in non-first ruby text nodes.
+        https://bugs.webkit.org/show_bug.cgi?id=92818
+
+        Reviewed by Levi Weintraub.
+
+        The patch is based on the one submitted by Sukolsak Sakshuwong.
+
+        The bug was caused by the fact isSelectionRoot was returning false on RenderRubyRun even though
+        it doesn't lay down its children in block direction.
+
+        The selection painting code assumes that all blocks in each selection root are laid down in
+        the containing block direction. In particular, InlineTextBox::paintSelection calls
+        RootInlineBox::selectionTopAdjustedForPrecedingBlock in order to determine the end of the previous
+        line, which in turn calls blockBeforeWithinSelectionRoot. blockBeforeWithinSelectionRoot goes
+        through block nodes that appears before "this" block, and selectionTopAdjustedForPrecedingBlock
+        assumes that to compute the end of the previous line.
+
+        Now suppose we have markup such as <ruby>Ichi<rt>One</rt></ruby><ruby>Ni<rt>Two</rt></ruby>. When
+        selectionTopAdjustedForPrecedingBlock is called on the line box generated for "Two", it tries to
+        determine the bottom of the inline box above that of "Two", which blockBeforeWithinSelectionRoot
+        determines to be that of "One". At this point, everything goes wrong and the selection height is
+        computed to be 0.
+
+        The fix to this problem is to allow RenderRubyRun to be a selection root. Since RenderRubyRun is
+        already an inline-block, it suffices to bypass the !nonPseudoNode() check. In fact, there is no
+        need to check this condition anymore as far as I can tell. The check was added in
+        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.
+
+        Test: fast/ruby/select-ruby.html
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::isSelectionRoot):
+
 2013-01-23  Kentaro Hara  <haraken@chromium.org>
 
         [V8] Reduce usage of deprecatedV8String() and deprecatedV8Integer()
index a0ca6018e9f908f965f18b10afab77f5684f6bd5..9c375fa360e0371dbf94881c47e5dcb375ab1452 100644 (file)
@@ -3252,8 +3252,9 @@ bool RenderBlock::shouldPaintSelectionGaps() const
 
 bool RenderBlock::isSelectionRoot() const
 {
-    if (!nonPseudoNode())
+    if (isPseudoElement())
         return false;
+    ASSERT(node() || isAnonymous());
         
     // FIXME: Eventually tables should have to learn how to fill gaps between cells, at least in simple non-spanning cases.
     if (isTable())