Bug 7555 – :hover style not applied on hover if its display property is different from original style's

• Component: CSS
• Status: RESOLVED FIXED

If an element's display property is supposed to change when hovered, then is never changes on hover. In TOT, it also drives Safari's CPU usage high for as long as the element is hovered.

See the test case for an example.

I included a fix for this as part of my now-rejected patch for bug 6431. I am going to attach that part as a proposed fix for this bug.

Created attachment 6808 [details]
Test case

Created attachment 6822 [details]
Patch

This also fixes a similar issue with :active.

A better long term fix, I think, is to address
            // ### Suboptimal. Style gets calculated again.
when re-attaching in recalcStyle().

[Darin Adler]

Comment on attachment 6822 [details]
Patch

I have a couple questions.

    1) Is there a guarantee that neither hoverNode or activeNode can be deallocated inside attach() or detach()? I seem to recall that attach() could cause an entire subframe to load and thus run JavaScript. If that's so, hoverNode and activeNode need to be RefPtr<NodeImpl> to avoid calling renderer() and parent() on a deallocated object. On the other hand, I don't see anyone doing a ref/deref on the element itself, so perhaps this is not a real issue.

    2) Regarding moving the hover/active node to the first parent that has a renderer, is that OK for the case where children are newly added? It seems that in some case the new proper hover or active node might be a child of the old one? Does this get fixed later when a hover timer fires?

It might also be nice to put:

+            m_hovered = wasHovered;
+            m_active = wasActive;

inside the "if (attached())" statement. Really a very minor thing.

(In reply to comment #3)
> (From update of attachment 6822 [details] [edit])
> I have a couple questions.
> 
>     1) Is there a guarantee that neither hoverNode or activeNode can be
> deallocated inside attach() or detach()?

I am starting to think hoverNode is pointless. The idea was not to maintain correct hover state (that's what the timer is for, in response to your second question) but only to never allow nodes with m_hovered==true to be cut out of the chain. However, detach() will reset m_hovered to false all the way down the chain, so that's not an issue.

activeNode is just as pointless in this patch, since detach() also resets m_inActiveChain, but this should be fixed by setting it back to true when climbing up the old active chain (the timer will then set m_active to true on those children that remained under the mouse, but not on any new children, since the active chain is frozen). This means I can't avoid the question of whether activeNode can be deallocated.

Created attachment 6828 [details]
Patch

Indeed, attach() can cause a subframe to load and run JavaScript that removes the element (in practice, this only increases the elements refcount after attach() since it's referenced by JavaScript objects pending GC). Therefore in this patch activeNode is a RefPtr and I added a ref/deref. This does not address the issue of callers to recalcStyle() assuming that it won't deallocate (or even just move the element). Thus the ref/deref is a non-solution to this non-problem...

Following up on my previous comment, this patch does not rebuild the hover chain but does recreate the active chain.

I also did as Darin had suggested in the end of comment #3.

[Darin Adler]

Comment on attachment 6828 [details]
Patch

Since attach() could do arbitrary amounts of work, I think this code could malfunction if there's a new hover node or active node. For example, a modal dialog might even run inside the attach().

But I have an idea about how to fix this in a way that's cleaner and more bulletproof.

We should factor the code to set the hover and active nodes out of RenderLayer:: updateHoverActiveState. That function could simply decide what the new nodes should be given the NodeInfo and then call a function in DocumentImpl to actually change the state of all the bits on the nodes.

After all, the booleans are effectively caches of the hoverNode and activeNode setting on the document so they should be maintained together.

The code in ElementImpl could then call that same function to restore the hover and active node before calling attach(). The only strange thing about this is that it would set the hover and active node to things that don't yet have a renderer.

After calling attach() it can then call a function that will move the node off of anything that doesn't have a renderer. That function, too, can be in DocumentImpl.

That way, all this code to maintain hover and active nodes can be together inside DocumentImpl, and the flags won't ever be out of sync. with the nodes in the document.

I think this approach will let you do the bug fix you're doing here and keep the code a little easier to read.

What do you think?

Created attachment 6927 [details]
Naive patch

As Darin has suggested, I moved the code that sets the hover/active/inActiveChain bets out of RenderLayer into DocumentImpl. While doing this, I introduced a couple of possible over-simplifications:

1) The new code walks up the DOM tree directly instead of walking up the render tree and looking up the corresponding elements. I couldn't figure out the case where this makes a difference (despite the comment about CSS3 ::outside).

2) The old code had a separate mustBeInActiveChain boolean, theoretically allowing for nodes not in the active chain to become hovered and active while an active chain existed. Again, I failed to come up with a case where this could actually happen (needs a non-readonly NodeInfo with the mouse button down which is not a mouseMoved and is not the initial mouseDown).

