Bug 132241

Summary: REGRESSION (r159345): The hover state for links in the top navigation of Yahoo.com doesn't work
Product: WebKit Reporter: Darin Adler <darin>
Component: Layout and RenderingAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, kling, koivisto, kondapallykalyan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch kling: review+

Description Darin Adler 2014-04-27 20:01:04 PDT
REGRESSON (r159344): The hover state for links in the top navigation of Yahoo.com doesn't work
Comment 1 Darin Adler 2014-04-27 20:09:04 PDT
Created attachment 230274 [details]
Patch
Comment 2 Darin Adler 2014-04-27 20:16:30 PDT
Created attachment 230275 [details]
Patch
Comment 3 Darin Adler 2014-04-27 20:33:49 PDT
I’ll fix the title of the bug in the patch before landing it.
Comment 4 Andreas Kling 2014-04-27 21:16:31 PDT
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.
Comment 5 Darin Adler 2014-04-27 21:19:37 PDT
Committed r167870: <http://trac.webkit.org/changeset/167870>
Comment 6 Antti Koivisto 2014-05-10 05:00:46 PDT
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?
Comment 7 Darin Adler 2014-05-10 10:51:56 PDT
(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.
Comment 8 Antti Koivisto 2014-05-21 11:24:08 PDT
https://bugs.webkit.org/show_bug.cgi?id=133155 reverts and fixes this by layout invalidation.