Issue 2095833003: :active pseudo class is not applied for element with display none.

Issue 2095833003: :active pseudo class is not applied for element with display none. (Closed)

Created:
4 years, 6 months ago by ramya.v

Modified:
4 years, 5 months ago

Reviewers:
Timothy Loh, rune

CC:
chromium-reviews, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

:active pseudo class is not applied for element with display none. Handled for display none case. BUG=619206 Committed: https://crrev.com/86a54a0ee007a875d44bc4aebb7a3ad5d0cfae69 Cr-Commit-Position: refs/heads/master@{#402425}

Patch Set 1 #

Patch Set 2 : Added testcase #

Total comments: 10

Patch Set 3 : Updated as per review comments #

Patch Set 4 : Moved LocalStyleChange change to setActive from setHovered #

Patch Set 5 : Updated test case which require LocalStyleChange calculation from both setActive and setHover #

Created: 4 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+80 lines, -7 lines)			Patch
A	third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html	View	1 2	1 chunk	+32 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css/pseudo-hover-active-display-none.html	View	1 2 3 4	1 chunk	+33 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css/pseudo-hover-active-display-none-expected.html	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/ContainerNode.cpp	View	1 2 3 4	1 chunk	+12 lines, -7 lines	0 comments	Download

Messages

Total messages: 18 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

rune

https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html File third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html (right): https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html#newcode3 third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html:3: <head> Drop html and head. https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html#newcode18 third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html:18: <body> drop ...

4 years, 6 months ago (2016-06-24 12:28:51 UTC) #3

ramya.v

PTAL! Thanks https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html File third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html (right): https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html#newcode3 third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html:3: <head> On 2016/06/24 12:28:51, rune wrote: > ...

4 years, 5 months ago (2016-06-27 06:28:01 UTC) #4

PTAL!

Thanks

https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html
(right):

https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html:3:
<head>
On 2016/06/24 12:28:51, rune wrote:
> Drop html and head.

Done.

https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html:18:
<body>
On 2016/06/24 12:28:51, rune wrote:
> drop </head> and <body>

Done.

https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html:22: if
(window.eventSender) {
On 2016/06/24 12:28:51, rune wrote:
> I'd convert this if into a test. E.g.:
> 
>     test(function () {
>         assert_true(window.eventSender !== null);
>     }, "window.eventSender is required for the test to run");

Done.

https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html:36:
</html>
On 2016/06/24 12:28:51, rune wrote:
> Drop </body> and </html>

Done.

https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2095833003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:1031:
toElement(this)->pseudoStateChanged(CSSSelector::PseudoActive);
On 2016/06/24 12:28:51, rune wrote:
> The setHovered() method has an else here where it sets a LocalStyleChange. Did
> you figure out why?
> 
> If there is a good reason why they're different we can keep it like this.
> Otherwise that else should be added here or removed from setHovered. Whichever
> is correct.
> 

The else case is required in setHovered method, without which the below test
fails.
src/third_party/WebKit/LayoutTests/fast/css/clear-activechain-list-shadow-dom.html

Ex: Created a simple test case from above test, 
https://jsfiddle.net/8406tbqm/
If we click on Click Me span element, if we don't have else implemented in
setHovered, after mouseup also the text is not displayed. (its active state is
not cleared)

However could not find a testcase where else part would be required in setActive
function. Could you please let me know if this would be required in any
particular scenario?

rune

On 2016/06/27 06:28:01, ramya.v wrote: > The else case is required in setHovered method, without ...

4 years, 5 months ago (2016-06-27 07:54:41 UTC) #5

ramya.v

On 2016/06/27 07:54:41, rune wrote: > On 2016/06/27 06:28:01, ramya.v wrote: > > > The ...

4 years, 5 months ago (2016-06-27 09:19:27 UTC) #6

rune

On 2016/06/27 09:19:27, ramya.v wrote: > Moved else part from setHovered to setActive in latest ...

4 years, 5 months ago (2016-06-27 09:39:18 UTC) #7

ramya.v

On 2016/06/27 09:39:18, rune wrote: > On 2016/06/27 09:19:27, ramya.v wrote: > > > Moved ...

4 years, 5 months ago (2016-06-27 12:01:55 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2095833003/80001

4 years, 5 months ago (2016-06-28 03:36:19 UTC) #11

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/253552)

4 years, 5 months ago (2016-06-28 06:07:04 UTC) #13

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2095833003/80001

4 years, 5 months ago (2016-06-28 06:09:26 UTC) #15

commit-bot: I haz the power

4 years, 5 months ago (2016-06-28 08:41:02 UTC) #18

Message was sent while issue was closed.

Patchset 5 (id:??) landed as
https://crrev.com/86a54a0ee007a875d44bc4aebb7a3ad5d0cfae69
Cr-Commit-Position: refs/heads/master@{#402425}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages