Bug 179114 – First letter text renderer should be anonymous

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

Currently both RenderTextFragment and RenderText for first letter point to the same Text node. There should only be one non-anonymous renderer per node.

[Antti Koivisto]

Created attachment 325569 [details]
patch

[Build Bot]

Attachment 325569 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderBoxModelObject.cpp:2553:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 325570 [details]
patch

[Antti Koivisto]

Created attachment 325571 [details]
patch

[Build Bot]

Comment on attachment 325571 [details]
patch

Attachment 325571 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5062417

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 325573 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Build Bot]

Comment on attachment 325571 [details]
patch

Attachment 325571 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5062479

New failing tests:
fast/css-generated-content/initial-letter-border-padding.html
fast/css-generated-content/initial-letter-basic.html
fast/css-generated-content/initial-letter-raised.html
fast/css-generated-content/initial-letter-sunken.html
ietestcenter/css3/text/textshadow-004.htm

[Build Bot]

Created attachment 325574 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325571 [details]
patch

Attachment 325571 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5062493

New failing tests:
ietestcenter/css3/text/textshadow-004.htm
fast/css-generated-content/initial-letter-basic.html
imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-match.https.html
svg/wicd/test-rightsizing-a.xhtml
fast/css-generated-content/initial-letter-raised.html
fast/css-generated-content/initial-letter-border-padding.html
fast/css-generated-content/initial-letter-sunken.html

[Build Bot]

Created attachment 325575 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Antti Koivisto]

Created attachment 325576 [details]
patch

[Darin Adler]

Comment on attachment 325576 [details]
patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:131
> +typedef HashMap<const RenderBoxModelObject*, WeakPtr<RenderTextFragment>> FirstLetterRemainingTextMap;

In new code it’s nice to use "using" instead of typedef.

[zalan]

Comment on attachment 325576 [details]
patch

We should tighten the first letter type here. It's odd that we set it on the RenderElement but use it in RenderBoxModelObject for cleanup (I know it's not a new code). I also believe we could even use a more concrete renderer type for all this.

[Build Bot]

Comment on attachment 325576 [details]
patch

Attachment 325576 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5062977

New failing tests:
ietestcenter/css3/text/textshadow-004.htm
fast/selectors/166a.html
fast/text/firstline/003.html
fast/css/first-letter-first-line-hover.html
fast/css/first-letter-hover.html
fast/css/first-letter-detach.html
fast/css/first-letter-float-after-float.html
fast/css/first-letter-visibility.html
fast/css/first-letter-capitalized.html
fast/css/first-letter-recalculation.html
fast/css/first-letter-float.html
fast/selectors/039b.html
fast/multicol/shadow-breaking.html
fast/text/firstline/002.html
fast/css/first-letter-punctuation.html
fast/selectors/039.html

[Build Bot]

Created attachment 325581 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Antti Koivisto]

(In reply to zalan from comment #13)
> Comment on attachment 325576 [details]
> patch
> 
> We should tighten the first letter type here. It's odd that we set it on the
> RenderElement but use it in RenderBoxModelObject for cleanup (I know it's
> not a new code). I also believe we could even use a more concrete renderer
> type for all this.

RenderBoxModelObject is the most concrete type of the first letter renderer as it can be either RenderInline or RenderBlock. 

RenderElement is used for bits simply because that's where we currently keep all the bits.

[Antti Koivisto]

> RenderElement is used for bits simply because that's where we currently keep
> all the bits.

We could make the setters available only in RBMO though.

[Simon Fraser (smfr)]

Comment on attachment 325576 [details]
patch

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

> Source/WebCore/rendering/RenderElement.h:344
> +    unsigned m_isFirstLetter : 1;

Is it worth using a RenderElement bit here? I think first letter is rare enough that you could use RenderObject::rareData().

[Antti Koivisto]

> Is it worth using a RenderElement bit here? I think first letter is rare
> enough that you could use RenderObject::rareData().

I don't think so. There are plenty of free bits in RenderElement and we could probably just add more too (renderer size doesn't appear to be a huge concern in memory or performance).

I'm also unconvinced that RareData is a good pattern in general. As you pile up stuff there you'll end up with a data structure that is large and not rare. See ElementRareData.

The traditional feature-bit-and-side-hashmap approach used all over render tree seems strictly better to me.

[Antti Koivisto]

Created attachment 325626 [details]
patch

[Build Bot]

Comment on attachment 325626 [details]
patch

Attachment 325626 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5066822

New failing tests:
fast/selectors/039b.html
fast/selectors/039.html

[Build Bot]

Created attachment 325639 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Antti Koivisto]

Created attachment 325696 [details]
patch

[WebKit Commit Bot]

Comment on attachment 325696 [details]
patch

Clearing flags on attachment: 325696

Committed r224324: <https://trac.webkit.org/changeset/224324>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35567715>