Bug 103697

Created attachment 176904 [details] Patch

Comment on attachment 176904 [details] Patch Attachment 176904 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15059217 New failing tests: editing/inserting/insert-div-027.html accessibility/radio-button-group-members.html css3/flexbox/flex-flow-margins.html dom/xhtml/level2/html/HTMLOptionsCollection03.xhtml dom/xhtml/level2/html/HTMLDocument13.xhtml dom/html/level2/html/HTMLDocument13.html dom/html/level2/html/HTMLOptionsCollection03.html editing/execCommand/4580583-2.html css3/flexbox/preferred-widths.html css3/flexbox/flex-flow-orientations.html dom/html/level2/html/HTMLCollection10.html editing/inserting/insert-div-018.html css3/flexbox/flex-flow-margins-auto-size.html fast/block/block-remove-child-delete-line-box-crash.html css3/flexbox/flex-flow-auto-margins.html css3/flexbox/flex-flow-padding.html animations/body-removal-crash.html css3/flexbox/flex-align-end.html dom/xhtml/level2/html/HTMLCollection10.xhtml css3/flexbox/flex-flow-overflow.html editing/execCommand/5481523.html editing/inserting/insert-div-014.html dom/html/level2/html/HTMLCollection02.html editing/execCommand/4580583-1.html css3/flexbox/true-centering.html css3/flexbox/flex-flow-border.html editing/inserting/break-blockquote-after-delete.html editing/pasteboard/paste-RTFD.html dom/xhtml/level2/html/HTMLCollection02.xhtml

Created attachment 176911 [details] Patch

Comment on attachment 176911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176911&action=review > Source/WebCore/dom/Node.h:-695 > - IsHoveredFlag = 1 << 11, I'm not sure if we want to move hover flag. It's updated whenever user moves a mouse pointer. Doing hash look ups on every mouse move doesn't seem like a good idea. Focus & active on the other hand only respond to mouse clicks, so they're probably not as crucial. Also, why don't we just move them to NodeRareData instead of adding a new class & hanging it off of Document?

Comment on attachment 176911 [details] Patch Attachment 176911 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15055337 New failing tests: http/tests/local/drag-over-remote-content.html css3/flexbox/flex-flow-margins.html dom/xhtml/level2/html/HTMLOptionsCollection03.xhtml dom/xhtml/level2/html/HTMLDocument13.xhtml dom/html/level2/html/HTMLDocument13.html dom/html/level2/html/HTMLOptionsCollection03.html editing/execCommand/4580583-2.html css3/flexbox/preferred-widths.html css3/flexbox/flex-flow-orientations.html dom/html/level2/html/HTMLCollection10.html css3/flexbox/flex-flow-overflow.html css3/flexbox/flex-flow-margins-auto-size.html animations/suspend-resume-animation-events.html http/tests/inspector/console-cd-completions.html css3/flexbox/flex-flow-auto-margins.html css3/flexbox/flex-flow-padding.html http/tests/misc/char-encoding-in-hidden-charset-field-default.html animations/body-removal-crash.html css3/flexbox/flex-align-end.html dom/xhtml/level2/html/HTMLCollection10.xhtml http/tests/misc/multiple-submit.html editing/execCommand/5481523.html dom/html/level2/html/HTMLCollection02.html http/tests/inspector/console-cd.html http/tests/misc/form-post-textplain.html editing/execCommand/4580583-1.html http/tests/misc/post-submit-button.html css3/flexbox/true-centering.html css3/flexbox/flex-flow-border.html dom/xhtml/level2/html/HTMLCollection02.xhtml

Created attachment 176928 [details] Patch

I expect Document::m_focusedNode, m_activeNode and m_hoverNode can also be moved to this class. That will make it easy to track lifetime of these nodes. But it should happen in separate changes.

Created attachment 176930 [details] Patch

