Bug 47815 – First-letter style is applied to the letters that were once first but not any more due to style change

• Component: CSS
• Status: NEW

[Yuzo Fujishima]

Created attachment 71025 [details]
Test

Some style changes, e.g., change in before pseudo-element content, can change what the first letter is.
Then first letter style should be applied only to the new first letters. But WebKit r69940 applies first
letter style to letters that are not first any more.

This can be though of as a part of 14550.
But this seems to be more difficult to fix from the comment in RenderBlock.cpp:
       // FIXME: We need to destroy the first-letter object if it is no longer the first child.  Need to find
       // an efficient way to check for that situation though before implementing anything.

Hence filing a separate bug should make sense.

[Yuzo Fujishima]

I meant we probably should first fix easier parts as 14550, and then fix this part as this bug.

[Yuzo Fujishima]

Created attachment 72162 [details]
Patch

[Yuzo Fujishima]

Created attachment 72297 [details]
Fixed first letter removal logic, updated test expectation

[Yuzo Fujishima]

Created attachment 72301 [details]
Fixed from where to call stale first letter check

[Yuzo Fujishima]

Ping?

[Yuzo Fujishima]

Created attachment 78496 [details]
Pre Source/WebCore move

[Yuzo Fujishima]

Created attachment 78635 [details]
Post Source/WebCore move

[Alexey Proskuryakov]

Duplicate of bug 15602?

[Yuzo Fujishima]

This bug is not specific to quirksmode, while 15602 is. Hence the two bugs should not be DUP.

[Eric Seidel (no email)]

Comment on attachment 78635 [details]
Post Source/WebCore move

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

> Source/WebCore/rendering/RenderBlock.cpp:5122
> +    RenderText* newText = new (renderArena()) RenderText(remainingTextFragment->node() ? remainingTextFragment->node() : remainingTextFragment->document(), remainingTextFragment->fullOriginalText());

Oh please.  There must be a RenderTextFragment method to create this?

> Source/WebCore/rendering/RenderBlock.cpp:5123
> +    newText->setStyle(remainingTextFragment->style());

And this.

> Source/WebCore/rendering/RenderBlock.cpp:5128
> +    view()->disableLayoutState();

Don't we have RIIA to do this eanble/disable?

> Source/WebCore/rendering/RenderBlock.cpp:5255
> +    removeStaleFirstLetterForFirstLetterBlock(firstLetterBlock);

What's the runtime effect of adding this child-walk here?

> Source/WebCore/rendering/RenderBlock.cpp:5547
> +        m_rareData = new RenderBlockRareData(this);

Is this OK with the new safe OwnPtrs?

[Ryosuke Niwa]

Comment on attachment 78635 [details]
Post Source/WebCore move

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

> Source/WebCore/ChangeLog:7
> +        Fix for Bug 47815 - First-letter style is applied to the letters that were once first but not any more due to style change
> +        Record if the first letter is created for a block. Scan for and remove stale first letter when style changed.
> +        https://bugs.webkit.org/show_bug.cgi?id=47815

A change log entry should have a bug title, followed by a bug url, and then a change description.

> Source/WebCore/rendering/RenderTextFragment.cpp:55
>      Node* e = node();
> -    RefPtr<StringImpl> result = ((e && e->isTextNode()) ? static_cast<Text*>(e)->dataImpl() : contentString());
> +    return ((e && e->isTextNode()) ? static_cast<Text*>(e)->dataImpl() : contentString());

I now you didn't originally write this function but please do not use one letter variable.

[Yuzo Fujishima]

Created attachment 94924 [details]
Address comments from Eric and Ryosuke

[Yuzo Fujishima]

Thank you for the review.

