diff --git a/.agent/plans/eepp_css_selector_optimization_plan.md b/.agent/plans/eepp_css_selector_optimization_plan.md index f072e71ec..8f3bf3151 100644 --- a/.agent/plans/eepp_css_selector_optimization_plan.md +++ b/.agent/plans/eepp_css_selector_optimization_plan.md @@ -60,6 +60,8 @@ Do not start with a large rewrite. Measure after each phase. # Phase 1: Inline Tiny Hot-Path Accessors +**Status: Implemented** + ## Motivation CSS selector matching repeatedly calls small accessors and type checks while walking parents/siblings and matching selector rules. These functions are ideal candidates for header inlining. @@ -189,6 +191,8 @@ Create an HTML tree containing a normal eepp widget between/inside HTML elements # Phase 2: Fix Confirmed Sibling Combinator Direction Bug +**Status: Implemented** + ## Motivation This is a confirmed correctness bug, not intentional behavior. Right-to-left selector matching for CSS combinators should follow CSS semantics: @@ -1142,4 +1146,3 @@ Inlining is worthwhile but not the main algorithmic win. The main problem is can - Keep string fields even after adding hashes to avoid breaking public APIs, debug output, CSS serialization, and tests. - Prefer minimal changes with tests after each phase. - Do not implement signature caching or sort redesign until instrumentation proves they are still needed. - diff --git a/include/eepp/scene/node.hpp b/include/eepp/scene/node.hpp index ded70e251..56ac4a219 100644 --- a/include/eepp/scene/node.hpp +++ b/include/eepp/scene/node.hpp @@ -215,7 +215,7 @@ class EE_API Node : public Transformable { virtual bool isType( const Uint32& type ) const; /** @return True if this node is a UITextNode, false otherwise. */ - bool isTextNode() const; + inline bool isTextNode() const { return 0 != ( mNodeFlags & NODE_FLAG_TEXTNODE ); } /** * @brief Posts a message to this node and its ancestors. @@ -369,7 +369,7 @@ class EE_API Node : public Transformable { * * @return Pointer to the parent node or nullptr. */ - Node* getParent() const; + inline Node* getParent() const { return mParentNode; } /** * @brief Sets the parent node. @@ -429,7 +429,7 @@ class EE_API Node : public Transformable { * * @return Pointer to the next sibling node or nullptr. */ - Node* getNextNode() const; + inline Node* getNextNode() const { return mNext; } /** * @brief Gets the previous sibling node in the parent's child list. @@ -439,7 +439,7 @@ class EE_API Node : public Transformable { * * @return Pointer to the previous sibling node or nullptr. */ - Node* getPrevNode() const; + inline Node* getPrevNode() const { return mPrev; } /** * @brief Gets the next sibling node, wrapping to first if at end. @@ -525,7 +525,7 @@ class EE_API Node : public Transformable { * * @return The flags as a Uint32 bitmask. */ - const Uint32& getNodeFlags() const; + inline const Uint32& getNodeFlags() const { return mNodeFlags; } /** * @brief Sets the node flags directly. @@ -571,7 +571,7 @@ class EE_API Node : public Transformable { * * @return True if this node is a UIWidget, false otherwise. */ - bool isWidget() const; + inline bool isWidget() const { return 0 != ( mNodeFlags & NODE_FLAG_WIDGET ); } /** * @brief Checks if this node is a Window. diff --git a/include/eepp/ui/uiwidget.hpp b/include/eepp/ui/uiwidget.hpp index 2cd199503..413d85a41 100644 --- a/include/eepp/ui/uiwidget.hpp +++ b/include/eepp/ui/uiwidget.hpp @@ -784,7 +784,11 @@ class EE_API UIWidget : public UINode { * * @return Pointer to the parent element widget or nullptr. */ - UIWidget* getStyleSheetParentElement() const; + inline UIWidget* getStyleSheetParentElement() const { + return NULL != mParentNode && mParentNode->isWidget() && getType() != UI_TYPE_HTML_HTML + ? mParentNode->asType() + : NULL; + } /** * @brief Gets the previous sibling element for CSS styling. @@ -793,7 +797,15 @@ class EE_API UIWidget : public UINode { * * @return Pointer to the previous sibling element widget or nullptr. */ - UIWidget* getStyleSheetPreviousSiblingElement() const; + inline UIWidget* getStyleSheetPreviousSiblingElement() const { + Node* node = mPrev; + while ( NULL != node ) { + if ( node->isWidget() && !node->isTextNode() ) + return node->asType(); + node = node->getPrevNode(); + } + return NULL; + } /** * @brief Gets the next sibling element for CSS styling. @@ -802,7 +814,15 @@ class EE_API UIWidget : public UINode { * * @return Pointer to the next sibling element widget or nullptr. */ - UIWidget* getStyleSheetNextSiblingElement() const; + inline UIWidget* getStyleSheetNextSiblingElement() const { + Node* node = mNext; + while ( NULL != node ) { + if ( node->isWidget() && !node->isTextNode() ) + return node->asType(); + node = node->getNextNode(); + } + return NULL; + } /** * @brief Gets the active pseudo-classes for this widget. @@ -811,7 +831,7 @@ class EE_API UIWidget : public UINode { * * @return Bitmask of active pseudo-classes. */ - Uint32 getStyleSheetPseudoClasses() const { return mPseudoClasses; } + inline Uint32 getStyleSheetPseudoClasses() const { return mPseudoClasses; } /** * @brief Gets the pseudo-classes as string array. diff --git a/src/eepp/scene/node.cpp b/src/eepp/scene/node.cpp index 5871ebcff..bddc1ad30 100644 --- a/src/eepp/scene/node.cpp +++ b/src/eepp/scene/node.cpp @@ -227,10 +227,6 @@ bool Node::isDisabled() const { return !mEnabled; } -Node* Node::getParent() const { - return mParentNode; -} - void Node::updateDrawInvalidator( bool force ) { mNodeDrawInvalidator = getDrawInvalidator(); @@ -431,14 +427,6 @@ void Node::onClose() { sendCommonEvent( Event::OnClose ); } -Node* Node::getNextNode() const { - return mNext; -} - -Node* Node::getPrevNode() const { - return mPrev; -} - Node* Node::getNextNodeLoop() const { if ( NULL == mNext ) return getParent()->getFirstChild(); @@ -527,10 +515,6 @@ Rectf Node::getLocalBounds() const { return Rectf( 0, 0, mSize.getWidth(), mSize.getHeight() ); } -const Uint32& Node::getNodeFlags() const { - return mNodeFlags; -} - void Node::setNodeFlags( const Uint32& flags ) { mNodeFlags = flags; } @@ -1094,14 +1078,6 @@ void Node::onSceneChange() { } } -bool Node::isWidget() const { - return 0 != ( mNodeFlags & NODE_FLAG_WIDGET ); -} - -bool Node::isTextNode() const { - return 0 != ( mNodeFlags & NODE_FLAG_TEXTNODE ); -} - bool Node::isWindow() const { return 0 != ( mNodeFlags & NODE_FLAG_WINDOW ); } diff --git a/src/eepp/ui/css/stylesheetselector.cpp b/src/eepp/ui/css/stylesheetselector.cpp index c7ff29844..14d448839 100644 --- a/src/eepp/ui/css/stylesheetselector.cpp +++ b/src/eepp/ui/css/stylesheetselector.cpp @@ -189,7 +189,7 @@ bool StyleSheetSelector::select( UIWidget* element, const bool& applyPseudo ) co break; // continue evaluating } case StyleSheetSelectorRule::PREVIOUS_SIBLING: { - curElement = curElement->getStyleSheetPreviousSiblingElement(); + curElement = curElement->getStyleSheetNextSiblingElement(); if ( NULL == curElement || !selectorRule.matches( curElement, applyPseudo ) ) return false; @@ -197,7 +197,7 @@ bool StyleSheetSelector::select( UIWidget* element, const bool& applyPseudo ) co break; // continue evaluating } case StyleSheetSelectorRule::DIRECT_SIBLING: { - curElement = curElement->getStyleSheetNextSiblingElement(); + curElement = curElement->getStyleSheetPreviousSiblingElement(); if ( NULL == curElement || !selectorRule.matches( curElement, applyPseudo ) ) return false; @@ -206,24 +206,13 @@ bool StyleSheetSelector::select( UIWidget* element, const bool& applyPseudo ) co } case StyleSheetSelectorRule::SIBLING: { bool foundSibling = false; - UIWidget* prevSibling = curElement->getStyleSheetPreviousSiblingElement(); - UIWidget* nextSibling = curElement->getStyleSheetNextSiblingElement(); - while ( NULL != prevSibling && !foundSibling ) { - if ( selectorRule.matches( prevSibling, applyPseudo ) ) { + for ( UIWidget* sibling = curElement->getStyleSheetPreviousSiblingElement(); + NULL != sibling; sibling = sibling->getStyleSheetPreviousSiblingElement() ) { + if ( selectorRule.matches( sibling, applyPseudo ) ) { + curElement = sibling; foundSibling = true; - } else { - prevSibling = prevSibling->getStyleSheetPreviousSiblingElement(); - } - } - - if ( !foundSibling ) { - while ( NULL != nextSibling && !foundSibling ) { - if ( selectorRule.matches( nextSibling, applyPseudo ) ) { - foundSibling = true; - } else { - nextSibling = nextSibling->getStyleSheetNextSiblingElement(); - } + break; } } @@ -321,26 +310,13 @@ std::vector StyleSheetSelector::getRelatedElements( UIWidget* element } case StyleSheetSelectorRule::SIBLING: { bool foundSibling = false; - UIWidget* prevSibling = curElement->getStyleSheetPreviousSiblingElement(); - UIWidget* nextSibling = curElement->getStyleSheetNextSiblingElement(); - while ( NULL != prevSibling && !foundSibling ) { - if ( selectorRule.matches( prevSibling, applyPseudo ) ) { + for ( UIWidget* sibling = curElement->getStyleSheetPreviousSiblingElement(); + NULL != sibling; sibling = sibling->getStyleSheetPreviousSiblingElement() ) { + if ( selectorRule.matches( sibling, applyPseudo ) ) { foundSibling = true; - curElement = prevSibling; - } else { - prevSibling = prevSibling->getStyleSheetPreviousSiblingElement(); - } - } - - if ( !foundSibling ) { - while ( NULL != nextSibling && !foundSibling ) { - if ( selectorRule.matches( nextSibling, applyPseudo ) ) { - foundSibling = true; - curElement = nextSibling; - } else { - nextSibling = nextSibling->getStyleSheetNextSiblingElement(); - } + curElement = sibling; + break; } } diff --git a/src/eepp/ui/uiwidget.cpp b/src/eepp/ui/uiwidget.cpp index ce986d8bd..90bc00664 100644 --- a/src/eepp/ui/uiwidget.cpp +++ b/src/eepp/ui/uiwidget.cpp @@ -1050,32 +1050,6 @@ const std::string& UIWidget::getStyleSheetTag() const { return mTag; } -UIWidget* UIWidget::getStyleSheetParentElement() const { - return NULL != mParentNode && mParentNode->isWidget() && getType() != UI_TYPE_HTML_HTML - ? mParentNode->asType() - : NULL; -} - -UIWidget* UIWidget::getStyleSheetPreviousSiblingElement() const { - Node* node = mPrev; - while ( NULL != node ) { - if ( node->isWidget() && !node->isTextNode() ) - return node->asType(); - node = node->getPrevNode(); - } - return NULL; -} - -UIWidget* UIWidget::getStyleSheetNextSiblingElement() const { - Node* node = mNext; - while ( NULL != node ) { - if ( node->isWidget() && !node->isTextNode() ) - return node->asType(); - node = node->getNextNode(); - } - return NULL; -} - std::vector UIWidget::getStyleSheetPseudoClassesStrings() const { return StyleSheetSelectorRule::fromPseudoClass( mPseudoClasses ); } diff --git a/src/tests/unit_tests/uihtml_tests.cpp b/src/tests/unit_tests/uihtml_tests.cpp index dac94d2ce..b10823795 100644 --- a/src/tests/unit_tests/uihtml_tests.cpp +++ b/src/tests/unit_tests/uihtml_tests.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -2695,6 +2696,113 @@ ul > li { Engine::destroySingleton(); } +UTEST( UIHTML, StyleSheetTraversalBoundaries ) { + Engine::instance()->createWindow( WindowSettings( 1024, 768, "CSS Traversal Test", + WindowStyle::Default, WindowBackend::Default, + 32, {}, 1, false, true ), + ContextSettings( false, 0, 0, GLv_default, true, false ) ); + + UISceneNode* sceneNode = init_test_inline_block(); + const std::string html = R"html( + + + + + + + + text +
+ + +)html"; + + sceneNode->loadLayoutFromString( HTMLFormatter::HTMLtoXML( html ) ); + sceneNode->update( Seconds( 1 ) ); + + auto* htmlNode = sceneNode->getRoot()->findByType( UI_TYPE_HTML_HTML )->asType(); + auto* body = sceneNode->getRoot()->findByType( UI_TYPE_HTML_BODY )->asType(); + auto* a = sceneNode->getRoot()->find( "a" )->asType(); + auto* b = sceneNode->getRoot()->find( "b" )->asType(); + ASSERT_TRUE( htmlNode != nullptr ); + ASSERT_TRUE( body != nullptr ); + ASSERT_TRUE( a != nullptr ); + ASSERT_TRUE( b != nullptr ); + + EXPECT_TRUE( htmlNode->getStyleSheetParentElement() == nullptr ); + EXPECT_EQ( CSSDisplay::InlineBlock, a->getDisplay() ); + EXPECT_TRUE( b->getStyleSheetPreviousSiblingElement() == a ); + + auto* nativeWidget = UIWidget::New(); + nativeWidget->setParent( body ); + auto* nativeChild = UIWidget::New(); + nativeChild->setParent( nativeWidget ); + EXPECT_TRUE( nativeWidget->getStyleSheetPreviousSiblingElement() == b ); + EXPECT_TRUE( nativeChild->getStyleSheetParentElement() == nativeWidget ); + + Engine::destroySingleton(); +} + +UTEST( UIHTML, StyleSheetSiblingCombinators ) { + Engine::instance()->createWindow( WindowSettings( 1024, 768, "CSS Sibling Test", + WindowStyle::Default, WindowBackend::Default, + 32, {}, 1, false, true ), + ContextSettings( false, 0, 0, GLv_default, true, false ) ); + + UISceneNode* sceneNode = init_test_inline_block(); + const std::string html = R"html( + + + + + + +
+ text +
+ +
+ + +)html"; + + sceneNode->loadLayoutFromString( HTMLFormatter::HTMLtoXML( html ) ); + sceneNode->update( Seconds( 1 ) ); + + auto* a = sceneNode->getRoot()->find( "a" )->asType(); + auto* b = sceneNode->getRoot()->find( "b" )->asType(); + auto* c = sceneNode->getRoot()->find( "c" )->asType(); + ASSERT_TRUE( a != nullptr ); + ASSERT_TRUE( b != nullptr ); + ASSERT_TRUE( c != nullptr ); + + EXPECT_TRUE( b->getBackgroundColor() == Color::Red ); + EXPECT_TRUE( c->getBackgroundColor() == Color::Green ); + EXPECT_TRUE( a->getBackgroundColor() != Color::Blue ); + + StyleSheetSelector directRelated( "#a:hover + #b" ); + auto related = directRelated.getRelatedElements( b, false ); + ASSERT_EQ( 1u, related.size() ); + EXPECT_TRUE( related.front() == a ); + + StyleSheetSelector futureGeneralSibling( "#c:hover ~ #a" ); + EXPECT_TRUE( futureGeneralSibling.getRelatedElements( a, false ).empty() ); + + StyleSheetSelector inverseSibling( "#b | #a" ); + EXPECT_TRUE( inverseSibling.select( a ) ); + EXPECT_FALSE( inverseSibling.select( b ) ); + + Engine::destroySingleton(); +} + UTEST( UIHTML, BlockList ) { Engine::instance()->createWindow( WindowSettings( 1024, 768, "Block List Test", WindowStyle::Default, WindowBackend::Default,