Comment on attachment 176930 [details] Patch Attachment 176930 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15065167 New failing tests: fast/html/details-add-summary-10-and-click.html fast/html/details-add-summary-8-and-click.html fast/html/details-add-summary-1-and-click.html fast/html/details-add-summary-2-and-click.html fast/html/details-remove-summary-6-and-click.html fast/html/details-add-summary-7-and-click.html fast/html/details-remove-summary-5-and-click.html fast/html/details-add-summary-5-and-click.html fast/html/details-remove-summary-1-and-click.html fast/html/details-add-summary-6-and-click.html fast/html/details-remove-summary-4-and-click.html fast/html/details-add-summary-3-and-click.html fast/html/details-remove-summary-3-and-click.html fast/html/details-add-summary-4-and-click.html fast/html/details-remove-summary-2-and-click.html fast/html/details-add-summary-9-and-click.html

Comment on attachment 176930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176930&action=review > Source/WebCore/dom/Node.cpp:919 > +bool Node::respondingNodeIs(RespondingNodeFlags flags) const > +{ > + ASSERT(responding()); > + if (Document* doc = document()) > + return doc->respondingNodes()->hasFlags(this, flags); > + return false; > } respondingNodeIs function seems bit unnecessary. Why not just call respondingNodes()->isActive() from Node::active() directly, similar to the setters? Then Node won't have to know about the flags at all. > Source/WebCore/dom/Node.h:326 > + bool responding() const { return getFlag(IsRespondingFlag); } isResponding(). The existing accessors don't follow the pattern but there is not need why new ones wouldn't. > Source/WebCore/dom/Node.h:334 > + void setResponding(bool flag) { setFlag(flag, IsRespondingFlag); } > + > + enum RespondingNodeFlags { > + IsActiveFlag = 1 , > + InActiveChainFlag = 1 << 1, > + IsHoveredFlag = 1 << 2, > + IsFocusedFlag = 1 << 3 > + }; Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags. I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though. > Source/WebCore/dom/Node.h:340 > + bool respondingNodeIs(RespondingNodeFlags) const; This should be private. > Source/WebCore/dom/RespondingNodeSet.h:39 > +class RespondingNodeSet { Do these flags ever have meaning on non-Elements? If not this could be tightened to be RespondingElementSet. > Source/WebCore/dom/RespondingNodeSet.h:64 > + void documentRemovedLastRef() { m_nodes.clear(); } documentDidRemoveLastRef

Comment on attachment 176930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176930&action=review >> Source/WebCore/dom/Node.h:334 >> + }; > > Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags. > > I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though. The CSS spec calls these "User Action pseudo classes". It's a bit wordy, but InteractivePseudoFlags maybe?

