Issue 16599003: :hover style not applied on hover if its display property is different from original style's

Issue 16599003: :hover style not applied on hover if its display property is different from original style's (Closed)

Created:
7 years, 6 months ago by stavila

Modified:
7 years, 6 months ago

Reviewers:
Julien - ping for review, eseidel, esprehn, eae, ojan

CC:
blink-reviews, chromiumbugtracker_adobe.com, webcomponents-bugzilla_chromium.org, eae+blinkwatch, feature-media-reviews_chromium.org, dglazkov+blink, f(malita), adamk+blink_chromium.org, pdr, Stephen Chennney, vcarbune.chromium

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

Visibility:
Public.

More Reviews

Description

Migrated from WebKit Bugzilla: https://bugs.webkit.org/show_bug.cgi?id=7555 :hover style not applied on hover if its display property is different from original style's Properly apply the :hover pseudo-class when reattaching is required (e.g. when changing the display type) A new AttachContext class was created to be passed along as an optional parameter to the attach/detach/reattach methods. This new parameter is used to: - prevent the element from being removed from the list of hovered/active elements upon detach when a reattach is in progress - prevent the style from being incorrectly computed (due to the previous point) - prevent the style from being computed twice (the attach() method used to recompute it) Special care was required to the case when display:none is specified in the :hover class. Enabling the :hover style was leaving the element without a renderer, which was causing it to remain stuck in the :hover state (subsequent mouseMove events were not able to reset the element to its normal style due to the fact that it had no renderer). The DragController::startDrag method was updated to properly handle the case when dragImage is NULL (for instance by setting display:none inside the -webkit-drag pseudo-class). New tests: - fast/regions/hover-display-block-inline.html - fast/regions/hover-display-block-none.html BUG=247375 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152289

Patch Set 1 #

Patch Set 2 : Patch #

Total comments: 7

Patch Set 3 : Patch #

Patch Set 4 : Patch (fixed test that was expected to fail and is now passing) #

Created: 7 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+429 lines, -156 lines)			Patch
A	LayoutTests/fast/css/focus-display-block-inline.html	View	1 2	1 chunk	+64 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css/focus-display-block-inline-expected.txt	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css/hover-display-block-inline.html	View	1 2	1 chunk	+78 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css/hover-display-block-inline-expected.txt	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css/hover-display-block-none.html	View	1 2	1 chunk	+76 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css/hover-display-block-none-expected.txt	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/dom/shadow/hover-active-drag-distributed-nodes-expected.txt	View	1 2 3	1 chunk	+3 lines, -3 lines	0 comments	Download
M	LayoutTests/fast/events/drag-display-none-element.html	View		2 chunks	+2 lines, -1 line	0 comments	Download
M	LayoutTests/fast/events/drag-display-none-element-expected.txt	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	Source/core/dom/ContainerNode.h	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/dom/ContainerNode.cpp	View	1 2	2 chunks	+16 lines, -4 lines	0 comments	Download
M	Source/core/dom/Document.h	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/dom/Document.cpp	View	1 2	4 chunks	+12 lines, -4 lines	0 comments	Download
M	Source/core/dom/Element.h	View	1 2	2 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/core/dom/Element.cpp	View	1 2	5 chunks	+11 lines, -9 lines	0 comments	Download
M	Source/core/dom/Node.h	View	1 2	2 chunks	+16 lines, -6 lines	0 comments	Download
M	Source/core/dom/Node.cpp	View	1 2	3 chunks	+13 lines, -9 lines	0 comments	Download
M	Source/core/dom/NodeRenderingContext.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/dom/NodeRenderingContext.cpp	View		1 chunk	+8 lines, -0 lines	0 comments	Download
M	Source/core/dom/PseudoElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/dom/PseudoElement.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/dom/Text.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/dom/Text.cpp	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/dom/shadow/InsertionPoint.h	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/dom/shadow/InsertionPoint.cpp	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	Source/core/dom/shadow/ShadowRoot.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/dom/shadow/ShadowRoot.cpp	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLCanvasElement.h	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLCanvasElement.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLFormControlElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLFormControlElement.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLFrameElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLFrameElement.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLFrameElementBase.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLFrameElementBase.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLFrameSetElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLFrameSetElement.cpp	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLImageElement.h	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLImageElement.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLInputElement.h	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLInputElement.cpp	View	1 2	2 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/core/html/HTMLLIElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLLIElement.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLMediaElement.h	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLMediaElement.cpp	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLOptGroupElement.h	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLOptGroupElement.cpp	View		2 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/core/html/HTMLOptionElement.h	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLOptionElement.cpp	View	1 2	2 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/core/html/HTMLPlugInElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLPlugInElement.cpp	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLPlugInImageElement.h	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLPlugInImageElement.cpp	View		4 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/core/html/HTMLProgressElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLProgressElement.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLTextAreaElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLTextAreaElement.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/HTMLVideoElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/HTMLVideoElement.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/PluginDocument.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/PluginDocument.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/shadow/ClearButtonElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/shadow/ClearButtonElement.cpp	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/shadow/PickerIndicatorElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/shadow/PickerIndicatorElement.cpp	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/shadow/SliderThumbElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/shadow/SliderThumbElement.cpp	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/shadow/SpinButtonElement.h	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/html/shadow/SpinButtonElement.cpp	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/shadow/TextControlInnerElements.h	View	1 2	3 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/core/html/shadow/TextControlInnerElements.cpp	View	1 2	3 chunks	+6 lines, -6 lines	0 comments	Download
M	Source/core/html/shadow/TextFieldDecorationElement.h	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/html/shadow/TextFieldDecorationElement.cpp	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	Source/core/page/DragController.cpp	View	1 2	1 chunk	+6 lines, -2 lines	0 comments	Download
M	Source/core/svg/SVGImageElement.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/svg/SVGImageElement.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 19 (0 generated)