Hyatt wrote the current hover/active code so he's likely to know what cases I missed.

Comment on attachment 6927 [details]
Naive patch

Hyatt wrote the current hover/active code so he's likely to know what cases I missed.

Created attachment 7189 [details]
Naive patch

Updated to merge after the renames. Please see my earlier comments about this patch, too.

[Dave Hyatt]

CSS3 is going to introduce new types of generated content.  I believe (and I could be wrong about this, but it's what I was anticipating) that it is also going to lift the restriction on pseudo classes occurring after pseudo elements.  This means you will be able to write rules like:

div::before:hover { ... }

In the above example, a RenderObject with no corresponding node gets hovered.  The current code doesn't deal with this anyway, since the hover bit is on nodes.

But the real reason I used the render tree is that XBL is going to be introduced at some point and it will produce an altered DOM tree.  Walking the render tree will *just work* with XBL (even in the current code), since anonymous XBL shadow elements will still have DOM nodes.  

Switching to walk the DOM tree instead basically breaks code that has been future-proofed for XBL, but then again this is a classic example of why relying too much on the render tree is bad (since render objects can get torn away out from under you but DOM nodes can't).

[Dave Hyatt]

Comment on attachment 7189 [details]
Naive patch

I'm not convinced that this new code deals correctly with :active.

When the mouse goes down you put a whole chain of elements into the :active state.  That chain is then frozen and considered special, since all of the elements in that chain are the only ones that are allowed to be in :active as long as the mouse remains down.

However they can leave the :active state.  As the user moves the mouse (with the button still down) outside of elements in that chain, they have to leave the :active state.

If the user moves the mouse back into an element in the chain it has to go back into :active.

A classic example of this is form controls.  Hold your mouse down on a checkbox and watch how it goes active.  Then move the mouse (with the button still held down) outside of the checkbox and watch it pop out of :active.  Then move the mouse back in and it will return to being :active.  The current code deals with this situation correctly all the way up the :active hierarchy.

You'll need to test the new code to make sure it does.  I would really rather leave this code in RenderLayer, though, if possible, since it is "XBL-ready".

(In reply to comment #11)
> (From update of attachment 7189 [details] [edit])
> I'm not convinced that this new code deals correctly with :active.
> 
> You'll need to test the new code to make sure it does.

I tested it again and it does. The theory is that setActiveNode is called once on mouse down and establishes the active chain. It is called again on mouse up to clear the active chain. It is also called from recalcStyle to maintain the chain after detach(). setHoverNode checks if there is an active chain in place, and if so it limits it effect to the active chain, and of course sets both hovered and active state.

Created attachment 7868 [details]
Refreshed patch

A version that merges with TOT.

[jay]

http://bugs.webkit.org/show_bug.cgi?id=13046

may be related, demonstrates styles changing in SVG

Created attachment 15168 [details]
Updated patch

*** Bug 16591 has been marked as a duplicate of this bug. ***

[Robert Blaut]

(In reply to comment #15)
> Created an attachment (id=15168) [edit]
> Updated patch
> 

Why this patch isn't ready for review?

(In reply to comment #17)
> (In reply to comment #15)
> > Created an attachment (id=15168) [edit]
> > Updated patch
> > 
> 
> Why this patch isn't ready for review?

Because it does not address earlier review comments, namely the issues with XBL and generated content.

[Tom Van Vleck]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > Created an attachment (id=15168) [edit] [details]
> > > Updated patch
> > > 
> > 
> > Why this patch isn't ready for review?
> 
> Because it does not address earlier review comments, namely the issues with XBL
> and generated content.

Hey guys. I have been waiting for this bug to be fixed for a long time.
Should I assume that it will never be fixed?

[Darin Adler]

(In reply to comment #19)
> Hey guys. I have been waiting for this bug to be fixed for a long time.

Me too.

> Should I assume that it will never be fixed?

No.

[Alejandro Araneda]

Also present in "Chrome 4.0.302.3 dev" and "Safari 4.0.4 (531.21.10)" for Win.
Weird ... 46 months and counting.

[David Kilzer (:ddkilzer)]

<rdar://problem/7908601>

[Radu Stavila]

Created attachment 203593 [details]
Patch

[WebKit Commit Bot]

Attachment 203593 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/regions/hover-display-block-inline-expected.txt', u'LayoutTests/fast/regions/hover-display-block-inline.html', u'LayoutTests/fast/regions/hover-display-block-none-expected.txt', u'LayoutTests/fast/regions/hover-display-block-none.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/ContainerNode.cpp', u'Source/WebCore/dom/ContainerNode.h', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/dom/Element.h', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/dom/Node.h', u'Source/WebCore/dom/NodeRenderingContext.cpp', u'Source/WebCore/dom/PseudoElement.cpp', u'Source/WebCore/dom/PseudoElement.h', u'Source/WebCore/dom/ShadowRoot.cpp', u'Source/WebCore/dom/ShadowRoot.h', u'Source/WebCore/dom/Text.cpp', u'Source/WebCore/dom/Text.h', u'Source/WebCore/html/HTMLCanvasElement.cpp', u'Source/WebCore/html/HTMLCanvasElement.h', u'Source/WebCore/html/HTMLFormControlElement.cpp', u'Source/WebCore/html/HTMLFormControlElement.h', u'Source/WebCore/html/HTMLFrameElement.cpp', u'Source/WebCore/html/HTMLFrameElement.h', u'Source/WebCore/html/HTMLFrameElementBase.cpp', u'Source/WebCore/html/HTMLFrameElementBase.h', u'Source/WebCore/html/HTMLFrameSetElement.cpp', u'Source/WebCore/html/HTMLFrameSetElement.h', u'Source/WebCore/html/HTMLImageElement.cpp', u'Source/WebCore/html/HTMLImageElement.h', u'Source/WebCore/html/HTMLInputElement.cpp', u'Source/WebCore/html/HTMLInputElement.h', u'Source/WebCore/html/HTMLLIElement.cpp', u'Source/WebCore/html/HTMLLIElement.h', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/html/HTMLOptGroupElement.cpp', u'Source/WebCore/html/HTMLOptGroupElement.h', u'Source/WebCore/html/HTMLOptionElement.cpp', u'Source/WebCore/html/HTMLOptionElement.h', u'Source/WebCore/html/HTMLPlugInElement.cpp', u'Source/WebCore/html/HTMLPlugInElement.h', u'Source/WebCore/html/HTMLPlugInImageElement.cpp', u'Source/WebCore/html/HTMLPlugInImageElement.h', u'Source/WebCore/html/HTMLProgressElement.cpp', u'Source/WebCore/html/HTMLProgressElement.h', u'Source/WebCore/html/HTMLTextAreaElement.cpp', u'Source/WebCore/html/HTMLTextAreaElement.h', u'Source/WebCore/html/HTMLVideoElement.cpp', u'Source/WebCore/html/HTMLVideoElement.h', u'Source/WebCore/html/PluginDocument.cpp', u'Source/WebCore/html/PluginDocument.h', u'Source/WebCore/html/shadow/InsertionPoint.cpp', u'Source/WebCore/html/shadow/InsertionPoint.h', u'Source/WebCore/html/shadow/SliderThumbElement.cpp', u'Source/WebCore/html/shadow/SliderThumbElement.h', u'Source/WebCore/html/shadow/SpinButtonElement.cpp', u'Source/WebCore/html/shadow/SpinButtonElement.h', u'Source/WebCore/html/shadow/TextControlInnerElements.cpp', u'Source/WebCore/html/shadow/TextControlInnerElements.h', u'Source/WebCore/loader/PlaceholderDocument.cpp', u'Source/WebCore/loader/PlaceholderDocument.h', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/svg/SVGImageElement.cpp', u'Source/WebCore/svg/SVGImageElement.h']" exit_code: 1
Source/WebCore/html/shadow/SliderThumbElement.h:59:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Element.h:412:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Element.h:413:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/loader/PlaceholderDocument.h:40:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Element.cpp:1483:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/html/shadow/SpinButtonElement.h:73:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Document.h:535:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Document.h:536:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/ContainerNode.h:109:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/ContainerNode.h:110:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLInputElement.h:191:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLInputElement.h:354:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLTextAreaElement.h:114:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLPlugInElement.h:86:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/shadow/TextControlInnerElements.h:94:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/shadow/TextControlInnerElements.h:134:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLCanvasElement.h:147:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/shadow/InsertionPoint.h:69:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/shadow/InsertionPoint.h:70:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLFormControlElement.h:114:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLVideoElement.h:81:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLOptionElement.h:75:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLOptionElement.h:76:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLFrameSetElement.h:74:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Node.h:488:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Node.h:492:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Node.h:498:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Node.h:499:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/PluginDocument.h:47:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLFrameElement.h:42:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLLIElement.h:42:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/html/HTMLOptGroupElement.h:50:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLOptGroupElement.h:51:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/svg/SVGImageElement.h:57:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/Text.h:53:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/ShadowRoot.h:74:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLProgressElement.h:63:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLFrameElementBase.h:55:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLImageElement.h:91:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLPlugInImageElement.h:113:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLPlugInImageElement.h:114:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/PseudoElement.h:46:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLMediaElement.h:391:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 46 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Radu Stavila]

Created attachment 203597 [details]
Patch

[Antti Koivisto]

Comment on attachment 203597 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +            - 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)

It would be better to do these in pieces. At least the last one should be a separate patch. Let's just fix the hover bug here.

> Source/WebCore/dom/Element.cpp:1420
> -void Element::createRendererIfNeeded()
> +void Element::createRendererIfNeeded(RenderStyle* computedStyle)
>  {
> -    NodeRenderingContext(this).createRendererForElementIfNeeded();
> +    NodeRenderingContext(this, computedStyle).createRendererForElementIfNeeded();
>  }

I think these changes are only needed for the double style computation optimisation.

> Source/WebCore/dom/Element.cpp:1422
> +void Element::attach(const Node::AttachDetachContext* context)

There shouldn't be need to qualify this with Node:: (anywhere).

This should be a mandatory parameter and so use reference instead of pointer, const AttachContext& context.

> Source/WebCore/dom/Element.cpp:1428
> +    createRendererIfNeeded(context ? context->computedStyle.get() : 0);

Then you can get rid of all null testing.

> Source/WebCore/dom/Node.cpp:978
> +Node::AttachDetachContext::AttachDetachContext()
> +: computedStyle(0)
> +, performingReattach(false)

Indentation.

Please inline the constructor.

> Source/WebCore/dom/Node.cpp:1029
> +void Node::detach(const Node::AttachDetachContext* /*context*/)

Unnecessary comment.

> Source/WebCore/dom/Node.h:170
> +    struct AttachDetachContext {
> +        RefPtr<RenderStyle> computedStyle;
> +        bool performingReattach;
> +
> +        AttachDetachContext();
> +        ~AttachDetachContext();
> +    };

AttachDetachContext is a clumsy name. AttachContext should be fine for both cases.

Please move this next to attach()/detach() function declarations in the the class.

Remove the empty destructor.

> Source/WebCore/dom/Node.h:488
> +    virtual void attach(const AttachDetachContext* = 0);

I believe we have a pretty limited number of call sites. There shouldn't be a default value.

> Source/WebCore/dom/NodeRenderingContext.cpp:69
>  NodeRenderingContext::NodeRenderingContext(Node* node, RenderStyle* style)
>      : m_node(node)
> -    , m_renderingParent(0)
>      , m_style(style)
>      , m_parentFlowRenderer(0)
>  {
> +    m_renderingParent = NodeRenderingTraversal::parent(node, &m_parentDetails);
>  }

What is this change about?

[Radu Stavila]

Comment on attachment 203597 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +            - prevent the style from being computed twice (the attach() method used to recompute it)
> 
> It would be better to do these in pieces. At least the last one should be a separate patch. Let's just fix the hover bug here.

The speed improvement (by passing the style along and not having attach recomputing it) is just a side-effect, can't really do it separately. Before this patch, it was recomputing it inside the attach() method, but it was doing it incorrectly. However, before calling the reattach method, we already have the new style so I passed it along to the attach method who just used it. So, the fix of the problem and the speed improvement are actually the same solution.

>> Source/WebCore/dom/Element.cpp:1420
>>  }
> 
> I think these changes are only needed for the double style computation optimisation.

It is also part of the actual fix, passing the correct style along.

>> Source/WebCore/dom/NodeRenderingContext.cpp:69
>>  }
> 
> What is this change about?

It seems like those two constructors should not initialise the rendering parent differently based on if it receives a render style or not, I believe it is a mistake. As I am now constructing the NodeRenderingContext object by also passing along the RenderStyle*, I am now using the second constructor, which was not setting m_renderingParent.

[Radu Stavila]

Created attachment 203774 [details]
Patch

Implemented suggestions from anttik

[Radu Stavila]

@anttik: I could not implement your requests to inline the constructor and to remove the empty destructor of the Node::AttachContext struct. If I did that, the body of those methods would actually reside in Node.h, which doesn't have the complete definition of RenderStyle, just a forward declaration "class RenderStyle;". The complete definition is required by the "RefPtr<RenderStyle> computedStyle" member of Node::AttachContext. An alternative would be to not use RefPtr and just stick to RenderStyle*, but I think it's better to use RefPtr and keep an empty destructor in Node.cpp.

[Build Bot]

Comment on attachment 203774 [details]
Patch

Attachment 203774 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/752183

New failing tests:
fast/events/drag-display-none-element.html

[Build Bot]

Created attachment 203795 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Radu Stavila]

Created attachment 203849 [details]
Patch

[Andreas Kling]

Comment on attachment 203849 [details]
Patch

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

> Source/WebCore/dom/Node.h:479
> +        RefPtr<RenderStyle> computedStyle;

I don't think this should be a RefPtr unless the reffing is actually necessary.
Then we can also get rid of the out-of-lined member functions.

[Antti Koivisto]

Comment on attachment 203849 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:1571
> +            // Create a context for the reattach procedure to ensure that the style does not get recomputed.
> +            // During the detach procedure, the element's flags would be cleared (including the IsHovered flag)
> +            // which would cause the new style to be computed incorrectly during the attach procedure.

This comment is unclear and overly verbose. I'm not sure if a comment is needed at all really.

>> Source/WebCore/dom/Node.h:479
>> +        RefPtr<RenderStyle> computedStyle;
> 
> I don't think this should be a RefPtr unless the reffing is actually necessary.
> Then we can also get rid of the out-of-lined member functions.

Something like resolvedStyle would be a better name. "computed style" is sort of DOM level concept (getComputedStyle()).

> Source/WebCore/page/DragController.cpp:858
> -        ASSERT(m_dragSourceAction & DragSourceActionDHTML);
> -        m_client->willPerformDragSourceAction(DragSourceActionDHTML, dragOrigin, clipboard);
> -        doSystemDrag(dragImage, dragLoc, dragOrigin, clipboard, src, false);
> +        if (dragImage) {
> +            ASSERT(m_dragSourceAction & DragSourceActionDHTML);
> +            m_client->willPerformDragSourceAction(DragSourceActionDHTML, dragOrigin, clipboard);
> +            doSystemDrag(dragImage, dragLoc, dragOrigin, clipboard, src, false);
> +        } else
> +            startedDrag = false;

What is this change about?

> Source/WebCore/rendering/RenderObject.cpp:1731
> -    else
> +    else if (style)
>          setStyle(style);

And this?

You should explain all non-obvious changes in function level comments in ChangeLog.

[Radu Stavila]

Comment on attachment 203849 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:1571
>> +            // which would cause the new style to be computed incorrectly during the attach procedure.
> 
> This comment is unclear and overly verbose. I'm not sure if a comment is needed at all really.

Ok, I'll remove it.

>>> Source/WebCore/dom/Node.h:479
>>> +        RefPtr<RenderStyle> computedStyle;
>> 
>> I don't think this should be a RefPtr unless the reffing is actually necessary.
>> Then we can also get rid of the out-of-lined member functions.
> 
> Something like resolvedStyle would be a better name. "computed style" is sort of DOM level concept (getComputedStyle()).

OK.

>> Source/WebCore/page/DragController.cpp:858
>> +            startedDrag = false;
> 
> What is this change about?

With the new behaviour, an existing test would cause a crash (drag-display-none-element.html). The test was changing the display to none in the -webkit-drag pseudo-class which, up until now, had no effect. Now that the new style is actually applied (and the renderer is destroyed because of the display:none style), it would cause a crash because dragImage would be null.

>> Source/WebCore/rendering/RenderObject.cpp:1731
>>          setStyle(style);
> 
> And this?
> 
> You should explain all non-obvious changes in function level comments in ChangeLog.

This is not really required for the current patch, it's just a slight improvement. NULL-checking was performed for the first statement but overlooked for the next one. Do you think I should remove it?

[Antti Koivisto]

(In reply to comment #35)
> This is not really required for the current patch, it's just a slight improvement. NULL-checking was performed for the first statement but overlooked for the next one. Do you think I should remove it?

Better do unrelated changes separately.

[Radu Stavila]

Created attachment 203939 [details]
Patch

[Antti Koivisto]

Comment on attachment 203939 [details]
Patch

r=me

[Antti Koivisto]

Comment on attachment 203939 [details]
Patch

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

> Source/WebCore/dom/Node.h:483
> +        inline AttachContext() : resolvedStyle(0), performingReattach(false) { }
> +        inline AttachContext(const AttachContext& other) : resolvedStyle(other.resolvedStyle), performingReattach(other.performingReattach) { }

Please remove inline keywords here, they do nothing.
I don't think the explicit copy constructor is needed, default should work fine.

[Radu Stavila]

Created attachment 203942 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 203942 [details]
Patch for landing

Rejecting attachment 203942 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 203942 [details]
Patch for landing

Rejecting attachment 203942 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 203942, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/677949

[Radu Stavila]

Created attachment 203944 [details]
Patch for landing

Added "Reviewed by Antti Koivisto."

[WebKit Commit Bot]

Comment on attachment 203944 [details]
Patch for landing

Clearing flags on attachment: 203944

Committed r151282: <http://trac.webkit.org/changeset/151282>