Bug 132241 - REGRESSION (r159345): The hover state for links in the top navigation of Yahoo.com doesn't work
Summary: REGRESSION (r159345): The hover state for links in the top navigation of Yaho...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-27 20:01 PDT by Darin Adler
Modified: 2014-05-21 11:24 PDT (History)
6 users (show)

See Also:

Attachments
Patch (9.93 KB, patch)
2014-04-27 20:09 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (9.96 KB, patch)
2014-04-27 20:16 PDT, Darin Adler 		kling: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.