Expand Messages | Collapse Messages

eseidel

This is a good patch for elliot. Unfortunately he's out this week. :(

7 years, 6 months ago (2013-06-07 20:00:46 UTC) #2

eae

Does this apply to other psedo-selectors as well? If so any it would be nice ...

7 years, 6 months ago (2013-06-11 19:56:32 UTC) #3

stavila

I'll make the changes you requested in the test files. As for the AttachContext structure, ...

7 years, 6 months ago (2013-06-11 21:08:50 UTC) #4

eae

On 2013/06/11 21:08:50, stavila wrote: > I'll make the changes you requested in the test ...

7 years, 6 months ago (2013-06-11 21:45:31 UTC) #5

esprehn

If this is a general bug it needs a non regions test, somewhere general in ...

7 years, 6 months ago (2013-06-11 22:15:21 UTC) #6

stavila

1. Moved tests from fast/regions to fast/css 2. Added test for :focus 3. Made changes ...

7 years, 6 months ago (2013-06-12 09:58:51 UTC) #7

eae

LGTM This is great, thanks for doing this and for making the suggested changes.

7 years, 6 months ago (2013-06-12 15:17:23 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stavila@adobe.com/16599003/12001

7 years, 6 months ago (2013-06-12 15:17:58 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stavila@adobe.com/16599003/31001

7 years, 6 months ago (2013-06-12 15:53:59 UTC) #11

esprehn

I just got back from vacation and finally looked closer and this patch doesn't work ...

7 years, 6 months ago (2013-06-13 00:21:11 UTC) #13

esprehn

Btw had you correctly reused the style this would have broken dragging tests because of ...

7 years, 6 months ago (2013-06-13 00:33:54 UTC) #14

esprehn

On 2013/06/13 00:33:54, esprehn wrote: > Btw had you correctly reused the style this would ...

7 years, 6 months ago (2013-06-13 02:09:00 UTC) #15

Mihai Maerean

On 2013/06/13 02:09:00, esprehn wrote: > On 2013/06/13 00:33:54, esprehn wrote: > > Btw had ...

7 years, 6 months ago (2013-06-13 05:21:28 UTC) #16

esprehn

On 2013/06/13 05:21:28, Mihai Maerean wrote: > ... > > > > I think we ...

7 years, 6 months ago (2013-06-13 05:27:38 UTC) #17

stavila

On 2013/06/13 00:21:11, esprehn wrote: > I just got back from vacation and finally looked ...

7 years, 6 months ago (2013-06-13 13:26:11 UTC) #18

Message was sent while issue was closed.

On 2013/06/13 00:21:11, esprehn wrote:
> I just got back from vacation and finally looked closer and this patch doesn't
> work because you don't pass the performingReattach information all the way
down
> the detach() recursion. If an ancestor of the :hover element is being
reattached
> we'll still lose the hover state, it just appears to sort of work because
we'll
> do another hit test.
> 
> ex. http://jsbin.com/egilet/1/ If you hover the "Foo" it'll keep flashing at
you
> since we go from :hover to not hover over and over again.
> 
> You also don't correctly use the resolvedStyle inside NodeRenderingContext
since
> createRendererForElementIfNeeded is going to do m_style =
> element->styleForRenderer(); so you never actually use that value.
> 
> I'm not sure why no one caught this before, this patch is not right.
> 
> It also looks like the patch that went into webkit reused the style properly
> because of an earlier webkit only patch related to regions:
> https://bugs.webkit.org/show_bug.cgi?id=74144 but it has the other bugs
> mentioned, and I'm pretty sure it broke :-webkit-drag.
> 
> The style reuse in
> NodeRenderingContext wasn't done right (it's dead code right now), it doesn't
> work for descendants/shadow DOM and it doesn't work for lazyReattach.

You are correct, there were a few issues which, following your recommandations,
I now addressed:
1. The style was indeed overwritten due to the subtle difference between webkit
and blink, which I missed (sorry about that).
2. Now the context is properly propagated to the element's children.
3. As a result of the fix at nr. 2, the first test you posted
(http://jsbin.com/egilet/1/) is now working properly, no more flickering.
4. Now the context is properly set on the lazyReattach method.
5. As a result of the fix at nr. 4, the second test you posted
(http://jsbin.com/aruref/1) is now working properly, no more flickering.
6. Could you elaborate on the :-webkit-drag issue? I re-ran all drag tests and
everything is fine.

I created a new issue for these changes here:
https://chromiumcodereview.appspot.com/16951003/. Please consider this solution
(of creating a separate issue) instead of a rollback of this patch and then a
complete re-upload because the changes in the already-landed patch are very
extensive and I had a lot of issues with conflicts. 
Rolling it back only to commit it again 1-2 days later would only serve to
create unnecessary problems in my opinion.

Does that sound ok for you? If so, please review the new patch I uploaded and
let me know if there are any more issues.

esprehn

7 years, 6 months ago (2013-06-13 18:25:18 UTC) #19

Message was sent while issue was closed.

On 2013/06/13 13:26:11, stavila wrote:
> ...
> 6. Could you elaborate on the :-webkit-drag issue? I re-ran all drag tests and
> everything is fine.
> 

The SelectorChecker uses the renderer to decide if the :drag pseudo matches.
Your patch changed behavior there, are you sure it's correct now? Perhaps you
fixed a bug there.

I'd really appreciate you making smaller changes in the future. We should have
done the style reuse in one patch and the :hover fix in another one. Patches
that need a bulleted list of things that are different probably are too big. :)

Expand Messages | Collapse Messages