(In reply to comment #10)
> (From update of attachment 78635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78635&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:5122
> > +    RenderText* newText = new (renderArena()) RenderText(remainingTextFragment->node() ? remainingTextFragment->node() : remainingTextFragment->document(), remainingTextFragment->fullOriginalText());
> 
> Oh please.  There must be a RenderTextFragment method to create this?
> 
> > Source/WebCore/rendering/RenderBlock.cpp:5123
> > +    newText->setStyle(remainingTextFragment->style());
> 
> And this.

Added a method to RenderTextFragment to address these two comments.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:5128
> > +    view()->disableLayoutState();
> 
> Don't we have RIIA to do this eanble/disable?

Added LayoutStateDisabler (bug 61352) and changed to use it here.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:5255
> > +    removeStaleFirstLetterForFirstLetterBlock(firstLetterBlock);
> 
> What's the runtime effect of adding this child-walk here?

The recursion occurs only for a block for which first letter RenderTextFragment has been created.
For such blocks, recursion is an inevitable price to pay, I believe. (I'm open to ideas for better approaches.)

> 
> > Source/WebCore/rendering/RenderBlock.cpp:5547
> > +        m_rareData = new RenderBlockRareData(this);
> 
> Is this OK with the new safe OwnPtrs?

No.
Fixed.

[Yuzo Fujishima]

Thank you for the review.

(In reply to comment #11)
> (From update of attachment 78635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78635&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        Fix for Bug 47815 - First-letter style is applied to the letters that were once first but not any more due to style change
> > +        Record if the first letter is created for a block. Scan for and remove stale first letter when style changed.
> > +        https://bugs.webkit.org/show_bug.cgi?id=47815
> 
> A change log entry should have a bug title, followed by a bug url, and then a change description.
> 

Done.

> > Source/WebCore/rendering/RenderTextFragment.cpp:55
> >      Node* e = node();
> > -    RefPtr<StringImpl> result = ((e && e->isTextNode()) ? static_cast<Text*>(e)->dataImpl() : contentString());
> > +    return ((e && e->isTextNode()) ? static_cast<Text*>(e)->dataImpl() : contentString());
> 
> I now you didn't originally write this function but please do not use one letter variable.

Done.

[Yuzo Fujishima]

I'd like to release the ownership.

[Ryosuke Niwa]

Is this bug still valid? Isn't your patch obsolete at this point?

[Darin Adler]

Comment on attachment 94924 [details]
Address comments from Eric and Ryosuke

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

Code looks generally OK, but there seem to be a few problems.

> Source/WebCore/rendering/RenderBlock.cpp:5299
> +    RenderObject* nextObj = remainingTextFragment->nextSibling();

Please don’t use abbreviations like “obj” in new code in WebKit. We prefer whole words.

> Source/WebCore/rendering/RenderBlock.h:195
> +    bool isFirstLetterCreated() const { return m_rareData ? m_rareData->m_isFirstLetterCreated : false; }

Naming here is not right.

When we have a boolean member function, we name it so that it works in a phrase like this “<class> <function>”, in this case “render block <function>”. But the phrase “render block is first letter created” is nonsense, showing us that it’s not a good name. One good name might be “has first letter block”, which would be written like this: hasFirstLetterBlock.

The code says this is used to avoid unnecessary searching, but

> Source/WebCore/rendering/RenderBlock.h:353
> +    void removeFirstLetter(RenderObject* firstLetter);

These functions that use the words “first letter” for the “first letter block” are really confusing. Such functions do not “remove the first letter”, so it’s confusing to call them “remove first letter”.

> Source/WebCore/rendering/RenderBlock.h:355
> +    void removeFirstLetter(RenderObject* firstLetter);
> +    bool removeStaleFirstLetter(RenderObject* container);
> +    void removeStaleFirstLetterForFirstLetterBlock(RenderObject* firstLetterBlock);

All three of these functions seem like they should be static member functions. None of them seem to use the “this” pointer.

> Source/WebCore/rendering/RenderTextFragment.cpp:52
> +PassRefPtr<StringImpl> RenderTextFragment::fullOriginalText() const

It is incorrect for this function to return a PassRefPtr. It does not produce a new object.

> Source/WebCore/rendering/RenderTextFragment.cpp:54
> +    Node* domNode = node();

WebKit coding style in cases like this is to do this:

    Node* node = this->node();

That way we don’t end up with awkward names like domNode.

> Source/WebCore/rendering/RenderTextFragment.cpp:55
> +    return ((domNode && domNode->isTextNode()) ? static_cast<Text*>(domNode)->dataImpl() : contentString());

We normally would not use those parentheses in WebKit coding style.

> Source/WebCore/rendering/RenderTextFragment.cpp:66
> +RenderText* RenderTextFragment::reproduceRenderText(RenderArena* renderArena)

The verb “reproduce” is not really an apt verb to use here. In cases like this we sometimes use the verb “clone”, but I think the function name ideally needs to be considerably clearer on what kind of RenderText this creates. I’m also not clear that it makes sense to have this function be a member function of RenderTextFragment. Perhaps it could be a RenderText static member function that takes a RenderTextFragment* argument instead.

> Source/WebCore/rendering/RenderTextFragment.cpp:110
> +        Node* domNode = node();
> +        StringImpl* original = ((domNode && domNode->isTextNode()) ? static_cast<Text*>(domNode)->dataImpl() : contentString());

This is a copy of the new fullOriginalText function. We do not want two copies of this code.

> Source/WebCore/rendering/RenderTextFragment.h:51
> +    PassRefPtr<StringImpl> fullOriginalText() const;

Why does this need to be a public function? It’s not used outside the class at this time.

[Radar WebKit Bug Importer]

<rdar://problem/96908042>

[Brent Fulgham]

Safari incorrectly handles the first test case line. The others match. Firefox and Chrome work properly.