[2.8][WebProfilerBundle] Fix search button click listener #15928
Conversation
| @@ -349,7 +349,7 @@ | |||
| } | |||
|
|
|||
| tabNavigation[j].addEventListener('click', function(e) { | |||
| var activeTab = e.target || e.srcElement; | |||
| var activeTab = this; | |||
There was a problem hiding this comment.
is this set to the current element in a consistent way cross-browser ?
There was a problem hiding this comment.
Good point @stof. When using native addEventListener I'm quite sure that this references the target element in all browsers, but I just notice Sfjs.addEventListener falls back to attachEvent if addEventListener isn't avialable (especially for IE<9) since #13636. In listeners attached with attachEvent this doesn't references the target element, without further effort.
When looking too lines below, it turns out that the navigation tab listener already considers the case of embedded html by traversing the DOM tree up to the expected tag. I overlooked that before. I will update this PR to revert this listener and use the same technique for the search button listener.
|
The PR has been updated, as @stof pointed out an issue with the usage of Is a list of supported browser versions available for the profiler? I think IE 8 must be the oldest supported IE because of the usage of |
| @@ -372,7 +372,7 @@ | |||
| }, | |||
|
|
|||
| createToggles: function() { | |||
| var toggles = document.querySelectorAll('.sf-toggle'); | |||
| var toggles = document.querySelectorAll('a.sf-toggle'); | |||
There was a problem hiding this comment.
why forcing it on <a> elements ? You could keep the class-only selector and use hasClass(toggle) in your while loop
There was a problem hiding this comment.
You are right. I adopt it from the navigation tabs, where the li elements don't have a class and the tag name is used. I agree, in case of the toggle button, verifying the class name is smarter.
|
Thanks @stof, the PR has been updated again, considering your hint. |
|
👍 thanks @xelaris |
|
Thank you @xelaris. |
…(xelaris) This PR was merged into the 2.8 branch. Discussion ---------- [2.8][WebProfilerBundle] Fix search button click listener | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | This fixes an issue when clicking the sidebar "Search" button **text** instead of the **button**. Then the click event target/srcElement is the *span* child-element, instead of the listening *a* element, which causes errors in the listener, since it expects the listening element. In consequence of that the search form isn't shown. To fix this, the same technique is used, as for the navigation tabs. Traversing the DOM up to the expected *a* element. Commits ------- f9ddddb [WebProfilerBundle] Fix search button click listener
This fixes an issue when clicking the sidebar "Search" button text instead of the button. Then the click event target/srcElement is the span child-element, instead of the listening a element, which causes errors in the listener, since it expects the listening element. In consequence of that the search form isn't shown.
To fix this, the same technique is used, as for the navigation tabs. Traversing the DOM up to the expected a element.