(In reply to comment #11) > (From update of attachment 176930 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176930&action=review > > >> Source/WebCore/dom/Node.h:334 > >> + }; > > > > Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags. > > > > I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though. > > The CSS spec calls these "User Action pseudo classes". It's a bit wordy, but InteractivePseudoFlags maybe? So these nodes are interacting. InteractingNodeSet might be better name then. I'll update the patch that way. Thanks for suggestions, and reviews!

> > > Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags. > > > > > > I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though. > > > > The CSS spec calls these "User Action pseudo classes". It's a bit wordy, but InteractivePseudoFlags maybe? > So these nodes are interacting. InteractingNodeSet might be better name then. > I'll update the patch that way. Thanks for suggestions, and reviews! Yeah. I think that's better. Although, "interacting" sounds to me like the nodes are interacting with each other. So, I think InteractiveNodeSet would be a little less confusing. It's still not a very clear name. I'm having trouble thinking of anything better.

(In reply to comment #13) > > > > Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags. > > > > > > > > I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though. > > > > > > The CSS spec calls these "User Action pseudo classes". It's a bit wordy, but InteractivePseudoFlags maybe? > > So these nodes are interacting. InteractingNodeSet might be better name then. > > I'll update the patch that way. Thanks for suggestions, and reviews! > > Yeah. I think that's better. Although, "interacting" sounds to me like the nodes are interacting with each other. So, I think InteractiveNodeSet would be a little less confusing. How about NodesInUserInteraction or NodesWithUserAction?

Created attachment 177178 [details] Patch

Comment on attachment 176930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176930&action=review Thanks for the review, Antti! I updated the patch. >> Source/WebCore/dom/Node.cpp:919 >> } > > respondingNodeIs function seems bit unnecessary. Why not just call respondingNodes()->isActive() from Node::active() directly, similar to the setters? Then Node won't have to know about the flags at all. I tried to hide RespondingNodeSet.h in Node.cpp. But it might have been overkill. I moved it to Node.h and get rid of this indirection. >> Source/WebCore/dom/Node.h:326 >> + bool responding() const { return getFlag(IsRespondingFlag); } > > isResponding(). The existing accessors don't follow the pattern but there is not need why new ones wouldn't. Right done. >>>> Source/WebCore/dom/Node.h:334 >>>> + }; >>> >>> Instead of being in Node, RespondingNodeFlags might be more natural as RespondingNodeSet::Flags. >>> >>> I'm not sure "responding" really captures these well. Responding to what? I don't have better suggestions though. >> >> The CSS spec calls these "User Action pseudo classes". It's a bit wordy, but InteractivePseudoFlags maybe? > > So these nodes are interacting. InteractingNodeSet might be better name then. > I'll update the patch that way. Thanks for suggestions, and reviews! I moved these flags to UserActionElementSet (yes, this is new name). >> Source/WebCore/dom/RespondingNodeSet.h:39 >> +class RespondingNodeSet { > > Do these flags ever have meaning on non-Elements? If not this could be tightened to be RespondingElementSet. Right. These for selector matching thus makes sense only for Elements. Updated patch reflects this fact. >> Source/WebCore/dom/RespondingNodeSet.h:64 >> + void documentRemovedLastRef() { m_nodes.clear(); } > > documentDidRemoveLastRef Renamed.

I would not call this “cleanup”. I retitled the bug.

Comment on attachment 177178 [details] Patch Causing many failures in fast/html/details-*

Created attachment 177868 [details] Patch

> Created an attachment (id=177868) [details] > Patch Fixed broken tests, addressed misleading title.

Comment on attachment 177868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177868&action=review > Source/WebCore/dom/Node.h:712 > + IsInUserActionFlag = 1 << 10, IsUserActionElementFlag/IsInUserActionElementSetFlag or similar. > Source/WebCore/dom/UserActionElementSet.h:48 > +public: > + enum ElementFlags { > + IsActiveFlag = 1 , > + InActiveChainFlag = 1 << 1, > + IsHoveredFlag = 1 << 2, > + IsFocusedFlag = 1 << 3 > + }; I think this enum can be private. > Source/WebCore/dom/UserActionElementSet.h:58 > + static PassOwnPtr<UserActionElementSet> create() { return adoptPtr(new UserActionElementSet()); } > + static bool isFocused(const Node* node) { return hasFlags(node, IsFocusedFlag); } > + static void setFocused(Node* node, bool enable) { setFlags(node, enable, IsFocusedFlag); } > + static bool isActive(const Node* node) { return hasFlags(node, IsActiveFlag); } > + static void setActive(Node* node, bool enable) { setFlags(node, enable, IsActiveFlag); } > + static bool isInActiveChain(const Node* node) { return hasFlags(node, InActiveChainFlag); } > + static void setInActiveChain(Node* node, bool enable) { setFlags(node, InActiveChainFlag, enable); } > + static bool isHovered(const Node* node) { return hasFlags(node, IsHoveredFlag); } > + static void setHovered(Node* node, bool enable) { setFlags(node, enable, IsHoveredFlag); } Jumping trough these static functions seems strange and unnecessary. Why is this done?

Comment on attachment 177868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177868&action=review >> Source/WebCore/dom/Node.h:712 >> + IsInUserActionFlag = 1 << 10, > > IsUserActionElementFlag/IsInUserActionElementSetFlag or similar. Sure. Will fix. >> Source/WebCore/dom/UserActionElementSet.h:48 >> + }; > > I think this enum can be private. Right. will do. >> Source/WebCore/dom/UserActionElementSet.h:58 >> + static void setHovered(Node* node, bool enable) { setFlags(node, enable, IsHoveredFlag); } > > Jumping trough these static functions seems strange and unnecessary. Why is this done? Yeah, ideally we could write document()->userActionElementSet().isFocused() or something but we need some kind of indirection like this since neither Node.h and UserActionElement.h cannot depend on Document which owns UserActionElement. We can access Document::userActionElementSet() only inside .cpp files due to this dependency restriction. But I agree that having many static functions looks strange. Will try to move them to non-statics.

