Bug 213052

Summary:

[Line clamp] Do not apply the special anchor handling when the anchor content is visible after clamping

Product:

WebKit

Reporter:

zalan <zalan>

Component:

Layout and Rendering

Assignee:

zalan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, changseok, darin, ddkilzer, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, simon.fraser, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none

Description zalan 2020-06-10 15:10:45 PDT

<rdar://problem/59739131>

Comment 1 zalan 2020-06-10 15:34:02 PDT

Created attachment 401592 [details]
Patch

Comment 2 Darin Adler 2020-06-10 16:16:14 PDT

Comment on attachment 401592 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Line clamping tries to preserve the anchor text if it is at the bottom of the paragraph to support cases like "... Read mode", where the "read more" is an actual link.

mode/more?

Comment 3 zalan 2020-06-10 19:25:25 PDT

Created attachment 401615 [details]
Patch

Comment 4 EWS 2020-06-10 20:01:54 PDT

Committed r262892: <https://trac.webkit.org/changeset/262892>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401615 [details].

Comment 5 David Kilzer (:ddkilzer) 2020-06-11 11:01:42 PDT

Comment on attachment 401615 [details]
Patch

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

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:1004
> -        if (anchorBox && anchorBox->renderer().style().isLink())
> +        auto& anchorRenderer = anchorBox->renderer();
> +        auto& lastVisibleRenderer = lastVisibleLine->firstChild()->renderer();
> +        if (anchorBox && anchorBox->renderer().style().isLink() && &lastVisibleRenderer != &anchorRenderer)

Is Line 1002 (auto& anchorRenderer = anchorBox->renderer();) susceptible to a nullptr crash since you're no longer checking `anchorBox` before dereferencing it?

Comment 6 zalan 2020-06-11 11:05:55 PDT

(In reply to David Kilzer (:ddkilzer) from comment #5)
> Comment on attachment 401615 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401615&action=review
> 
> > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:1004
> > -        if (anchorBox && anchorBox->renderer().style().isLink())
> > +        auto& anchorRenderer = anchorBox->renderer();
> > +        auto& lastVisibleRenderer = lastVisibleLine->firstChild()->renderer();
> > +        if (anchorBox && anchorBox->renderer().style().isLink() && &lastVisibleRenderer != &anchorRenderer)
> 
> Is Line 1002 (auto& anchorRenderer = anchorBox->renderer();) susceptible to
> a nullptr crash since you're no longer checking `anchorBox` before
> dereferencing it?
It should never really happen but I'll add a check. Thanks.

Comment 7 zalan 2020-06-11 11:09:02 PDT

(In reply to zalan from comment #6)
> (In reply to David Kilzer (:ddkilzer) from comment #5)
> > Comment on attachment 401615 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=401615&action=review
> > 
> > > Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:1004
> > > -        if (anchorBox && anchorBox->renderer().style().isLink())
> > > +        auto& anchorRenderer = anchorBox->renderer();
> > > +        auto& lastVisibleRenderer = lastVisibleLine->firstChild()->renderer();
> > > +        if (anchorBox && anchorBox->renderer().style().isLink() && &lastVisibleRenderer != &anchorRenderer)
> > 
> > Is Line 1002 (auto& anchorRenderer = anchorBox->renderer();) susceptible to
> > a nullptr crash since you're no longer checking `anchorBox` before
> > dereferencing it?
> It should never really happen but I'll add a check. Thanks.
The InlineBox interface should probably be changed to return references.