Bug 132241 – REGRESSION (r159345): The hover state for links in the top navigation of Yahoo.com doesn't work

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Darin Adler]

REGRESSON (r159344): The hover state for links in the top navigation of Yahoo.com doesn't work

[Darin Adler]

Created attachment 230274 [details]
Patch

[Darin Adler]

Created attachment 230275 [details]
Patch

[Darin Adler]

I’ll fix the title of the bug in the patch before landing it.

[Andreas Kling]

Comment on attachment 230275 [details]
Patch

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

r=me. perf.webkit.org will tell us if out-of-lining simpleLineLayout() really matters.

> Source/WebCore/ChangeLog:3
> +        REGRESSION (r159344): The hover state for links in the top navigation of Yahoo.com doesn't work

This should be r159345.

[Darin Adler]

Committed r167870: <http://trac.webkit.org/changeset/167870>

[Antti Koivisto]

Comment on attachment 230275 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        REGRESSION (r159344): The hover state for links in the top navigation of Yahoo.com doesn't work
> +        https://bugs.webkit.org/show_bug.cgi?id=132241
> +        rdar://problem/16501924
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/text/simple-lines-hover-underline.html

This ChangeLog entry does not explain how this change fixes the bug.

> Source/WebCore/rendering/RenderBlockFlow.cpp:3360
> +const SimpleLineLayout::Layout* RenderBlockFlow::simpleLineLayout() const
> +{
> +    if (m_lineLayoutPath == UndeterminedPath)
> +        const_cast<RenderBlockFlow&>(*this).m_lineLayoutPath = SimpleLineLayout::canUseFor(*this) ? SimpleLinesPath : LineBoxesPath;
> +
> +    if (m_lineLayoutPath == SimpleLinesPath)
> +        return m_simpleLineLayout.get();
> +
> +    const_cast<RenderBlockFlow&>(*this).createLineBoxes();
> +    return nullptr;
> +}

This is pretty terrible. Why all the const breakage? Why not use mutable? Why does this have side effect of creating lineboxes but not simple lines?

[Darin Adler]

(In reply to comment #6)
> This ChangeLog entry does not explain how this change fixes the bug.

Sorry about that.

What this fixes is that m_lineLayoutPath is set to UndeterminedPath, but the code continues to use the simple line layout until and unless RenderBlockFlow::layoutInlineChildren is called. I don’t understand all the details thoroughly. I’d be happy if you could find a better fix.

> This is pretty terrible. Why all the const breakage? Why not use mutable? Why does this have side effect of creating lineboxes but not simple lines?

I’d be fine with it if you roll this out and fix the problem another way.

[Antti Koivisto]

https://bugs.webkit.org/show_bug.cgi?id=133155 reverts and fixes this by layout invalidation.