Bug 189119 – Remove redundant inline text boxes for empty combined text

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Daniel Bates]

While investigating bug #184390 I discovered that we are creating empty inline text boxes for combined text. We should look to avoid doing this both to save memory and make it easier to reason about painting of text.

[Daniel Bates]

An empty inline text box does not visually effect the rendering of the page.

[Daniel Bates]

Created attachment 348448 [details]
Patch

[Daniel Bates]

(In reply to Daniel Bates from comment #0)
> While investigating bug #184390 I discovered that we are creating empty
> inline text boxes for combined text. We should look to avoid doing this both
> to save memory and make it easier to reason about painting of text.

To be more precise: I discovered that our logic for removing "redundant" inline text boxes does not remove inline text boxes for empty combined texts. It does remove inline text boxes for non-combined text that are empty. We should consider inline text boxes with empty combined text to also be empty and remove such boxes because they have not visual appearance, take up memory, and make it harder to reason about painting of the inline text box tree (since we currently need to explicitly consider the case of an inline text box with a RenderCombineText renderer whose combined text string is the empty string as opposed to the higher level concept of whether the box is visually empty).

[Daniel Bates]

For those not familiar with the concept of combined text. Combined text is defined to be "the combination of multiple typographic character units into the space of a single typographic character unit. The resulting composition is treated as a single upright glyph for the purposes of layout and decoration." (https://drafts.csswg.org/css-writing-modes-4/#propdef-text-combine-upright; Editor's Draft, 17 August 2018). We model this in the engine with an inline text box that has length 1 (InlineTextBox::len() == 1) and its associated renderer (RenderCombineText) stores the resulting composition to be rendered in the box (RenderCombineText::combinedStringForRendering()).

Additional remarks:

Notice that inline text boxes hang off of renderers and we only create a text renderer if the underlying DOM text node is non-empty by RenderTreeUpdater::textRendererIsNeeded().

DOM operations, including Range.surroundContents(), can mutate both the DOM text nodes and the text in the associated renderers such that the composed string stored in a RenderCombineText can become the empty string (RenderCombineText::combinedStringForRendering().isEmpty() evaluates to true).

[Daniel Bates]

Comment on attachment 348448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348448&action=review

> Source/WebCore/ChangeLog:10
> +        An empty inline text box does not visually effect the rendering of the page. We should not
> +        create such text boxes both to reduce memory and to make it easier to reason about the line
> +        box tree.

Will update this before landing to read:

We should consider inline text boxes that have a combined text renderer (RenderCombineText) whose composed string is empty as "redundant" just as we do for inline text boxes that have a non-combined text renderer that have zero length so that we remove them. Such boxes are visibly empty and do not take up space visually. By removing them we reduce memory and make it easier to reason about the line box tree.

Currently RenderBlockFlow::computeBlockDirectionPositionsForLine() tests if an inline text box is empty by checking if it has a zero length (InlineTextBox::len()). However an inline text box associated with a RenderCombineText always has length 1 regardless of whether the composed string it represents is the empty string. Instead we should expose a way to check if an inline text box is visually empty and have RenderBlockFlow::computeBlockDirectionPositionsForLine() query the inline text box for this answer.

[Daniel Bates]

Created attachment 348510 [details]
Patch

Updated ChangeLogs to reflect updated title and my remarks in comment 5.

[Brent Fulgham]

Comment on attachment 348510 [details]
Patch

I think this change looks good, but I'd like Alan or Myles to give it a look, too.

[alan]

Comment on attachment 348510 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348510&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:79
> +bool InlineTextBox::isEmpty() const

In some cases we use "empty" in the context of something is _visually_ empty. I'd rather try the term hasTextContent() or something along those lines.

[Daniel Bates]

(In reply to zalan from comment #8)
> Comment on attachment 348510 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348510&action=review
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:79
> > +bool InlineTextBox::isEmpty() const
> 
> In some cases we use "empty" in the context of something is _visually_
> empty. I'd rather try the term hasTextContent() or something along those
> lines.

Will rename isEmpty() to hasTextContent() before landing.

[Daniel Bates]

(In reply to Daniel Bates from comment #9)
> (In reply to zalan from comment #8)
> > Comment on attachment 348510 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=348510&action=review
> > 
> > > Source/WebCore/rendering/InlineTextBox.cpp:79
> > > +bool InlineTextBox::isEmpty() const
> > 
> > In some cases we use "empty" in the context of something is _visually_
> > empty. I'd rather try the term hasTextContent() or something along those
> > lines.
> 
> Will rename isEmpty() to hasTextContent() before landing.

Clearly, I will also need to invert the logic in InlineTextBox::isEmpty() such that it return true if the text box has content and false otherwise.

[Daniel Bates]

Committed r235615: <https://trac.webkit.org/changeset/235615>

[Radar WebKit Bug Importer]

<rdar://problem/44101162>

[Daniel Bates]

Committed attempt to fix layout tests in <https://trac.webkit.org/changeset/235621>.