Bug 129465 - :active style is not cleared when its display property is set to none before mouse released.
Summary: :active style is not cleared when its display property is set to none before ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-27 20:33 PST by Sanghyup Lee
Modified: 2014-03-03 22:32 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.65 KB, patch)
2014-02-27 20:35 PST, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch (3.90 KB, patch)
2014-02-28 23:52 PST, Sanghyup Lee
no flags Details | Formatted Diff | Diff
Patch (4.50 KB, patch)
2014-03-01 19:18 PST, Sanghyup Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sanghyup Lee 2014-02-27 20:33:48 PST
We currently clearing the :active style when element has a renderer.
This patch makes elements active style without renderer cleared.
Comment 1 Sanghyup Lee 2014-02-27 20:35:05 PST
Created attachment 225433 [details]
Patch
Comment 2 Andreas Kling 2014-02-28 01:49:01 PST
Comment on attachment 225433 [details]
Patch

It's great that you are fixing this bug, but we should add a layout test so it doesn't regress again.
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
Comment 3 Sanghyup Lee 2014-02-28 23:52:21 PST
Created attachment 225542 [details]
Patch
Comment 4 Darin Adler 2014-03-01 16:11:43 PST
Comment on attachment 225542 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:5839
> +        if (!oldActiveElement->renderer())
> +            oldActiveElement->setActive(false);

This code change seems like only a partial fix. Last time Document::updateHoverActiveState was called, we called setActive(true) on multiple elements. It seems that elements that were ancestors of the main active one will have the same bug as before. We should construct test cases that cover that and fix the whole problem instead of patching over the most obvious surface part of the problem.

There’s no real need for the !oldActiveElement->renderer() check. It would be harmless and clearer to call oldActiveElement->setActive(false) unconditionally. It’s hard to understand why we would do this only for an element without a renderer.

Slightly ugly to fetch the renderer twice, here and for the loop just below it.
Comment 5 Sanghyup Lee 2014-03-01 19:18:08 PST
Created attachment 225580 [details]
Patch
Comment 6 Antonio Gomes 2014-03-02 07:36:01 PST
Comment on attachment 225580 [details]
Patch

r=me
Comment 7 Gyuyoung Kim 2014-03-03 22:01:30 PST
Comment on attachment 225580 [details]
Patch

cq=me. It looks latest patch is fixed according to Darin's comment.
Comment 8 WebKit Commit Bot 2014-03-03 22:32:04 PST
Comment on attachment 225580 [details]
Patch

Clearing flags on attachment: 225580

Committed r165037: <http://trac.webkit.org/changeset/165037>
Comment 9 WebKit Commit Bot 2014-03-03 22:32:07 PST
All reviewed patches have been landed.  Closing bug.