Comment on attachment 177868 [details] Patch Attachment 177868 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15158538 New failing tests: touchadjustment/touch-links-active.html fast/events/touch/gesture/gesture-tap-active-state.html editing/selection/editable-links.html fast/css/hover-active-drag.html fast/forms/search-cancel-button-mouseup.html fast/events/touch/gesture/gesture-tap-active-state-iframe.html

Comment on attachment 177868 [details] Patch Attachment 177868 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15157612 New failing tests: touchadjustment/touch-links-active.html fast/events/touch/gesture/gesture-tap-active-state.html editing/selection/editable-links.html fast/css/hover-active-drag.html fast/forms/search-cancel-button-mouseup.html fast/events/touch/gesture/gesture-tap-active-state-iframe.html

Created attachment 177918 [details] Patch

(In reply to comment #25) > Created an attachment (id=177918) [details] > Patch Addressed Antti's point, fixed broken test (which was caused by a bad copy-n-paste)

Comment on attachment 177918 [details] Patch Attachment 177918 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15170594

Created attachment 178126 [details] Patch

Bots are all green! Antti, could you take another look when you have time?

Comment on attachment 178126 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178126&action=review I’ve started to think that maybe it’s better to make NodeRareData deletable. > Source/WebCore/ChangeLog:13 > + aElementFlags. Typo: aElementFlags. > Source/WebCore/ChangeLog:22 > + flags. However, there are certain differences, that are why such a > + data structure is worth adding: This sentence doesn’t read smoothly. How about something like "however, the following characteristics of these flags make adding a new data structure on Document an attractive alternative".

Comment on attachment 178126 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178126&action=review > Source/WebCore/dom/UserActionElementSet.cpp:38 > + if (Document* doc = node->document()) Please don’t abbreviate document to doc in new code. Just use the whole word.

Created attachment 178450 [details] Patch

Comment on attachment 178126 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178126&action=review Ryosuke, Dari, Thanks for the review! I updated the patch to address these points. >> Source/WebCore/ChangeLog:13 >> + aElementFlags. > > Typo: aElementFlags. Fixed. >> Source/WebCore/ChangeLog:22 >> + data structure is worth adding: > > This sentence doesn’t read smoothly. How about something like "however, the following characteristics of these flags make adding a new data structure on Document an attractive alternative". Sounds good. I copy-pasted this. >> Source/WebCore/dom/UserActionElementSet.cpp:38 >> + if (Document* doc = node->document()) > > Please don’t abbreviate document to doc in new code. Just use the whole word. Right. I should get rid of my bad habit :-/

Created attachment 178696 [details] Patch

I talked Antti at IRC and updated the patch to take his advice.

Comment on attachment 178696 [details] Patch Looks fine to me.

Comment on attachment 178696 [details] Patch Thanks! Landing.

Yeh, looks good.

Comment on attachment 178696 [details] Patch Clearing flags on attachment: 178696 Committed r137277: <http://trac.webkit.org/changeset/137277>

All reviewed patches have been landed. Closing bug.