Skip to content

Commit

Permalink
ruby: Secure text height even if ruby base is empty
Browse files Browse the repository at this point in the history
New behavior matches to Firefox.

Implementation:
Layout(NG)RubyBase is always created just after a Layout(NG)RubyRun is
created, by calling LayoutRubyRun::RubyBaseSafe().
Set |-internal-empty-line-height:fabricated| to keep one line height and
put a ruby annotation on it.

- RubyBase existence check does not make sense any longer. Code like
  |if (HasRubyBase())...| should be |if (RubyBase()->FirstChild()) ...|.
- We need some changes in order to avoid removing empty RubyBase.

Test:
* ruby-base-different-size.html:
  It passed accidentally. It fails now due to a wrong base:annotation
  matching.

Bug: 847274
Change-Id: I46711c1433e9e154fc75346d4c40acf4c2bcaa02
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3636540
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Auto-Submit: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1001906}
  • Loading branch information
tkent-google authored and Chromium LUCI CQ committed May 11, 2022
1 parent e5dfc67 commit 12434db
Show file tree
Hide file tree
Showing 13 changed files with 25 additions and 20 deletions.
7 changes: 7 additions & 0 deletions third_party/blink/renderer/core/layout/layout_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
#include "third_party/blink/renderer/core/layout/layout_object_factory.h"
#include "third_party/blink/renderer/core/layout/layout_object_inl.h"
#include "third_party/blink/renderer/core/layout/layout_object_inlines.h"
#include "third_party/blink/renderer/core/layout/layout_ruby_run.h"
#include "third_party/blink/renderer/core/layout/layout_table_caption.h"
#include "third_party/blink/renderer/core/layout/layout_text_fragment.h"
#include "third_party/blink/renderer/core/layout/layout_theme.h"
Expand Down Expand Up @@ -3760,6 +3761,12 @@ void LayoutObject::DestroyAndCleanupAnonymousWrappers(
if (destroy_root_parent->Parent() &&
destroy_root_parent->Parent()->IsLayoutNGFieldset())
break;
// RubyBase should be kept if RubyText exists
if (destroy_root_parent->IsRubyBase()) {
auto* ruby_run = DynamicTo<LayoutRubyRun>(destroy_root_parent->Parent());
if (ruby_run && ruby_run->HasRubyText())
break;
}

// We need to keep the anonymous parent, if it won't become empty by the
// removal of this LayoutObject.
Expand Down
2 changes: 2 additions & 0 deletions third_party/blink/renderer/core/layout/layout_ruby.cc
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ void LayoutRubyAsInline::AddChild(LayoutObject* child,
if (!last_run || last_run->HasRubyText()) {
last_run = LayoutRubyRun::StaticCreateRubyRun(this, *ContainingBlock());
LayoutInline::AddChild(last_run, before_child);
last_run->RubyBaseSafe();
}
last_run->AddChild(child);
}
Expand Down Expand Up @@ -164,6 +165,7 @@ void LayoutRubyAsBlock::AddChild(LayoutObject* child,
if (!last_run || last_run->HasRubyText()) {
last_run = LayoutRubyRun::StaticCreateRubyRun(this, *this);
LayoutBlockFlow::AddChild(last_run, before_child);
last_run->RubyBaseSafe();
}
last_run->AddChild(child);
}
Expand Down
6 changes: 3 additions & 3 deletions third_party/blink/renderer/core/layout/layout_ruby.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ namespace blink {
// 0 or 1 LayoutRubyText - shuffled to the front in order to re-use
// existing block layouting
// 0-n inline object(s)
// 0 or 1 LayoutRubyBase - contains the inline objects that make up the
// ruby base
// 1-n inline object(s)
// 1 LayoutRubyBase - contains the inline objects that make up the
// ruby base
// 0-n inline object(s)
//
// Note: <rp> elements are defined as having 'display:none' and thus normally
// are not assigned a layoutObject.
Expand Down
23 changes: 10 additions & 13 deletions third_party/blink/renderer/core/layout/layout_ruby_run.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,22 +114,24 @@ void LayoutRubyRun::AddChild(LayoutObject* child, LayoutObject* before_child) {
DCHECK_EQ(before_child->Parent(), this);
LayoutObject* ruby = Parent();
DCHECK(ruby->IsRuby());
LayoutBlock* new_run = StaticCreateRubyRun(ruby, *ContainingBlock());
auto* new_run = StaticCreateRubyRun(ruby, *ContainingBlock());
ruby->AddChild(new_run, NextSibling());
new_run->RubyBaseSafe();
// Add the new ruby text and move the old one to the new run
// Note: Doing it in this order and not using LayoutRubyRun's methods,
// in order to avoid automatic removal of the ruby run in case there is no
// other child besides the old ruby text.
LayoutBlockFlow::AddChild(child, before_child);
LayoutBlockFlow::RemoveChild(before_child);
new_run->AddChild(before_child);
} else if (HasRubyBase()) {
} else if (RubyBase()->FirstChild()) {
// Insertion before a ruby base object.
// In this case we need insert a new run before the current one and split
// the base.
LayoutObject* ruby = Parent();
LayoutRubyRun* new_run = StaticCreateRubyRun(ruby, *ContainingBlock());
ruby->AddChild(new_run, this);
new_run->RubyBaseSafe();
new_run->AddChild(child);

// Make sure we don't leave anything in the percentage descendant
Expand Down Expand Up @@ -158,16 +160,14 @@ void LayoutRubyRun::RemoveChild(LayoutObject* child) {
if (!BeingDestroyed() && !DocumentBeingDestroyed() && child->IsRubyText()) {
LayoutRubyBase* base = RubyBase();
LayoutObject* right_neighbour = NextSibling();
if (base && right_neighbour && right_neighbour->IsRubyRun()) {
// Ruby run without a base can happen only at the first run.
if (base->FirstChild() && right_neighbour && right_neighbour->IsRubyRun()) {
auto* right_run = To<LayoutRubyRun>(right_neighbour);
if (right_run->HasRubyBase()) {
LayoutRubyBase* right_base = right_run->RubyBaseSafe();
LayoutRubyBase* right_base = right_run->RubyBaseSafe();
if (right_base->FirstChild()) {
// Collect all children in a single base, then swap the bases.
right_base->MoveChildren(base);
MoveChildTo(right_run, base);
right_run->MoveChildTo(this, right_base);
// The now empty ruby base will be removed below.
DCHECK(!RubyBase()->FirstChild());
}
}
Expand All @@ -176,16 +176,12 @@ void LayoutRubyRun::RemoveChild(LayoutObject* child) {
LayoutBlockFlow::RemoveChild(child);

if (!BeingDestroyed() && !DocumentBeingDestroyed()) {
// Check if our base (if any) is now empty. If so, destroy it.
// If this has only an empty LayoutRubyBase, destroy this sub-tree.
LayoutBlockFlow* base = RubyBase();
if (base && !base->FirstChild()) {
if (!HasRubyText() && !base->FirstChild()) {
LayoutBlockFlow::RemoveChild(base);
base->DeleteLineBoxTree();
base->Destroy();
}

// If any of the above leaves the run empty, destroy it as well.
if (!HasRubyText() && !HasRubyBase()) {
DeleteLineBoxTree();
Destroy();
}
Expand All @@ -200,6 +196,7 @@ LayoutRubyBase* LayoutRubyRun::CreateRubyBase() const {
GetDocument().GetStyleResolver().CreateAnonymousStyleWithDisplay(
StyleRef(), EDisplay::kBlock);
new_style->SetTextAlign(ETextAlign::kCenter); // FIXME: use WEBKIT_CENTER?
new_style->SetHasLineIfEmpty(true);
layout_object->SetStyle(std::move(new_style));
return layout_object;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,8 @@ crbug.com/1035708 virtual/css-highlight-inheritance/external/wpt/css/css-pseudo/
crbug.com/1035708 virtual/css-highlight-inheritance/external/wpt/css/css-pseudo/selection-originating-decoration-color.html [ Failure ]

### external/wpt/css/css-ruby/
crbug.com/591099 external/wpt/css/css-ruby/abs-in-ruby-base-container.html [ Failure ]
crbug.com/591099 external/wpt/css/css-ruby/abs-in-ruby-base.html [ Failure ]
crbug.com/591099 external/wpt/css/css-ruby/br-clear-all-002.html [ Failure ]
crbug.com/591099 external/wpt/css/css-ruby/ruby-line-breaking-002.html [ Failure ]
crbug.com/591099 external/wpt/css/css-ruby/ruby-with-floats-002.html [ Failure ]
Expand Down
5 changes: 1 addition & 4 deletions third_party/blink/web_tests/TestExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -1183,8 +1183,6 @@ crbug.com/1238243 external/wpt/css/css-tables/collapsed-scroll-overflow.html [ F

crbug.com/880802 external/wpt/css/css-contain/contain-layout-017.html [ Failure ]
crbug.com/868704 external/wpt/css/css-contain/contain-layout-breaks-002.html [ Failure ]
crbug.com/847274 external/wpt/css/css-contain/contain-paint-005.html [ Failure ]
crbug.com/847274 external/wpt/css/css-contain/contain-paint-006.html [ Failure ]
crbug.com/880802 external/wpt/css/css-contain/contain-paint-021.html [ Failure ]
crbug.com/882367 external/wpt/css/css-contain/contain-paint-clip-015.html [ Failure ]
crbug.com/882367 external/wpt/css/css-contain/contain-paint-clip-016.html [ Failure ]
Expand Down Expand Up @@ -2993,8 +2991,6 @@ crbug.com/1110401 external/wpt/css/css-pseudo/active-selection-045.html [ Failur
crbug.com/1024156 external/wpt/css/css-pseudo/active-selection-027.html [ Failure ]
crbug.com/1024156 external/wpt/css/css-pseudo/active-selection-025.html [ Failure ]

crbug.com/27659 external/wpt/css/css-ruby/abs-in-ruby-base-container.html [ Failure ]
crbug.com/27659 external/wpt/css/css-ruby/abs-in-ruby-base.html [ Failure ]
crbug.com/27659 external/wpt/css/css-ruby/block-ruby-001.html [ Failure ]
crbug.com/27659 external/wpt/css/css-ruby/block-ruby-002.html [ Failure ]
crbug.com/27659 external/wpt/css/css-ruby/block-ruby-005.html [ Failure ]
Expand All @@ -3012,6 +3008,7 @@ crbug.com/27659 external/wpt/css/css-ruby/ruby-align-002a.html [ Failure ]
crbug.com/27659 external/wpt/css/css-ruby/ruby-annotation-pairing-001.html [ Failure ]
crbug.com/27659 external/wpt/css/css-ruby/ruby-base-container-abs.html [ Failure ]
crbug.com/27659 external/wpt/css/css-ruby/ruby-base-container-float.html [ Failure ]
crbug.com/27659 external/wpt/css/css-ruby/ruby-base-different-size.html [ Failure ]
crbug.com/27659 external/wpt/css/css-ruby/ruby-bidi-002.html [ Failure ]
crbug.com/27659 external/wpt/css/css-ruby/ruby-bidi-003.html [ Failure ]
crbug.com/27659 external/wpt/css/css-ruby/ruby-box-generation-001.html [ Failure ]
Expand Down
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file not shown.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit 12434db

Please sign in to comment.